[OPIK-8062] [SDK] feat: one-command MCP + skill pack setup - #7958
Conversation
…+ opencode Half of MCP installs never issue a tool call. The install itself is the onboarding, and it asked permission it should assume, verified nothing, reached three of the seven hosts users actually run, and could not be scripted at all. - Honour `--install-mcp` without a TTY. The interactivity guard was checked before the explicit flag, so `opik configure -y --install-mcp` silently did nothing in exactly the environments people automate — and exited 0. `-y` alone still skips MCP; a blanket yes should not edit another tool's config. - Add `--host` to `opik mcp configure` (repeatable, or `all`). A terminal was only ever needed to *ask* which host to use, so naming one lets the command run from a coding agent, a Dockerfile, or CI. An explicit host installs whether or not it is detected, so a fresh image can be configured before the editor is. - Verify before claiming success. The installer wrote JSON and told the user to go check; an unconfigured server starts happily and advertises every tool, so a broken setup was indistinguishable from a working one until the agent hit a 401 mid-conversation. Now it makes a real call with the values it just wrote and reports the workspace and project count, or fails with the reason. - Add Codex and opencode host targets. Codex is the highest-volume, most reliable client in the telemetry and was hand-writing JSON. Codex is driven through its own CLI (its config is TOML, which we will not hand-edit) and read back via `codex mcp get --json`; opencode gets its own block shape (`local`/`remote`, argv as one list, `environment`). - Refuse to guess the workspace. An unnamed workspace makes the server send `default`, which resolves to the account default — so reads come back from the wrong place instead of failing. On an account with several workspaces we now stop and say so. A failed lookup is not treated as evidence of one workspace. - Name the detected host in the consent prompt, and stop asking twice: the configurator's prompt and the installer's picker were two questions about the same thing. Default stays "no". - Name the exact `uv` install command per platform when `uvx` is missing. Analytics is deliberately not wired here — it ships on a separate branch. `ANALYTICS:` comments mark each call site and the event it owes. Unit tests gained an autouse stub for the new verification call, so the configurator suite no longer reaches the network (65s -> 2s). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MCP page carried the CLI changes, but three other surfaces were left stale or silent: - `sdk_configuration.mdx` documented `opik configure --use_local` and `--yes` but never mentioned `--install-mcp`, so the flag was only discoverable by reading `--help`. It now has its own subsection, including the deliberate `--yes` / `--install-mcp` distinction. - `home.mdx` and `integrations/overview.mdx` advertised the MCP server as Claude Code / Cursor / VS Code Copilot only. No changelog entry: those are cut as weekly batches by the release owner, and inventing a mid-cycle dated file would fake a release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
⏱️ pre-commit per-hook timing
⏭️ 39 skipped (no matching files changed)
|
|
Already covered by a test in this PR. This is Python SDK/CLI surface only: the new assistant step in also touches Python SDK Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 28 Aug 12:19 UTC — nothing the verdict depends on changed. |
|
🌿 Preview your docs: https://opik-preview-01a0484f-7e80-7007-a310-db2694f9f0e3.docs.buildwithfern.com/docs/opik No broken links found Unverified links (timeout / rate-limited / server error — not failing the check)• https://ai.pydantic.dev/ (timeout) 📌 Results for commit 7530b87 |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
The MCP server gives an assistant tools; the skill pack gives it the knowledge
of how to use Opik. The telemetry says the second half is the gap — 43 installs
have loaded the tool list 14,120 times between them without ever calling a
tool, and `schema` is the most widely-called tool of all. Agents are connected
and groping.
New `opik skills` group (configure / status / remove), plus
`opik configure --install-skills` and an interactive prompt after the MCP one.
The two are asked separately on purpose: MCP writes credentials into a config
file the user already trusts with them, while this writes instruction files the
assistant executes with its own permissions. Same host list, different consent.
No third-party installer is involved. Skills are `SKILL.md` directories and the
assistants have converged on a shared user-level location, so this is a tarball
fetch plus a link:
- Codex resolves `$HOME/.agents/skills` as a user-scope root
(`codex-rs/ext/skills/src/host_roots.rs`, `ConfigLayerSource::User`).
- opencode loads global skills from `~/.agents/skills`, `~/.claude/skills` and
`~/.config/opencode/skills`.
- Cursor and VS Code Copilot read the shared directory.
- Claude Code is the exception — it reads `~/.claude/skills`, so it gets a
symlink into the shared copy (a copy on Windows, where symlinks need
elevation).
So one write plus one link covers every host, with no Node, no `npx`, and no
external CLI whose flags can change under us. It also means the install is
HOME-scoped and independent of the working directory, matching the MCP install
— there is no project to be inside — and needs no Opik credentials, so it works
before `opik configure`.
Extraction is hand-rolled rather than `TarFile.extractall`: the `filter="data"`
argument that makes that safe is 3.12+, and the SDK supports 3.10. Every member
is validated instead — regular files only, no absolute paths, no `..`, size
caps on the archive and each file. A test caught `PurePosixPath(".").parts`
being empty, which made the traversal guard vacuously true and let `.` through
as a skill name.
Version tracking uses a content digest rather than a commit sha: the codeload
tarball names its root after the ref, not the commit, so a sha is not available
without a second request. `~/.agents/skills/.opik-skills.json` records it.
A pack present but unrecorded is reported as installed outside the CLI rather
than ignored, since `npx skills add comet-ml/opik-skills` writes to the same
place; the two are interchangeable. `remove` only touches what we recorded
installing, so a hand-written skill sharing a name survives.
Also flags the one known duplicate: opik-claude-code-plugin ships its own
`opik` skill whose content has drifted from the pack's.
Analytics stays on its own branch; `ANALYTICS:` comments mark the call sites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alexkuzmik
changed the title
| with httpx_client.get( | ||
| workspace=None, | ||
| api_key=None, | ||
| check_tls_certificate=True, | ||
| compress_json_requests=False, | ||
| ) as client: | ||
| response = client.get( | ||
| url, timeout=DOWNLOAD_TIMEOUT_SECONDS, follow_redirects=True |
There was a problem hiding this comment.
Inconsistent HTTP timeout behavior
The skill-pack download hard-codes timeout=DOWNLOAD_TIMEOUT_SECONDS instead of using the shared httpx_client timeout, so centralized environment or application timeout changes do not affect skill installation — should we use the client timeout or expose this value through shared configuration?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/configurator/skills/pack.py around lines 86-93, update the
`download` method so the skill-pack request does not hard-code
`timeout=DOWNLOAD_TIMEOUT_SECONDS` and bypass the centralized `httpx_client`
configuration. Refactor it to use the shared client timeout, or expose the timeout
through the shared configuration while preserving an appropriate default.
| if staging.exists(): | ||
| shutil.rmtree(staging) | ||
| staging.mkdir(parents=True) | ||
|
|
||
| for relative_path, content in files.items(): |
There was a problem hiding this comment.
Concurrent installs corrupt skill deployment
Concurrent setup_skills() invocations share the fixed staging path .{name}.opik-staging, so one can remove or modify the other’s data and make staging.replace(target) fail or install a partially interleaved pack, with the resulting OSError reported as a failed installation — should we serialize install/uninstall operations with a per-user filesystem/process lock, or use unique staging directories with a lock around replacement and manifest updates?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/skills/pack.py` around lines 193-197, the
`write_skill` installation logic uses a fixed `.{name}.opik-staging` directory, allowing
concurrent `setup_skills()` calls to delete or interleave each other’s files. Refactor
the install/uninstall workflow to use a per-user filesystem/process lock covering
staging, target replacement, and manifest updates; also use a unique staging directory
per invocation so concurrent operations cannot share or corrupt staging state.
| recorded = manifest.get("skills") | ||
| recorded_names = set(recorded) if isinstance(recorded, list) else set() | ||
| content_hash = manifest.get("contentHash") | ||
| installed_at = manifest.get("installedAt") | ||
|
|
||
| shared_dir = skills_roots.shared_skills_dir() | ||
| statuses: List[SkillStatus] = [] | ||
|
|
||
| for name in sorted( | ||
| recorded_names | _skill_dirs_on_disk(shared_dir, recorded_names) | ||
| ): | ||
| skill_dir = shared_dir / name |
There was a problem hiding this comment.
Manifest traversal enables outside-root deletion
Untrusted recorded_names lets collect_status() resolve shared_dir / name outside ~/.agents/skills, so uninstall_skills() passes that path to _remove_path and recursively deletes directories outside the skills root — should we validate names against the shipped allow-list and enforce resolved-path containment before constructing SkillStatus?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/skills/manifest.py` around lines 82-93, harden
`collect_status()` against path traversal from the manifest’s `skills` list: entries
such as `../../victim` must not become recorded skill names or paths eligible for
uninstall. Filter recorded names to the shipped allow-list, require string entries, and
resolve each candidate path while enforcing that it remains within `shared_dir` before
constructing `SkillStatus`; preserve safe handling of malformed manifests.
There was a problem hiding this comment.
Commit 996d617 addressed this comment by deleting collect_status() and its traversal logic from manifest.py, eliminating the flagged path construction.
`opik mcp status` renders with rich; the install it tells you to run was a wall
of `OPIK:` log lines with absolute paths wrapping over three terminal lines. The
install is the surface a first-time user actually sees, so it was the wrong one
to leave raw.
The flow now narrates through an injected view. That is not indirection for its
own sake: `configurator.mcp.install` is reachable from `opik.configure()`, which
is a library call and must not take over the caller's stdout. So
`LoggingInstallView` stays the default and preserves today's behaviour, and the
CLI passes `RichInstallView`. Tests inject a recording double, which also
decoupled them from exact log strings — eight assertions that matched on log text
now assert on what the flow *decided*.
What a user sees:
Opik MCP server setup
Deployment Opik Cloud · workspace acme-ai
Connection Local server via uvx, credentials in the host config
Will update
Cursor ~/.cursor/mcp.json
Codex via `codex mcp add`
✓ Cursor Added
✗ Codex Could not register 'opik-mcp': the `codex` CLI was not found …
✓ Verified workspace acme-ai · 7 projects visible
Restart Cursor, then ask: "list my Opik projects"
The substantive change behind the formatting is the **plan block, shown before
anything is written**. The original plan for this work called for it and the
first pass shipped only the default-on flip; consent to edit files owned by
another tool is not meaningful if you cannot see which files. That needed
`_resolve_targets` split into `_candidate_targets` (no prompting) and
`_confirm_targets`, so the paths are known before the question is asked. A test
asserts the plan precedes the write rather than trusting the call order.
Smaller things that were each a papercut:
- Spinners on the three slow steps — the hosted probe, `uv tool install`, and
verification — which previously ran in silence for up to 30s.
- `~` instead of `$HOME`, including inside failure messages, where one absolute
path wrapped over three lines and buried the instruction.
- One grid for all result rows, so the host column aligns. A grid per row aligns
each row against itself and nothing else.
- Results say "Added" rather than repeating the path the plan just showed;
failures keep the full detail, because they need it.
- "Restart Cursor and Claude Code" — and only the hosts that actually succeeded.
- Deployment and transport stated up front, so it is clear which Opik is being
connected and whether credentials are being written to disk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| manual_config, | ||
| MCP_DOCS_URL, | ||
| ) | ||
| candidates = _candidate_targets(host_keys) |
There was a problem hiding this comment.
Consent can install the wrong host set
When host_keys is None, _candidate_targets(host_keys) re-detects hosts after _mcp_prompt_named_detected_hosts is captured, so setup_mcp_server(..., assume_confirmed=True) can install hosts added after consent or omit approved hosts that disappeared — should we capture the keys in _should_setup_mcp_server() and pass that snapshot as host_keys?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/mcp/install.py` around line 115, fix
`_candidate_targets` and its caller so an already-approved interactive setup uses the
exact host set that was detected during consent. Capture the detected host keys in
`_should_setup_mcp_server()` and pass them as `host_keys` to `setup_mcp_server`, or
otherwise pass an immutable candidate snapshot, preventing hosts that appear or
disappear later from changing the planned installations.
|
|
||
|
|
||
| def _codex_manual_instructions() -> str: | ||
| """What to tell the user when we cannot drive the ``codex`` CLI. | ||
|
|
||
| Codex stores servers in TOML, which we deliberately do not hand-edit: merging | ||
| into someone else's TOML without a writer risks losing their comments and | ||
| formatting. So when the CLI is unavailable we hand the work back rather than | ||
| guessing. | ||
| """ | ||
| return ( | ||
| f"the `codex` CLI was not found on your PATH, and {_codex_config_path()} is " |
There was a problem hiding this comment.
Opencode status misreports local targets
opik mcp status reads env from opencode registrations even though they store environment, so local entries show no workspace and Reports to: Opik Cloud instead of their recorded OPIK_URL/COMET_URL_OVERRIDE — should we normalize the shape or teach the parser to read environment?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/mcp/targets.py` around lines 210-221, fix the new
opencode target registration so `opik mcp status` correctly reads its `environment`
fields instead of losing the workspace and reporting the default Opik Cloud URL.
Normalize opencode registrations to the existing `env` shape, or update the status
parser to support `environment` (including `OPIK_URL`/`COMET_URL_OVERRIDE`), and add a
regression test covering a local/self-hosted opencode registration.
Two things the numbered menu got wrong. It made the user do the label-to-number
mapping themselves, and it gave no feedback until Enter. And for skills we never
asked at all — one yes/no installed into every detected assistant, which is the
wrong default for guidance the assistant then acts on: wanting it in the editor
you use for Opik work does not mean wanting it in every assistant on the machine.
Which AI assistants should the Opik MCP server be set up for?
◉ Claude Code
❯ ◉ Cursor
◯ Codex
↑↓ move · space select · a all · enter confirm
Hand-rolled on stdlib `termios`/`msvcrt` plus `rich`. The alternative was adding
`prompt_toolkit` (via `questionary` or similar) to the core SDK, which is a large
addition to every Opik install for one CLI nicety. Arrow keys, `j`/`k`, space to
toggle, `a` for all, Enter to confirm, Escape/Ctrl-C to cancel — and cancel
returns `None` rather than `[]`, because "I backed out" and "none of them,
deliberately" are different answers and only one of them should skip silently.
Not every terminal can host this: a pipe, a CI log, a platform with neither
key-reading module. `selector.is_supported()` says so and callers fall back to
the numbered menu rather than failing. A single candidate skips the list too —
arrow keys for one item is worse than a yes/no.
Selection moved onto the view (`choose_hosts`), since which-hosts is a
presentation concern. That surfaced a bug the tests caught: `RichInstallView`
extends the abstract base, so a `super().choose_hosts()` fallback silently
returned `None` instead of the menu. The menu is now a module-level
`numbered_menu()` both views call, rather than something inherited.
Scope: `opik mcp configure` and `opik skills configure` only. `opik configure`
keeps its existing prompts and plain log output for now — it still gets the
logger-backed default view, so nothing about that flow changes.
Also fixed two strings left stale by the native skills rewrite, which claimed the
skill pack needed `npx` and went through a third-party CLI. It does neither.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Python SDK Unit Tests Results (Python 3.13)4 811 tests 4 809 ✅ 2m 3s ⏱️ Results for commit c0c4e64. ♻️ This comment has been updated with latest results. |
Python SDK Unit Tests Results (Python 3.14)4 811 tests 4 809 ✅ 2m 2s ⏱️ Results for commit c0c4e64. ♻️ This comment has been updated with latest results. |
Python SDK Unit Tests Results (Python 3.11)4 811 tests 4 809 ✅ 2m 30s ⏱️ Results for commit c0c4e64. ♻️ This comment has been updated with latest results. |
Python SDK Unit Tests Results (Python 3.12)4 906 tests 4 904 ✅ 2m 29s ⏱️ Results for commit 79410b4. ♻️ This comment has been updated with latest results. |
Python SDK Unit Tests Results (Python 3.10)5 003 tests 5 001 ✅ 2m 28s ⏱️ Results for commit ccfc0b2. ♻️ This comment has been updated with latest results. |
The result column mixed two axes. "Added"/"Updated" said whether the entry was
new or replaced; "Registered" said we drove the host's own CLI (`claude mcp add`,
`codex mcp add`) instead of writing the config file. Reading
✓ Claude Code Registered
✓ Cursor Added
there is no way to tell what the difference is, and the mechanism is already
stated in the plan block one line above ("Claude Code via `claude mcp add`").
Worse, "Registered" hid new-vs-updated for exactly the hosts that go through a
CLI, because `claude mcp add` and `codex mcp add` cannot report it — we remove
first to keep re-runs idempotent, which erases the evidence.
So read before writing. Both CLI paths now check for an existing registration
first, using the readers `opik mcp status` already relies on, and every host
reports one thing:
✓ Claude Code Added → second run: Updated
✓ Cursor Added → Updated
✓ VS Code Copilot Added → Updated
The JSON-file read was extracted out of `read_registered_block` so the installers
can reuse it rather than duplicating the parse.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # The file path works out new-vs-existing itself. | ||
| return _install_via_json_file( |
There was a problem hiding this comment.
Malformed opencode config aborts installation
The no-CLI fallback’s _install_via_json_file passes a non-mapping mcpServers value into merge_server_into_json_file, where servers[server_name] = server_block raises TypeError; because the wrapper catches only ValueError and OSError, installation aborts instead of returning the documented failed InstallResult — should we validate the container and raise a handled configuration error before assignment?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/configurator/mcp/targets.py around lines 145-146, fix the
`_install_claude_code` no-CLI fallback and its shared JSON merge path so a valid config
with a non-mapping `mcpServers` value cannot cause an unhandled `TypeError`. Validate
that the selected server container is a mapping before assigning `servers[server_name]`,
and raise an exception already handled by the JSON-install wrapper (or update the
wrapper appropriately) so installation returns the failed `InstallResult` with redacted
manual instructions.
| detail=( | ||
| f"{'Updated' if was_registered else 'Added'} '{SERVER_NAME}' via " | ||
| f"`claude mcp add` (user scope)" | ||
| ), | ||
| summary="Updated" if was_registered else "Added", | ||
| ) |
There was a problem hiding this comment.
CLI disappearance aborts host installation
subprocess.run raises OSError when either CLI is missing or not executable, so _install_claude_code and _install_codex never receive a CompletedProcess to construct an InstallResult, and the caller can abort without reporting the selected host — should we catch OSError around both subprocess calls and return a failed InstallResult with the executable/error context?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/configurator/mcp/targets.py around lines 182-187, update the
`_install_claude_code` result handling and the corresponding `_install_codex` subprocess
logic to catch `OSError` from both the remove and add commands. Return a failed
`InstallResult` with the target name and executable/error context whenever either
command cannot be executed, while preserving the existing success and nonzero-exit
reporting.
There was a problem hiding this comment.
Commit ccfc0b2 addressed this comment by catching OSError during both Claude and Codex CLI remove/add operations. It returns a failed InstallResult containing the target and executable/error context.
| command = [codex_executable, "mcp", "add"] + server_spec.to_codex_add_args() | ||
|
|
||
| # Let `codex mcp add` print its own output so the user sees the result. | ||
| result = subprocess.run(command) |
There was a problem hiding this comment.
Codex install exposes API key in argv
StdioServerSpec.to_codex_add_args() expands OPIK_API_KEY into --env OPIK_API_KEY=<value>, which subprocess.run passes to codex mcp add, so authorized process inspection and audit tooling can read the key from the command line despite InstallResult.detail redaction. Could we use Codex’s env_vars in config.toml, or another registration path, to keep the value out of argv?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/mcp/targets.py` around lines 261-264, fix the Codex
registration logic in `_install_codex` so `OPIK_API_KEY` is never embedded in the `codex
mcp add` argv. Use Codex’s supported `env_vars`/`config.toml` mechanism, or another
secure supported path that forwards the already-present environment variable without
exposing its value to process inspection; update the argument-building logic and tests
while preserving idempotent registration and result reporting.
| url = os.environ.get("OPIK_URL_OVERRIDE", "").strip() | ||
| if url: | ||
| if url_helpers.get_base_url(url).rstrip("/").endswith("comet.com"): | ||
| return interactive_helpers.DeploymentType.CLOUD | ||
| if "/opik/api" in url: | ||
| # The Comet platform's path shape, on someone else's host. | ||
| return interactive_helpers.DeploymentType.SELF_HOSTED | ||
| return interactive_helpers.DeploymentType.LOCAL | ||
|
|
||
| if os.environ.get("OPIK_API_KEY", "").strip(): | ||
| # A key with no URL only makes sense for Opik Cloud. | ||
| return interactive_helpers.DeploymentType.CLOUD |
There was a problem hiding this comment.
_deployment_type() reads only OPIK_URL_OVERRIDE/OPIK_API_KEY, so headless opik configure -y ignores valid ~/.opik.config values and fails before creating a configurator — should we resolve them through the SDK’s OpikConfig state while preserving explicit env/session precedence? The cloud check matches raw URL suffixes, so evilcomet.com is misclassified as Opik Cloud — should we compare the parsed hostname with comet.com or a .comet.com suffix?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/configure.py` around lines 110-121, update
`_deployment_type()` so it does not rely solely on `os.environ`; it should resolve the
URL/API key through the same `OpikConfig` state used by the SDK (session cache,
environment, saved config, defaults), preserving explicit environment/session precedence
before falling back. Additionally, fix the cloud-detection check so it parses the URL's
hostname and treats it as Opik Cloud only when the hostname equals `comet.com` or ends
with `.comet.com`, rather than doing a raw `endswith("comet.com")` string match, so
arbitrary domains ending in that substring aren't misclassified as cloud; otherwise
preserve the self-hosted/local detection behavior and raise the missing-configuration
error when nothing resolves.
There was a problem hiding this comment.
Commit 79410b4 addressed this comment by parsing the URL hostname and requiring comet.com or a .comet.com suffix for cloud detection. It did not change _deployment_type() to resolve values through OpikConfig, so the saved-config concern remains.
| deployment_type_choice = _deployment_type() | ||
|
|
||
| if deployment_type_choice == interactive_helpers.DeploymentType.CLOUD: | ||
| configurator = opik_configure.OpikConfigurator( |
There was a problem hiding this comment.
Custom deployment URL is discarded
When OPIK_URL_OVERRIDE is set, this boundary passes only the enum to OpikConfigurator, so it falls back to a default base_url and api_url/_update_config() target and persist a different deployment for validation, assistant setup, and url_override. Should we pass the resolved override as url=..., including for the custom Cloud-shaped case?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/cli/configure.py around lines 157-160, update
`run_interactive_configure` and `_deployment_type()` so an `OPIK_URL_OVERRIDE` value is
not discarded after deployment classification. Carry the resolved override URL alongside
the deployment enum and pass it as `url` to the corresponding `OpikConfigurator` for
self-hosted, local, and custom Cloud-shaped deployments, while preserving the standard
Cloud URL when no override exists. Ensure validation, assistant setup, and persisted
configuration all use the same resolved endpoint.
| if not yes and not interactive_helpers.is_interactive(): | ||
| raise click.ClickException( | ||
| "`opik configure` asks a few questions and there is no terminal to " | ||
| "answer them in. Add `-y` to accept the defaults:\n\n" | ||
| " opik configure -y --install-mcp\n" | ||
| ) |
There was a problem hiding this comment.
Misleading headless configuration guidance
The non-interactive error recommends opik configure -y --install-mcp as accepting the remaining defaults, but _deployment_type() still fails when none of OPIK_URL_OVERRIDE, OPIK_API_KEY, or --use_local selects a deployment, so users hit the same error again — should we include a deployment selector in the example or clarify that -y only answers follow-up prompts?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/configure.py` around lines 251-256, update the non-interactive
validation in `configure` so its error message does not imply that `-y` alone is
sufficient. Explain that `-y` only answers follow-up prompts after the deployment type
is determined, and provide actionable examples including `--use_local -y` or the
required `OPIK_URL_OVERRIDE`/`OPIK_API_KEY` environment variables.
There was a problem hiding this comment.
Commit bf43be3 addressed this comment by removing the misleading -y-only error guidance and documenting that non-interactive runs assume defaults, while deployment still comes from environment variables or --use_local.
| def test_configure_no_terminal_with_yes__proceeds(): | ||
| runner = CliRunner() | ||
| with ( | ||
| mock.patch.object( | ||
| configure_cli.interactive_helpers, "is_interactive", return_value=False | ||
| ), | ||
| mock.patch.object(configure_cli, "run_interactive_configure") as spy, | ||
| ): | ||
| result = runner.invoke(cli, ["configure", "-y"]) | ||
|
|
||
| assert result.exit_code == 0 | ||
| spy.assert_called_once() |
There was a problem hiding this comment.
-y propagation is untested
The -y test only checks that run_interactive_configure was called, so a regression passing automatic_approvals=False would still pass and later prompt or abort headless runs — should we assert automatic_approvals=True along with the expected use_local and other defaults?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/cli/test_configure_cli.py` around lines 280-291, strengthen
`test_configure_no_terminal_with_yes__proceeds` so it verifies the arguments passed to
`run_interactive_configure`, not just that it was called. Assert that
`automatic_approvals` is `True` and that `use_local` and the other expected defaults are
propagated correctly, ensuring `-y` prevents prompting in headless execution.
There was a problem hiding this comment.
Commit bf43be3 addressed this comment by replacing the old -y test and explicitly asserting automatic_approvals=True for headless configuration. The test now covers automatic approval propagation, though it exercises the no-flag default path instead of -y specifically.
| if ( | ||
| not interactive_helpers.is_interactive() | ||
| and not host_keys | ||
| and not assume_confirmed | ||
| ): |
There was a problem hiding this comment.
Unpinned MCP install runs mutable package code
The new authorization gate lets non-interactive callers with host_keys or assume_confirmed reach _prefetch_opik_mcp(), which runs uv tool install opik-mcp without an exact version, trusted index, or hash verification, so an explicit coding-agent request executes mutable package installation/build code with the invoking user's privileges — should we restrict this path or require all three installation safeguards?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/mcp/install.py` around lines 79-83, review the
`setup_mcp_server` authorization gate because non-interactive `host_keys` or
`assume_confirmed` requests can reach `_prefetch_opik_mcp()` and install mutable package
code with user privileges. Refactor the unattended local-stdio path to fail closed
unless installation is explicitly and safely authorized, and make `_prefetch_opik_mcp`
install an exact, verified package version using a trusted index and hash verification
rather than an unpinned package. Add or update tests covering non-interactive requests
and the secure installation requirements.
The previous commit made the flow work but assumed the agent already knew the
invocation. Walking it as one showed the real failure was not a bad error, it was
a silent success:
$ opik configure
Error: ... Add `-y` to accept the defaults
$ opik configure -y
OPIK: Configuration completed successfully. <- and no MCP, silently
`-y` is exactly what the first error told it to add, so that is the path an agent
takes — and it configured Opik, said it had succeeded, and wrote nothing to the AI
client. An agent asked for both would report done having delivered half. A wrong
answer that looks right is worse than the abort it replaced.
So the skip now says so, and names what to add:
Skipped AI client setup: nothing named it, so nothing was written to your AI
client's config.
To include it: opik configure -y --install-mcp --install-skills
Only without a terminal: someone who typed `-y` chose this, an agent that was told
to add `-y` did not.
Both `--help` texts now carry the non-interactive recipe, since reading help is
what an agent does before guessing. The whole walk is three steps, each output
naming the next:
opik configure -> add -y
opik configure -y -> add --install-mcp
opik configure -y --install-mcp -> done
Tests cover the announcement firing without a terminal, staying quiet with one,
and `--help` carrying the flags.
4812 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
You asked why it took so many steps. It didn't need to. `-y` existed to say "yes,
the defaults" — and with no terminal there is nobody to ask, so requiring it was a
step that existed only to be discovered. Worse, the error teaching it was the step
an agent was most likely to stop at.
Without a terminal the defaults are now assumed, which collapses the walk to one
command:
before: opik configure -> error, add -y
opik configure -y -> succeeded, no MCP
opik configure -y --install-mcp -> done
now: opik configure --install-mcp --install-skills -> done
Broader than asked, and deliberately: implying `-y` only from the assistant flags
would have left `opik configure` alone still erroring, which is the same
discovery step one command further along. The questions being defaulted are
"use the local instance we found" and "keep the project name we derived", and
neither has a second sensible answer when nobody is there to give one.
A terminal changes nothing: `automatic_approvals` is still just `-y` there, so a
person keeps every prompt they had. Asserted both directions rather than only the
new one.
What is *not* defaulted is the part that writes outside Opik: `opik configure`
with no flags still touches no AI client config, and still says so with the
remedy — now without the `-y` that is no longer needed. `opik mcp configure` with
no client named still refuses, because there the missing piece is *which* client,
which has no default.
4812 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answering "what does -y do with mcp and skills now": nothing, and that was quietly
true in a terminal as well.
opik configure -y (terminal) -> Opik configured, no MCP, no message
`-y` does not install the MCP server or the skill pack and never has — it answers
Opik's own questions, and writing into another tool's config needs naming. But
`-y` reads as yes-to-everything, so someone who types it chose "stop asking me",
not "skip my editor", and got no hint that half the thing they expected did not
happen.
The skip announcement was gated to no-terminal runs on the reasoning that a person
who typed `-y` chose this. They didn't — they chose not to be asked. Same
surprise, same one line, now shown in both modes.
Also worth recording from checking this: with no terminal, `-y` is now a complete
no-op, because the defaults are already assumed. Every `-y` row in the matrix is
identical to the row without it. It stays supported so existing scripts keep
working, but it no longer buys anything there.
4812 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…figure-onboarding # Conflicts: # sdks/python/tests/unit/message_processing/test_payload_truncation.py
| def test_no_flags__asks_and_proceeds_on_yes(self): | ||
| confirm, setup_calls = self._run(answer=True) | ||
|
|
||
| assert confirm.called | ||
| assert len(setup_calls) == 1 |
There was a problem hiding this comment.
Assistant setup flags are unverified
These tests assert only that assistants.setup was called once, so regressions in skills_flag, host_keys, or assume_confirmed would still pass — should we assert the captured kwargs for the positive, declined, and explicit-flag cases?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/cli/test_configure_cli.py` around lines 108-112, strengthen
`TestAssistantConfirmation` so the assistant installer behavior is verified, not just
its call count. Assert the captured `assistants.setup` kwargs for the accepted,
declined, and explicit `install_mcp` cases, including the expected `skills_flag`,
`host_keys`, and `assume_confirmed` values; keep the declined case asserting no call
occurs.
| Defaults to no, matching `opik configure -y`'s refusal to reach into | ||
| another tool's config. | ||
| """ | ||
| detected = mcp_installer.detected_host_names() |
There was a problem hiding this comment.
Assistant probe failure aborts core configuration
mcp_installer.detected_host_names() lets probe failures escape from _confirm_assistant_step(), so a filesystem, parsing, or client-probe error aborts opik configure instead of taking the best-effort skip path — should we catch the expected exceptions, log them with context and exc_info=True, and return an empty detection result?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/configure.py` around lines 96-99, update
`_confirm_assistant_step()` so failures from the optional
`mcp_installer.detected_host_names()` probe do not abort `opik configure`. Catch the
expected filesystem, parsing, and client-probe exceptions, log a contextual
warning/error with `exc_info=True`, and treat the failure as an empty host-detection
result so the existing best-effort skip path runs; avoid catching process-control
exceptions or unrelated programming errors.
There was a problem hiding this comment.
Commit ccfc0b2 addressed this comment by converting client CLI probe OSError and timeout failures into unavailable results, which detection can skip. Filesystem/parsing failures and contextual exc_info=True logging remain unaddressed.
| "--ai-client", | ||
| "hosts", | ||
| multiple=True, | ||
| type=click.Choice(mcp_targets.HOST_KEYS + [HOST_ALL], case_sensitive=False), | ||
| help="AI client to register the server with. Repeatable, or pass `all` for " | ||
| "every one detected on this machine. Naming a client is what lets this run " | ||
| "without a terminal — a coding agent or a script should pass it.", |
There was a problem hiding this comment.
Headless host setup can hang indefinitely
The unattended --ai-client path calls assistants.setup() synchronously, reaching the remove/add subprocesses in configurator/mcp/targets.py without timeout or stdin/input, so unbounded communicate() can leave opik mcp configure --ai-client ... hanging on an authentication prompt or stalled process — should we use non-interactive stdin with a finite timeout and route timeout failures through InstallResult?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/mcp.py` around lines 109-115, address the unattended
`--ai-client` path and its Claude Code/Codex setup calls in
`configurator/mcp/targets.py`. Refactor the `remove`/`add` subprocess invocations to use
non-interactive stdin and a finite timeout so authentication prompts or stalled
processes cannot block indefinitely. Catch timeout failures and convert them into the
existing `InstallResult` error path with an actionable message.
There was a problem hiding this comment.
Commit ccfc0b2 addressed this comment by routing Claude Code and Codex subprocesses through a helper that uses stdin=DEVNULL and a 60-second timeout. Timeout and startup failures are converted into failed InstallResult responses with actionable messages.
| def configure( | ||
| local_server: bool, hosts: Tuple[str, ...], skills_flag: Optional[bool] | ||
| ) -> None: |
There was a problem hiding this comment.
Headless hosted setup rejects valid MCP endpoints
The explicit-client setup verification probes only GET /v1/mcp and treats 405 Method Not Allowed as failure, so valid Streamable HTTP MCP endpoints without optional GET streaming get reported as failed after writing config — should we probe with the required protocol POST/auth semantics, or at least accept 405 as reachability while still handling 401/403 as auth failures?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/mcp.py` around lines 124-126, update the `configure` flow and
its downstream hosted MCP endpoint verification so the new explicit-client unattended
path does not falsely report valid servers as failed. Replace the GET-only probe with
the required authenticated MCP POST/protocol check, or at minimum treat HTTP 405 Method
Not Allowed as endpoint reachability while preserving appropriate handling for
authentication failures such as 401/403.
| Without a terminal — a coding agent, a script — name the client, which is what | ||
| makes the request explicit: | ||
|
|
||
| opik mcp configure --ai-client cursor --skills |
There was a problem hiding this comment.
Unverified instructions reach assistants
The skills installation path accepts an archive downloaded from mutable refs/heads/main without authenticating its contents, then writes the resulting Markdown into assistant skill roots and records the hash only afterward; sdks/python/src/opik/cli/mcp.py:124-126 and sdks/python/src/opik/cli/mcp.py:135-135 expose this path. A compromised revision or response can therefore replace skills for every selected host, including shared ~/.agents/skills content and Claude links, without an integrity warning. Should sdks/python/src/opik/configurator/skills/pack.py and sdks/python/src/opik/configurator/skills/install.py pin the pack to a reviewed immutable revision and verify a separately trusted digest or signature before replacement?
Supporting evidence from every grouped finding:
-
sdks/python/src/opik/cli/mcp.py:124-126: The
--skillspath downloadshttps://codeload.github.com/comet-ml/opik-skills/tar.gz/refs/heads/main, whilecontent_hashis derived from those bytes and recorded only afterwrite_skill()replaces~/.agents/skills, so a compromised revision becomes active assistant instructions without authenticating the response. Could we pin the default to a reviewed immutable revision and verify a separately trusted signature or digest before replacing the skill root? -
sdks/python/src/opik/cli/mcp.py:135-135:
--skillsflows throughconfigure()→assistants.setup()→skills_installer.setup_skills(), wherepack.download()follows redirects and accepts any HTTP 200 archive from mutablerefs/heads/main, so_read_archive()sends untrusted Markdown towrite_skill()and the post-acceptancecontent_hashdoes not authenticate it. Should we updatesdks/python/src/opik/configurator/skills/pack.pyandinstall.pyto pin the pack to a reviewed immutable commit and verify an expected digest or signature beforewrite_skill()?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/mcp.py` around lines 135-135, review the `configure()`
`--skills` path and harden the downstream skills installation flow in
`sdks/python/src/opik/configurator/skills/pack.py` and `install.py`. Pin the downloaded
pack to a reviewed immutable commit rather than `refs/heads/main`, and verify an
independently supplied expected digest or signature before any archive content reaches
`write_skill()`; do not treat a hash computed from the accepted download as
authentication. Add or update tests covering mutable revisions, redirects, invalid
integrity metadata, and rejection before installation.
CodeQL flagged this on the deployment inference added two commits ago —
`py/incomplete-url-substring-sanitization`, high severity, and correct:
url_helpers.get_base_url(url).rstrip("/").endswith("comet.com")
`evil-comet.com` ends with `comet.com`, so a self-hosted deployment on a
lookalike host was classified as Opik Cloud and configured against the wrong
place. Parsing the hostname and requiring the dot to be a real label boundary is
the check that was meant:
host == "comet.com" or host.endswith(".comet.com")
Verified against the cases the old form got wrong — `evil-comet.com`,
`comet.com.evil.net`, and `comet.com` appearing only in a path or query all now
resolve to self-hosted or local, while `comet.com`, `www.comet.com` and
`staging.comet.com` still resolve to cloud. Case is normalised too, since
hostnames are case-insensitive and the old form was not.
Worth noting what this was and was not: the misclassification pointed *at* real
Opik Cloud rather than at the attacker's host, so it was a correctness bug
before it was an exposure — but a hostname check written as a substring test is
wrong either way, and it is the kind that grows teeth when someone later reuses
it to decide what to trust.
Also in this commit: the merge of origin/main. Its only conflict was
`test_payload_truncation.py`, where main had independently made the same
`capture_log` fix and additionally asserted the field name — theirs kept, since
it is strictly stronger than mine.
4904 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…figure-onboarding
…it did Adds a `configuration` analytics component covering `opik configure` and `opik mcp configure`, which were previously invisible: every existing MCP dashboard tile starts at the server already being installed. Each command reports a pair of events — entry with what was asked for, exit with what was actually written (`clients`, `skills`). The gap between the two is the drop-off, which is the number the flow most owes us. Both are reported from the click command rather than the installers underneath. Analytics drops an event whose immediate caller is a different `opik` module, so a configurator called from `opik.cli` reads as Opik calling itself; and a reporter nested inside an already-reporting stack is dropped too, which rules out a nested funnel. Sequential siblings from the same frame both survive, which is what the entry/exit pair relies on. The assistant step therefore returns its outcome instead of reporting it, and `run_interactive_configure` recovers it through a recorder — the configurator takes the step as a callback and discards its return value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # The tri-states as passed, so "asked and said yes" is separable from | ||
| # "never asked" — the flag is also how an agent drives this. | ||
| install_mcp=str(install_mcp), | ||
| install_skills=str(install_skills), | ||
| ) |
There was a problem hiding this comment.
Analytics loses nullable flag semantics
str(install_mcp) and str(install_skills) turn None into "None" instead of null, so analytics consumers cannot distinguish unset flags from booleans — should we pass the values directly or use a documented normalization, including str(skills_flag) in sdks/python/src/opik/cli/mcp.py?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/cli/configure.py around lines 316-320, update the `configure`
analytics event so `install_mcp` and `install_skills` are passed as their original
nullable boolean values rather than converted with `str()`. Preserve `None` as a true
null so analytics consumers can distinguish an unset flag from explicit `True` or
`False`; apply the same fix to the `str(skills_flag)` payload in
`sdks/python/src/opik/cli/mcp.py`.
There was a problem hiding this comment.
Commit 37f763b addressed this comment by removing the skills_flag analytics path in mcp.py while routing skill consent through the new verdict. However, configure.py still converts both nullable flags with str(), so the fix is only partial.
| if not configured_hosts: | ||
| # Nothing was registered, so there is no assistant to add a pack to and | ||
| # the installer has already explained why. | ||
| return NOTHING_DONE | ||
|
|
||
| view = install_view.RichInstallView() | ||
| components = ["MCP server"] | ||
| skills_installed = False | ||
|
|
||
| if _wants_skill_pack(skills_flag, view): | ||
| with view.step("Fetching the Opik skill pack"): | ||
| result = skills_installer.setup_skills(configured_hosts) |
There was a problem hiding this comment.
Failed MCP verification reported as success
assistants.setup() treats any non-empty configured_hosts result as verified, so it installs skills and renders the success view for unverified setups — should we require explicit verification before completing setup?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/assistants.py` around lines 76-87, fix `assistants.setup()` so
it does not treat a non-empty `configured_hosts` result as proof that setup succeeded.
Verify each client configuration result (including partial or failed setups) before
installing skills or calling `view.done`, and report only the clients and skills that
were actually configured successfully. Preserve the existing `Outcome` contract while
ensuring failure paths do not produce a misleading success message.
`skills` carried a tri-state string on the entry event and a bool on the result event. One property key holding two types breaks breakdowns and contradicts the project's own analytics convention, so the two are named apart: entry reports `skills_requested`, the result reports `skills_installed`. `clients` becomes `clients_written` for the same reason it is worth a longer name — the entry event already has `client_count`, which counts clients *named* on the command line rather than clients actually written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # The tri-state as passed, so "asked for it" stays separable from "never | ||
| # said". Named apart from the result event's boolean: one property key | ||
| # must not carry a string on one event and a bool on another. | ||
| skills_requested=str(skills_flag), |
There was a problem hiding this comment.
Bare MCP setup now aborts after writing
The omitted skills_flag remains None and triggers a second click.confirm() after MCP registration, so bare opik mcp configure invocations with only the existing host-selection answers hit EOF, raise click.Abort, and exit 1 after writing the MCP file — should we keep the bare command MCP-only or document and migrate callers to explicit --no-skills?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/mcp.py` around lines 149-152, update the `configure` flow so
an omitted `skills_flag` (`None`) preserves the legacy bare `opik mcp configure`
behavior and does not trigger the assistant skills setup or a second `click.confirm()`.
Keep explicit `--skills` and `--no-skills` behavior distinct, while retaining the
tri-state value for analytics if needed; ensure bare invocations complete as MCP-only
after writing the configuration.
petrotiurin
left a comment
petrotiurin
left a comment
There was a problem hiding this comment.
Reviewed the SDK side of this. The layering is the right call — configurator/ holding the logic with InstallView as the seam, and rendering kept in cli/ — and the comments carry real reasoning rather than restating the code. Five findings inline.
One is blocking: opik configure --no-install-mcp still registers the MCP server and installs the skill pack with no prompt (cli/configure.py:43-47, reproduced by running the path). The rest are a stale --host flag in a user-facing hint, a rich Live region fighting streamed subprocess output, the API key reaching the process argv, and one low-severity note on the skill-pack download client.
`opik configure --no-install-mcp` registered the MCP server anyway and installed the skill pack unprompted. The line at fault was real, but it was a symptom: the consent policy existed twice. The tested decision table governed only `opik.configure()`, while `opik configure` ran its own ad-hoc ladder that never called it — so the two could and did disagree. Five decision sites become one `configurator.consent.resolve`, shared by both surfaces. Its six rules are the whole policy, and it returns why alongside what, so callers explain themselves instead of guessing. `-y` and "no terminal" stop being conflated. The command still hands `-y` down whenever there is no tty, but the resolver takes them separately and reports NO_TERMINAL first, so an unattended run is no longer told that a flag it never passed is why its editor was skipped. The two halves of the step are now independent. `setup()` registered the server as its first act whatever it was asked for, which is why "skills but not the server" had to be faked by a call that forced `skills_flag=True` — overriding `--no-install-skills` on the way. The pack's targets fall back to detected clients, so either half can run alone. Deleted rather than added to: the CLI ladder and its unreachable branch, `_confirm_assistant_step`, `_wants_skill_pack`, both `*_decision` functions, a dead `_mcp_prompt` test seam no test called, three unused loggers, and a prompt argument threaded into an asker that ignored it. Behaviour change worth noting: an explicit skills request with no client detected used to skip in silence, and now reaches `setup_skills`, which names the locations it knows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Where the pack goes: the clients we just registered, or — when the server | ||
| # step was declined or skipped — whatever is on this machine. An empty list is | ||
| # passed through rather than special-cased, because `setup_skills` already | ||
| # names the clients it could not place the pack in, and it is the part that | ||
| # knows which locations are supported. | ||
| skills_targets = configured_hosts or skills_installer.detected_host_keys() |
There was a problem hiding this comment.
Failed explicit host loses skill targeting
setup_mcp_server() returns [] when every explicit host_keys write fails, so the truthiness fallback to skills_installer.detected_host_keys() can target unrelated clients or omit the requested one. Should we preserve explicit targets separately and return an outcome distinguishing no candidates, user decline, all failures, and success?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/cli/assistants.py` around lines 83-88, refactor the skill-target
selection logic in the MCP/skill-pack installation flow so it does not use the
successful `configured_hosts` list as a truthiness-based proxy for the requested
`host_keys`. Preserve explicit targets even when all MCP writes fail, avoid falling back
to unrelated detected clients in that case, and update the setup result or surrounding
state to distinguish no candidates, user decline, all-write failure, and successful
registration before choosing skill targets and reporting the outcome.
…ailure Six findings from review, each a case of a failure path doing more harm than the operation was worth. Writes to a client's config are atomic. `merge_server_into_json_file` truncated before writing, and `~/.claude.json` is the whole of Claude Code's user state — a full disk or a signal mid-write left the user's editor unparseable, while the caller caught OSError and printed a tidy message. It now writes a sibling temp file and renames. A file we create is 0600 because it holds an API key; one that already existed keeps the mode its owner chose. The client CLIs cannot raise out of configure. `shutil.which` only checks the executable bit, and `claude`/`codex` are Node shims, so node moving out from under them passed `which` and then raised FileNotFoundError at exec — an unhandled traceback. All four write calls now go through one guarded runner that also closes stdin and sets a timeout, since they inherited the terminal and a CLI that decided to prompt would wait forever. The errno text names node rather than the shim, so the message explains that itself. The skill pack's archive is capped while streaming. The limit was checked after `response.content` had already materialised the body, so it could only report the memory it existed to prevent. `write_skill` refuses a name that is not a direct child of the destination. It goes on to `rmtree` what it resolves, and the name comes from the downloaded archive: an empty name resolves to the root itself and `..` escapes it. Verified by reverting the guard — the destination really is damaged without it. The pack download uses a plain httpx client, not Opik's factory, whose hooks exist to decorate calls to the Opik API and would otherwise be free to add headers to a GitHub request. The uv prefetch runs `uv tool run opik-mcp --help` instead of `uv tool install opik-mcp`, which was building a persistent tool environment and putting a shim on the user's PATH — an install nothing announced. Its output is captured, since the caller wraps it in a rich status spinner and the two were overwriting each other, and it has a timeout. Also: the printed recovery hint said `--host`, which has been `--ai-client` since 17eb5a0, so following it failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Thirteen `# ANALYTICS:` comments named events to emit from inside the MCP and skills installers. They cannot be emitted there: reporting drops any event whose immediate caller is a different `opik` module, so an installer called from `opik.cli` reads as Opik calling itself, and a reporter nested inside an already-reporting stack is dropped too. The flow's events are reported from the click commands instead. That left the markers describing work that will not happen where they sit, which is a placeholder TODO wearing a comment's clothes. What they asked for and the events do not yet carry: the reason a run stopped early — declined, no client detected, uv missing, ambiguous workspace — and per-client success. The result events say how many clients were written and whether the pack landed, so a zero is visible but unexplained. Threading a reason up to the command that can report it is a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects in the atomic-write change, two of them regressions it introduced. A symlinked config was replaced rather than followed. Dotfile managers (chezmoi, stow, yadm) routinely symlink editor configs into a tracked repo, and `os.replace` on the link swapped it for a regular file — so the tracked file never received the change and the link was gone. Worse than it sounds: the write landed nowhere the user would look, and their next dotfiles sync would clobber it. The `write_text` this replaced wrote through the link, so following it restores the previous behaviour. `os.chmod` was handed the `mkstemp` descriptor. That only works where `os.chmod in os.supports_fd`, which is false on Windows — which the SDK supports, and where this would have raised instead of preserving the mode. Applied by path now, and after the write rather than before, so the file holding the key is never wider than it ends up. The uv prefetch warning sliced with `[-1:]`, handing `LOGGER.warning` a one-element list, so a failure rendered as `['network error']`. Now a plain string, with an explicit no-output fallback and a length cap, since the text comes from another tool. Each fix verified by reverting it and watching the new tests fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…step Two things a user cannot infer from the current closing block. The suggested prompt was "list my Opik projects", which an agent can answer without ever touching the MCP — the SDK is right there and the repo documents it. Naming the server makes the first thing they try actually exercise what was just installed. The hosted server also needs a sign-in that we never mention. How it is triggered is the host's choice: some open the browser on first use, others leave the server unauthorized until asked. An unauthorized server contributes no tools at all rather than an error, so a user who was not told to look never finds out why it went quiet. The tip goes through the existing note() hook rather than a done() parameter: done() is also called from cli.assistants with announce_next_steps=False, and that path has no access to the transport without changing what setup_mcp_server returns. Gated on RemoteServerSpec — the stdio server takes its credentials at startup and has nothing to sign in to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| for target in candidates | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Untrusted config modes expose API keys
The explicit-client setup writes OPIK_API_KEY to existing Cursor, VS Code, or Claude configs without checking permissions, while _write_atomically preserves group/world-readable modes, so the credential remains exposed — should we reject or tighten insecure modes before the write?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/configurator/mcp/install.py around line 158, update the
`setup_mcp_server` explicit-client installation path so it validates the permissions of
each existing Cursor, VS Code, or Claude configuration before installing a
`StdioServerSpec` containing `OPIK_API_KEY`. Reject the affected target with a clear
diagnostic, or securely tighten its mode before writing, and ensure `_write_atomically`
does not preserve group/world-readable permissions for credential-bearing
configurations.
…prompt The hint was printed right after the connection check, which put it between the user and the thing they are meant to try. It belongs last: it is the only part of the block they may still have to act on. Moving it there is not just a reorder. The CLI closes the run from cli.assistants, after the skill pack, by which point setup_mcp_server has returned and the server spec is gone — so the fact has to survive the gap. install still decides it and hands it to the view in plan(), which always runs first; the view carries it to done(). Deriving it in cli.assistants from force_local_server would have been wrong for self-hosted deployments that fall back to stdio without the flag, and changing what setup_mcp_server returns would have rippled through four call sites for a copy change. That needed cli.assistants to stop building two RichInstallView instances for one step, or the flag would be lost between them. The suggested prompt is now green — it is the one line here meant to be copied. Its test anchors on the prompt string, because the ✓ above renders 1;32 and a bare colour search would pass even if the prompt lost its styling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| def test_setup_mcp_server__hosted_detected__flags_the_sign_in_for_the_closing_block( | ||
| monkeypatch, | ||
| ): | ||
| """The CLI closes the run itself, so the fact has to survive on the view.""" | ||
| monkeypatch.setattr( | ||
| install.mcp_detection, | ||
| "detect_hosted_mcp_server", | ||
| lambda **kwargs: "https://dev.comet.com/opik/api/v1/mcp", | ||
| ) | ||
| install_spy = mock.Mock(return_value=targets.InstallResult("Cursor", True, "Added")) | ||
| monkeypatch.setattr(targets, "HOST_TARGETS", [_target("cursor", True, install_spy)]) | ||
| monkeypatch.setattr("builtins.input", lambda message: "y") |
There was a problem hiding this comment.
Hosted sign-in guidance can regress unnoticed
This test checks only the view’s private _needs_sign_in value, so a regression between setup_mcp_server() and the CLI’s final done() rendering would still pass — should we exercise the returned view’s closing behavior and assert the Signing in hint for hosted setup and its absence locally?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/tests/unit/configurator/mcp/test_install.py around lines 430-446, update
`test_setup_mcp_server__hosted_detected__flags_the_sign_in_for_the_closing_block` so it
exercises the returned or captured view through the CLI closing/`done()` behavior
instead of asserting only the private `_needs_sign_in` field. Assert that hosted setup
emits the user-visible “Signing in” guidance, and update the adjacent local-server
test around lines 449-459 to execute the same closing path and verify that this guidance
is absent.
|
|
||
| install.setup_mcp_server(**args) | ||
|
|
||
| assert args["view"]._needs_sign_in is True |
There was a problem hiding this comment.
Tests bypass sign-in behavior
These tests assert the private field args["view"]._needs_sign_in, so they can pass even when plan() or the view’s rendering/logging path no longer produces the user-visible sign-in behavior — should we assert captured log/rendered text from the relevant view and update the analogous assertion at line 459?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/configurator/mcp/test_install.py` around lines 430-459, update
`test_setup_mcp_server__hosted_detected__flags_the_sign_in_for_the_closing_block` and
`test_setup_mcp_server__local_server__does_not_flag_a_sign_in` to stop asserting the
private `args["view"]._needs_sign_in` field. Capture the relevant public view output or
log/rendered text and assert that hosted servers include the sign-in prompt while local
servers do not, so the tests verify observable behavior rather than internal plumbing.
| # Only the hosted server has a sign-in step, and how it gets triggered is | ||
| # the host's choice, not ours: some open the browser on first use, others | ||
| # leave the server unauthorized until asked. An unauthorized server | ||
| # contributes no tools at all rather than an error, so a user who is not | ||
| # told to look never finds out why it went quiet. The view carries this | ||
| # to its closing block, which `cli.assistants` prints after the skill | ||
| # pack — by then the spec is out of scope. | ||
| needs_sign_in=isinstance(server_spec, mcp_spec.RemoteServerSpec), |
There was a problem hiding this comment.
needs_sign_in is set from server_spec before any selected_targets installation succeeds, so cancelled, no-target, or failed HostTarget.install() runs leave it true and RichInstallView.done() prompts users to sign in to an unregistered server — should we set it only after at least one hosted target succeeds or pass MCP installation success explicitly to the renderer?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/src/opik/configurator/mcp/install.py` around lines 150-157, update
`setup_mcp_server` so `needs_sign_in` is not set solely from `server_spec` before target
selection and installation completes. Set it only when the server is hosted and at least
one `HostTarget.install()` call succeeds (or pass MCP installation success explicitly to
the completion renderer instead), and explicitly clear or avoid propagating it on
cancellation, no-target, and all-failed installation paths so `RichInstallView.done()`
never asks users to sign in to an unconfigured/unregistered server.
4 participants
Details
Roughly half of Opik MCP installs never issue a single tool call, and the install step is the onboarding. Today it asks permission it should assume, verifies nothing, reaches three of the seven AI hosts users actually run, and cannot be scripted at all — so a coding agent, a Dockerfile, or CI has no path to a configured MCP server. This reworks
opik mcp configure, adds a matchingopik skillscommand, and wires both intoopik configure.MCP server
--install-mcpwas dead code in exactly the environments people automate. Theis_interactive()guard in_should_setup_mcp_serverran before the explicit flag, soopik configure -y --install-mcpsilently did nothing and exited 0. An explicit flag is the user asking, so it is now honoured without a TTY.-yon its own still skips MCP — a blanket yes-to-everything should not edit another tool's config files.--hostflag onopik mcp configure(repeatable, orall). The command previously hard-failed without a terminal, but a terminal was only ever needed to ask which host to use. Naming one makes "set Opik up for me" a single step an agent can run. An explicit host installs whether or not it is detected, so a fresh CI image can be configured before the editor is.defaultand advertises all its tools — a completely broken setup was indistinguishable from a working one until the agent hit a 401 mid-conversation. It now makes a real call with exactly the values it wrote and reportsconnected to workspace acme-ai, 7 project(s) visible, or fails with the reason.codex mcp get --json, normalised soopik mcp statusneeds no per-host special casing. opencode gets its own block shape (local/remote, argv as one list,environmentrather thanenv).default, which resolves to the account default — so reads come back from the wrong place instead of failing. Returning confidently wrong data is the one failure here that doesn't look like a failure. A failed workspace lookup is explicitly not treated as evidence of a single workspace.uvinstall command for the platform whenuvxis missing.Skill pack
The MCP server gives an assistant tools; the skill pack gives it the knowledge of how to use Opik. The telemetry says the second half is the gap — 43 installs have loaded the tool list 14,120 times between them without ever calling a tool, and
schemais the most widely-called tool of all. Agents are connected and groping.New
opik skillsgroup (configure/update/remove/status), plusopik configure --install-skillsandopik mcp configure --skills.The pack is offered as a recommended follow-up to the MCP step, not as a choice up front. Asking "MCP or skills or both?" before anything happens makes the user decide between two things they cannot yet see; asking after the server is registered means the results are on screen, and the pack only applies to the hosts the server actually reached. The prompt does not re-list those hosts — the results table directly above it already does, and repeating three names buries the question.
No third-party installer is involved.
comet-ml/opik-skillsdocumentsnpx skills add, but that turns out to be unnecessary — skills areSKILL.mddirectories and the assistants have converged on a shared user-level location. Verified per host rather than taken on trust:$HOME/.agents/skillscodex-rs/ext/skills/src/host_roots.rs, underConfigLayerSource::User~/.agents/skills,~/.claude/skills,~/.config/opencode/skills~/.agents/skills~/.claude/skillsonlySo one write plus one link covers every host, with no Node, no
npx, and no external CLI whose flags can change under us. It also makes the install HOME-scoped and independent of the working directory — matching the MCP install, and answering "which project folder?" with "none" — and it needs no Opik credentials, so it works beforeopik configure.updatecompares a content hash rather than a commit: codeload tarballs name their root directory by ref, so the recorded "commit" was literally the stringmainand could never detect a change. Skills the pack has dropped are removed, so a rename upstream does not leave the old name behind for the assistant to keep reading.Interop is preserved:
npx skills add comet-ml/opik-skillswrites to the same place, so the two are interchangeable. A pack present but unrecorded is reported as installed outside this CLI rather than ignored, andremoveonly touches what we recorded installing, so a hand-written skill sharing a name survives.Wizard UX, and the layering it needed
The flow used to print a consent question and a config-saved log line on adjacent lines with no framing, then write files and stop — no plan, no completion signal.
termios/msvcrt. Deliberately notquestionary/prompt_toolkit: this is the core SDK, and a picker is not worth a dependency in every user's environment. Falls back to a plain confirmation where the terminal cannot host it.✓ Doneblock stating what was set up, for which assistants, and the one next action.InstallViewis now a port inconfigurator/;LoggingInstallViewis the library-safe default, soopik.configure()called from Python still just logs;RichInstallViewlives incli/.configurator/skills/install.pyreturns anInstallResultand renders nothing.Non-interactive behaviour
Every command in the group is now exercised with stdin closed, across an 18-case matrix. That found four prompts reachable from a headless path, each of which aborted the run:
opik mcp configure --host cursor--skillsopik skills configure(no--host)Aborted!--hostopik skills remove(no-y)Aborted!-y--install-mcp(no--host)EOFErrorin the numbered menu — found in reviewopik mcp configure --host … --skillswithOPIK_API_KEY/OPIK_WORKSPACEin the environment is a genuinely headless entry point.opik configureis not — it asks its own deployment-type and workspace questions first, which reproduces onmainand is out of scope here; the docs no longer advertise it as the CI path.Out of scope
ANALYTICS:comments at each call site naming the event it owes — including the decline rate on the consent prompts, which is currently unobservable and is the number that would justify or kill the prompt changes above.clineandcontinueMCP host targets — one install each in the telemetry, and config locations I could not verify.opikskill thatopik-claude-code-pluginalso ships. The installer flags the overlap and says how to drop one, but which repo owns that skill is someone else's call.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
codex mcp add/codex mcp get --jsonagainstcodex-rs/cli/src/mcp_cmd.rs, Codex's skill roots againstcodex-rs/ext/skills/src/host_roots.rs, and the opencode MCP and skills schemas against opencode's docs.-ysemantics, the decision to install skills without a third-party CLI, the prompt ordering and wording, and the docs.Testing
~3,700 test lines added across 15 files, 10 of them new. Highlights:
--install-mcpand--install-skillshonoured with no TTY, with and without-y;--no-*still wins; non-interactive with no flag skips; prompts name one host and several; no prompt when nothing is detected.is_interactiveforced False andbuiltins.inputpatched to raise, then asserting the installer still dispatched — so a future regression fails loudly instead of hanging. Pytest detaches stdin, so the suite needed an autouse fixture defaulting to "has a terminal"; without it the new guard silently changed what every prompt-driven test was exercising.--host: single / repeated / de-duplicated /all/allwith nothing detected / unknown value rejected by the parser; non-interactive with--hostsucceeds and without it errors suggesting the flag; non-interactive + unconfigured Opik errors instead of launching the interactive wizard.total, unparseable body, 401/403, 5xx, network failure; hosted probe where 401 is healthy and 404 is not; the API key never appears in a failure message.streamable_httpnormalised tohttp; opencode config-dir resolution acrossOPENCODE_CONFIG_DIR/XDG_CONFIG_HOME/ default,.jsoncpreferred only when it already exists, and unrelated keys preserved.q/Ctrl-C key handling, Windows two-byte arrow prefixes, cancel returningNoneversus a deliberate empty selection returning[], and rendering captured throughConsole.capture().SKILL.md, and no-skills-at-all; content hash stable across reads, sensitive to content, and insensitive to the tarball root name;write_skillreplaces a skill entirely (a file dropped upstream must not survive), leaves no staging dir, and replaces a symlink without following it; symlink→copy fallback; uninstall leaves un-recorded skills alone.A test caught a real bug in my own code:
PurePosixPath(".").partsis empty, which made the traversal guard vacuously true and let.through as a skill name.Documentation
The public MCP page is reframed around agent velocity: it now opens with what the setup unlocks and four copy-paste starter prompts, with the per-host technical detail moved below the fold. The old page led with transport tables and per-host JSON, which answered "how do I wire this up" but never "why would I".
prompt_engineering/mcp-server.mdxopik mcp configure --host … --skillsdocumented as the headless path; Codex, opencode and the skill pack addedtracing/advanced/sdk_configuration.mdxopik configureflag tri-states (--install-mcp/--install-skills) and the terminal requirement stated plainlyhome.mdx,integrations/overview.mdxThe docs no longer advertise
opik configure --install-mcpas the CI path, because it is not one — that was a review finding, and the correction points atopik mcp configureinstead.