[OPIK-8062] [SDK] feat: one-command MCP + skill pack setup - #7958
[OPIK-8062] [SDK] feat: one-command MCP + skill pack setup#7958alexkuzmik wants to merge 32 commits into
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>
⏱️ pre-commit per-hook timing
⏭️ 38 skipped (no matching files changed)
|
|
Could not tell whether this needs a test. This is a local CLI flow: 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 25 Aug 13:47 UTC — nothing the verdict depends on changed. |
|
🌿 Preview your docs: https://opik-preview-01a0392c-938b-761e-8b8f-bb369cfb6131.docs.buildwithfern.com/docs/opik No broken links found Unverified links (timeout / rate-limited / server error — not failing the check)• https://aistudio.google.com/apikey (401) 📌 Results for commit 8ae6250 |
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>
| 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)4 811 tests 4 809 ✅ 2m 7s ⏱️ Results for commit c0c4e64. ♻️ 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.
| 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.
The page had inflated to 694 lines and buried the setup steps under prose. Now 556, reordered so the top reads benefit -> configure -> verify -> use, with reference material below it. - Preamble cut from twelve lines to two. "What this unlocks" already carries the benefit; the MCP-server-vs-skill-pack mechanics were explaining machinery before the reader had a reason to care. - Steps moved back up under it, stripped of three pasted terminal transcripts. The verification guarantee survives as prose, which is the part that means something; the ASCII of it did not. - The starter prompts moved to their own section after the steps, each in a titled block, so they read as four labelled actions instead of four anonymous grey boxes. Absorbed "Using the MCP server" into the same section rather than leaving a second usage section 400 lines down, and the tool table came with it — reworded around what each tool lets the assistant do. - Dropped the standalone skill-pack section and the scripts/containers block. Both framed skills and flags as parallel workflows to learn, when the intent is configure once and get on with it. `opik skills update` survives as two lines under maintenance, since the pack does go stale and nothing else mentioned it. Install is now `pip install --upgrade opik` with no floor. A pinned minimum goes stale every time these commands gain a host or a flag, and a reader already on a newer version gains nothing from being told one. This also retires the bump-at-release chore noted on the ticket. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| codex mcp add opik-mcp \ | ||
| --env OPIK_API_KEY=<your-key> \ | ||
| --env OPIK_WORKSPACE=<your-workspace> \ | ||
| -- uvx opik-mcp |
There was a problem hiding this comment.
The copy-paste Codex command leaves OPIK_API_KEY=<your-key> and OPIK_WORKSPACE=<your-workspace> unquoted, so Bash treats <...> as redirection and fails before codex runs — should we quote these placeholders (e.g. --env 'OPIK_API_KEY=<your-key>') and apply the same fix to other copy-paste shell commands in this file?
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
`apps/opik-documentation/documentation/fern/docs-v2/prompt_engineering/mcp-server.mdx`
around lines 418-421, update the Codex `codex mcp add` command so the
`OPIK_API_KEY`/`<your-key>` and `OPIK_WORKSPACE`/`<your-workspace>` placeholders are
quoted (e.g. `--env 'OPIK_API_KEY=<your-key>'`) or replaced with shell-safe placeholder
text so Bash doesn't interpret `<...>` as redirection. Apply the same treatment to any
other copy-paste shell commands in this file with unquoted angle-bracket placeholders,
and keep the equivalent TOML example consistent if needed.
| configured_hosts = mcp_installer.setup_mcp_server( | ||
| **dict(setup_params), | ||
| force_local_server=force_local_server, |
There was a problem hiding this comment.
Misleading host identifier naming
configured_hosts holds host keys, but its name conflicts with the established host_keys terminology and obscures the value's contract — should we rename it and related usages to configured_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/cli/assistants.py` around lines 48-72, update the `setup` function
so the host-key list returned by `setup_mcp_server` is named `configured_host_keys`
instead of `configured_hosts`. Rename all related checks and arguments passed to
`setup_skills` and `skills_roots.display_names` so the variable’s key-based contract
is explicit and consistent with the existing `host_keys` terminology.
| def _resolve_host_keys(hosts: Tuple[str, ...]) -> Optional[List[str]]: | ||
| """Turn ``--host`` values into host keys, or ``None`` when none were named.""" | ||
| if len(hosts) == 0: | ||
| return None | ||
|
|
||
| if HOST_ALL in hosts: | ||
| detected = skills_roots.detected_host_keys() | ||
| if len(detected) == 0: | ||
| raise click.ClickException( | ||
| "`--host all` found no supported AI host on this machine. Name one " | ||
| f"explicitly instead: {', '.join(HOST_KEYS)}." | ||
| ) |
There was a problem hiding this comment.
Duplicated host-option resolution drifts
_resolve_host_keys duplicates the --host resolution flow in opik/cli/mcp.py:76-97, so changes to all expansion, validation, or errors can drift between commands — should we move it into a shared CLI helper parameterized by the known host keys and detected-key provider while keeping skills_roots and mcp_targets separate?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Commit 996d617 addressed this comment by deleting the _resolve_host_keys implementation and its associated skills command entirely.
| status_view.render_skills_status( | ||
| statuses=skills_manifest.collect_status(), | ||
| shared_dir=skills_roots.shared_skills_dir(), |
There was a problem hiding this comment.
Malformed manifest crashes skill lifecycle commands
collect_status() inserts unvalidated manifest skills into set(recorded) and sorts them with on-disk names, so malformed entries crash opik skills status before the documented fallback can report valid on-disk skills — should we filter skills to strings (or treat invalid records as absent) before the filesystem/name operations?
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/skills.py` around lines 194-196, the `status` command calls
`skills_manifest.collect_status()`, which can crash when persisted manifest entries are
non-strings. Update the `collect_status` logic to filter the recorded `skills` values to
strings, or treat malformed records as absent, before set construction, filesystem
checks, and sorting. Preserve on-disk skill detection and add regression tests for
numeric, null, and object entries so `status`, `update`, and `remove` continue safely
with malformed manifests.
There was a problem hiding this comment.
Commit 996d617 addressed this comment by removing collect_status() and the associated skill-status handling entirely, eliminating the malformed-entry crash path.
| if host_keys: | ||
| explicit: List[mcp_targets.HostTarget] = [] | ||
| for key in host_keys: | ||
| target = mcp_targets.find_target(key) | ||
| if target is None: | ||
| # Unreachable through the CLI, which validates against HOST_KEYS; | ||
| # reachable from a direct library call. | ||
| LOGGER.debug("Unknown AI host %r requested", key) | ||
| continue | ||
| explicit.append(target) | ||
| return explicit | ||
|
|
There was a problem hiding this comment.
Explicit empty host selection installs detected hosts
if host_keys: treats explicit host_keys=[] like host_keys=None, so it falls through to detected_targets() and may register detected assistants; _confirm_targets repeats this distinction and can open the interactive picker. Should we use if host_keys is not None in both paths, retain None as the automatic-detection sentinel, and add an explicit-empty test?
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 285-312, update
`_candidate_targets` and `_confirm_targets` so `None` remains the only sentinel for
automatic detection, while an explicit empty `host_keys` list means no hosts and never
invokes detection or the interactive picker. Replace the truthiness checks with
`host_keys is not None` consistently, and add a test verifying that `host_keys=[]`
returns no selected targets without registering detected assistants.
Unrelated to the MCP work on this branch — these 22 failures reproduce on `main`
with everything else stashed. Fixing them here because the branch cannot be
verified green otherwise.
Two independent causes.
`rouge-score` was never declared. Not in `tests/test_requirements.txt`, not in
`setup.py`, not installed by any workflow — so the 18 ROUGE tests in
`test_heuristics.py` could not pass on a clean install, only on a machine that
happened to have the package. Declared as a test requirement.
The other four asserted on `caplog` while the opik logger does not propagate.
`_logging.py` sets `propagate = False`, so the bare pytest fixture captures
nothing: the warning was being emitted the whole time and visible in captured
stderr, only the assertion could not see it.
assert ('span-id' in '')
where '' = <LogCaptureFixture>.text
-- Captured stderr --
OPIK: Span 'span-id' exceeded the per-span size limit of 1.0 MB ...
`tests/unit/conftest.py` already provides `capture_log` for exactly this — it
flips propagation for the duration and yields `caplog`. These four were the only
tests reaching for the raw fixture instead.
Whole unit suite now: 4809 passed, 3 skipped, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review pass against the goal that this branch's changes stay in their own
namespaces and leave the existing configure flow alone. Three findings, no
behaviour change — 988 tests in the touched areas pass untouched.
**The existing configurator had absorbed the feature.** `configure.py` carried
both consent policies, the prompt wording and a text helper: ~140 lines that are
about assistants, not about configuring Opik. Moved to a new
`configurator/assistants.py` as pure functions of their arguments — a `Decision`
enum plus `mcp_decision` / `skills_decision` / prompt text. The methods stay as
thin delegations, so all 44 existing test call sites keep working as they were.
Footprint on the pre-existing namespace:
configurator/configure.py +153 -11 -> +108 -20
cli/configure.py +105 -4 -> +99 -4
2.9% of the diff now touches pre-existing configure code, down from 4.1%.
**A duplicated helper, one copy of which crashes.** `_readable_list` existed in
both files and the copies had diverged: the CLI one indexes `names[-1]` and
raises `IndexError` on an empty list, where the other returns `""`. Unreachable
today behind a `len(detected) == 0` guard, one refactor from a crash. Now one
implementation.
**Two tangled CLI edges.** `skills.py` imported the *orchestration* module purely
to borrow a renderer, and `configure.py` reached into an MCP-named module for a
generic console. `cli/mcp_view.py` -> `cli/install_view.py` (it was never
MCP-specific — mcp, skills and configure all use it) and `render_skill_pack`
moved there, which drops both edges; `skills.py` no longer imports `assistants`
at all. Import graph verified acyclic: views are leaves, orchestration depends on
views, commands depend on orchestration.
Left alone deliberately: `mcp.py -> configure.py`, for one
`run_interactive_configure(install_mcp=False)` auto-launch. Breaking it needs a
shared bootstrap module for a single call site, which is worse than the edge.
Also removed dead code — `skills.detected_host_names` and `roots.needs_link`
(zero callers in `src/`, the latter alive only via its own test), and three test
patches left over from when the skills prompt named hosts. After this there is
not one unused symbol in 3047 lines of src. Added 23 tests for the extracted
policy as decision tables, which is what the consent rules always were.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| skills_flag = install_skills | ||
| if install_mcp is False: | ||
| # Server declined outright: only the pack is on the table. | ||
| if skills_flag is False: | ||
| return | ||
| assistants.setup(setup_params, skills_flag=True, host_keys=None) | ||
| return | ||
|
|
||
| if install_mcp is None and not _confirm_assistant_step(): | ||
| return | ||
|
|
There was a problem hiding this comment.
assistants.setup(..., skills_flag=True) still unconditionally calls mcp_installer.setup_mcp_server() for --no-install-mcp --install-skills, so skills-only setup registers MCP and can duplicate opik mcp configure — should we skip that call when install_mcp is false?
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/configure.py` around lines 34-45, fix the assistant-setup
dispatch used by the configure command and `run_interactive_configure` so
`--no-install-mcp --install-skills` (and any `install_mcp=False` path) cannot register
an MCP server indirectly through `assistants.setup`. Split skill-only installation from
MCP setup, or pass an explicit operation/flag that prevents
`mcp_installer.setup_mcp_server()` from running when MCP installation is disabled —
including host planning and prompting — while preserving skills installation,
assistant detection, and the existing MCP prerequisite behavior for `opik mcp
configure`.
| self.automatic_approvals, | ||
| ) | ||
| return | ||
|
|
There was a problem hiding this comment.
No-MCP flag still writes host configuration
The assistant-setup callback routes install_mcp=False through assistants.setup(...), which still installs MCP before skills, so opik configure --no-install-mcp can write MCP registrations when skills are enabled or unspecified — should we dispatch to a skills-only installer or make the shared setup honor install_mcp=False?
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/configure.py around line 127, update the
assistant-setup callback in `OpikConfigurator.configure` so `install_mcp=False` can
never install or register MCP servers. Route this case through a skills-only installer,
or update the shared assistant setup call to honor the flag and skip MCP installation
while retaining skills setup.
| def render_skill_pack( | ||
| result: skills_install.InstallResult, view: mcp_view.InstallView | ||
| ) -> bool: | ||
| """Report a skill-pack install. Returns whether it succeeded.""" | ||
| if not result.succeeded: | ||
| view.problem(f"Could not install the Opik skill pack: {result.error}.") |
There was a problem hiding this comment.
MCP view becomes cross-feature catch-all
render_skill_pack puts skill-pack reporting and skills_install/skills_roots in the MCP-focused install_view.py, so the view becomes a cross-feature catch-all — should we move it to a skill-specific CLI/view module or expose it from skills.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/install_view.py` around lines 179-184, move the
`render_skill_pack` function into a skill-specific CLI/view module (or expose it from
`skills.py`) because this MCP-only rendering module should not handle skill-pack
installation. Move the `skills_install` and `skills_roots` imports with the function,
then update all callers and tests to use the new location while preserving the existing
behavior.
Three failures on the pushed commit, all reproduced locally first.
`ruff-format` and `mypy` failed on files this branch added but that my local
runs never checked: I had been running the hooks on each step's changed files,
while CI runs them over every file in the PR. `test_verification.py` needed
reformatting and `verification.py:40` was missing a return annotation. Both were
invisible to `git diff --name-only | xargs pre-commit run --files` because the
files were untouched by the most recent commits. Reproduced by running the hooks
over `git diff --name-only $(git merge-base HEAD origin/main) HEAD`, which is
CI's actual scope.
The unit failure was a click version difference, not a code bug.
`test_configure__no_flag__defaults_to_detected_hosts` invoked the command with
no stdin and relied on `click.confirm` returning its default at EOF:
click 8.1.8 -> empty stdin + default=True -> returns True, exit 0
click 8.4.2 -> empty stdin -> Abort, exit 1
CI resolves 8.4.2, my venv had 8.1.8, so it passed locally on every Python
version and failed on all five in CI. One detected host takes the plain
confirmation rather than the picker, so the test now answers it with
`input="y\n"` instead of depending on which click is installed. Verified by
pinning 8.4.2 locally, reproducing the exact single failure, then fixing it —
whole suite green on 8.4.2: 4809 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both steps write into configuration files owned by other tools — `~/.claude.json`,
`~/.cursor/mcp.json`, `~/.agents/skills`. That is not something to do to a machine
nobody is sitting at, so it now happens only in a session the user is present for.
Flags say *which* assistant and *whether the user wants it*; they are not a licence
to write unattended.
This reverses the headless story earlier in the branch, and closes a regression it
had introduced. Measured against the merge-base with a detected host present:
main: configure(install_mcp=True), no tty -> wrote nothing
was: configure(install_mcp=True), no tty -> wrote ~/.cursor/mcp.json
now: configure(install_mcp=True), no tty -> wrote nothing
That was the only behavioural difference this PR had for non-interactive callers;
`configure()` and `configure(-y)` matched main throughout. The branch now matches
main exactly with stdin closed.
The rule lives in `configurator/assistants.py` (`mcp_decision` / `skills_decision`
return SKIP before consulting any flag), with backstops in both installers so a
library caller cannot route around it, and refusals in `opik mcp configure` /
`opik skills configure` that say why instead of aborting. `--host` no longer
implies consent; `opik mcp status`, `opik skills status` and `opik configure -y`
are unaffected.
BREAKING CHANGE: `install_mcp` is removed from the public `opik.configure()`.
It has been there since June (#6959), so callers passing it now get a TypeError.
Removed at the maintainer's request after the compatibility cost was raised: with
the step interactive-only, a flag on a programmatic entry point promises something
it cannot deliver. `install_skills` and `assistant_setup` — both added earlier in
this branch and never released — are removed from the public signature too;
`assistant_setup` was always CLI-internal wiring. All three remain on
`OpikConfigurator`, which is what the CLI now constructs directly.
Docs no longer advertise a headless path, because there isn't one: the MCP page
and `sdk_configuration` both state the terminal requirement and point at Manual
setup for images and scripts.
Tests: 12 cases that encoded "the flag beats a missing terminal" are inverted, a
`TestTerminalRequirementIsAbsolute` class asserts no flag value ever proceeds
without a session, and `skills/test_install.py` gains the autouse interactive
fixture the other suites already had, since it covers installer mechanics rather
than consent. 4817 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # The backstop for every caller, library included. Writing into a config file | ||
| # another tool owns is only done in a session someone is present for; flags | ||
| # and `host_keys` choose *which* assistant, never *whether* without a user. | ||
| if not interactive_helpers.is_interactive(): | ||
| display.skipped( | ||
| "Skipping MCP server setup: no interactive terminal. Run " | ||
| "`opik mcp configure` from a shell to set it up." | ||
| ) | ||
| # ANALYTICS: install skipped, reason="non_interactive". | ||
| return [] |
There was a problem hiding this comment.
setup_mcp_server() returns [] unconditionally in non-interactive terminals, so explicit headless requests such as opik configure -y --install-mcp or --host silently skip installation; the analogous guard in sdks/python/src/opik/configurator/skills/install.py does the same for --install-skills. Should we let explicit consent (install_mcp=True/assume_confirmed) or host selection bypass both guards while still skipping implicit prompt-driven installs?
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 75-84, update
`setup_mcp_server()` so the non-interactive early return does not unconditionally block
explicitly requested MCP installations. Propagate/honor an explicit-consent signal (e.g.
`install_mcp=True`, `assume_confirmed`) or explicit host selection to allow headless
installation to proceed, while still skipping implicit, unattended prompt-driven
installs. Apply the same consent-aware fix to the analogous guard in
`sdks/python/src/opik/configurator/skills/install.py` for `--install-skills`. Add tests
covering headless explicit installation and the default no-consent/implicit skip path
for both modules.
There was a problem hiding this comment.
Commit 060f64d addressed this comment by allowing headless MCP installation when hosts are explicitly named or consent is explicit, while retaining the implicit skip path. The analogous skills guard was removed, with explicit --install-skills consent and tests for both behaviors.
…oughout
`--host` was the MCP specification's word, not a word users have. The spec's
architecture is Host / Client / Server, where the host is the user-facing AI
application — correct for code implementing the protocol, wrong on a flag. And
the CLI was contradicting itself: "AI host" appeared 33 times in user-visible
text against 15 for "AI assistant", with the two colliding in the same breath —
the picker asked "Which AI assistant…?" directly above a flag called `--host`.
Checked what comparable installers actually expose before picking:
MCP spec Host (architecture only, no flag)
Linear docs client "compatible clients"
install-mcp (18 targets) client --client
mcp-add / Smithery / fastmcp client --client
Neon add-mcp agents -a, but its own output column
still reads "MCP Client"
Nobody ships `--host`. `--client` is the de facto standard, so `--ai-client`
keeps that recognisability while disambiguating from an HTTP or API client.
"agent" was considered and rejected: Opik's docs use "agent" 224 times for the
application being *traced*, so "set Opik up for your agent" would read as
"instrument my agent" — the wrong meaning, and the more plausible one in an
observability product.
User-visible text is now "AI client" everywhere; `HostTarget`, `HOST_KEYS` and
`host_keys` keep the spec's vocabulary, because that layer genuinely implements
the MCP host concept. `--host` on `opik proxy` is untouched — it is a network
bind address and has nothing to do with this.
Two corrections that came out of the pass rather than the rename:
- Help text and docstrings still advertised the headless path removed in the
previous commit — "the flag to use from a script, a Dockerfile, or a coding
agent", "usable from CI … nothing is prompted". Those were false as written.
They now say naming a client skips the picker, not the terminal requirement.
- The status header briefly rendered "configured for 1 AI AI client", because
the f-string already prefixes "AI " and the substitution added another.
No deprecation shim: `--host` has zero occurrences on main, so nothing released
ever accepted it. 4817 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| """Register the Opik MCP server with the user's AI client(s). | ||
|
|
||
| The decision of *whether* to run this lives in the configurator; by the time | ||
| this is called the user has opted in and the session is interactive. | ||
| The decision of *whether* to run this lives in the callers; by the time this | ||
| is called the user has opted in. | ||
|
|
||
| ``check_tls_certificate`` and ``force_local_server`` are keyword-only with | ||
| backward-compatible defaults, so the original positional call pattern |
There was a problem hiding this comment.
Public MCP API contract is incomplete
The public setup_mcp_server docstring mentions host_keys and assume_confirmed but omits accepted selector values, per-client installation outcomes, and its configuration-write and server-verification side effects, so callers using the re-export in sdks/python/src/opik/configurator/mcp/__init__.py cannot understand the full contract — should we document these details?
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 49-65, update the public
setup_mcp_server docstring to document the actual accepted host_keys selector values,
the return value including per-client installation outcomes, and its side effects.
Explicitly state that the function persists each selected client’s configuration and
verifies the configured MCP server before reporting success; derive the selector names
and result shape from the target implementations and callers so the documentation stays
accurate.
| The hosted server connects over HTTP and signs in with a browser-based OAuth | ||
| flow on first connect — no API key is stored in the host config. Point your host | ||
| flow on first connect — no API key is stored in the client config. Point your client | ||
| at your deployment's MCP endpoint, which is your Opik API base plus `/v1/mcp`. On | ||
| Opik Cloud that is `https://www.comet.com/opik/api/v1/mcp`. |
There was a problem hiding this comment.
Manual setup targets production by default
The manual setup instructions present https://www.comet.com/opik/api/v1/mcp for arbitrary deployments, so users copying them for self-hosted environments can connect to Opik Cloud instead — should we use https://<host><rootPath>/v1/mcp as the generic template and move the Cloud URL to a labeled example?
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
`apps/opik-documentation/documentation/fern/docs-v2/prompt_engineering/mcp-server.mdx`
around lines 242-245, update the manual hosted-server setup instructions so the primary
endpoint example is deployment-agnostic rather than implicitly directing users to Opik
Cloud. Use an explicit template such as `https://<host><rootPath>/v1/mcp`, then provide
`https://www.comet.com/opik/api/v1/mcp` only in a separately labeled Opik Cloud example.
There was a problem hiding this comment.
Commit 0292f1c addressed this comment by making the instructions deployment-agnostic and explicitly labeling the Opik Cloud URL as an example.
| "--ai-client", | ||
| "hosts", | ||
| multiple=True, | ||
| type=click.Choice(HOST_KEYS + [HOST_ALL], case_sensitive=False), | ||
| help="AI client to install the skill pack for. Repeatable, or pass `all` for " | ||
| "every host detected on this machine. Defaults to every detected host.", |
There was a problem hiding this comment.
Update deletes unrelated Claude skill links
Dropped-skill cleanup in configurator/skills/install.py iterates every skills_roots.LINKED_HOST_KEYS entry instead of recorded hosts, so dropping instrument upstream can delete an unrelated hand-written ~/.claude/skills/instrument when the Opik install recorded only codex — should we restrict deletion to recorded hosts? The cleanup at sdks/python/src/opik/configurator/skills/install.py:271-276 may need the same change.
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/skills.py` around lines 84-89 and
`sdks/python/src/opik/configurator/skills/install.py` around lines 271-276, fix the
skill installation cleanup so explicit `--ai-client` selections record only the selected
hosts and dropped-skill deletion iterates those recorded hosts rather than every
`LINKED_HOST_KEYS` entry. Preserve unrecorded links, such as a hand-written Claude skill
when only Codex was installed, and add or update a regression test covering this
scenario.
There was a problem hiding this comment.
Commit 996d617 addressed this comment by removing the affected skill update/uninstall cleanup code, including the iteration over every linked host. The related CLI implementation was also deleted.
…C key
Triaged all 67 bot review comments. Most were already marked addressed by earlier
commits or superseded by the interactive-only change; four were live defects that
reproduced against current code.
**A security test that could not fail.** `test_read_archive__path_traversal__is_rejected`
ended in
assert set(result.skills) <= {"opik", "escape"} - {"escape"} or True
so it passed whatever the parser returned, including an accepted `../../` entry —
in the test whose whole job is proving traversal is rejected. Replaced with an
exact-structure assertion. The parser turned out to be correct, so nothing was
hiding behind it; it just could have been. Same class as the
`PurePosixPath(".").parts` bug found earlier in this branch, which is exactly why
this one mattered.
**A broken hosted endpoint reported as healthy.** `verify_hosted_endpoint` only
failed on 404, so 400/500/502/503 all returned `succeeded=True` and the user was
told to expect a sign-in prompt. Only the 401/403 challenge proves a working
endpoint now; anything else fails with the status and next step. That undercut the
point of the verification step, which exists so a broken setup does not look
identical to a working one.
**Escape did nothing until you pressed another key.** `_read_key_posix()` followed
a bare `\x1b` with a blind `read(1)`, which blocks — so Escape appeared inert and
then swallowed the next keypress. An arrow key arrives as one burst, so a short
`select()` distinguishes them. Verified through a real pty: bare ESC now returns
`cancel` with no second key. Two unit tests cover `_has_pending_input` over a pipe
rather than relying on a pty.
**CLI tests in the configurator suite.** `TestRichInstallView` and
`TestChooseHosts` imported `opik.cli.install_view` from inside their methods —
invisible to a top-level grep — so the configurator suite depended on the CLI
layer it is meant to be independent of. Moved to `tests/unit/cli/test_install_view.py`;
`tests/unit/configurator/` no longer imports `opik.cli` at all.
Reported but deliberately not changed here, since each is a design decision rather
than a fix: the Codex API key in argv (the alternative is hand-editing TOML, which
this installer avoids on purpose), `codex mcp remove` running before `add` is
confirmed, manifest-name containment before `_remove_path` (needs attacker write
access to HOME already), and an aggregate expansion cap on the skill archive (per-
member and compressed caps already exist).
4823 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| A terminal emits an arrow key's whole escape sequence in one burst, while a | ||
| bare Escape arrives alone. A short select() is enough to tell them apart and | ||
| keeps Escape responsive instead of blocking on the next keypress. |
There was a problem hiding this comment.
Split escape sequences misclassified as cancellation
When an arrow key's escape sequence is split across reads, _has_pending_input() can return False after ESC before the continuation arrives, so the selector cancels instead of recognizing the arrow key — should we describe the short select() as a timeout heuristic and document this behavior rather than presenting burst delivery as guaranteed?
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/selector.py around lines 180-182, revise the
`_has_pending_input` documentation to describe `select()` as a short-timeout heuristic
rather than assuming terminal input delivers a complete arrow-key sequence in one burst.
Document that split escape sequences may cause the check to return false and the
selector to treat the initial ESC as cancellation, and align the nearby comments around
lines 202-208 with this behavior.
| bare Escape arrives alone. A short select() is enough to tell them apart and | ||
| keeps Escape responsive instead of blocking on the next keypress. | ||
| """ | ||
| import select |
There was a problem hiding this comment.
Scattered standard-library import
select is only imported inside _has_pending_input, so should we move it to the module-level imports for consistent dependency placement?
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/selector.py` around line 184, update the `_has_pending_input`
helper so its `select` dependency is imported at module scope rather than inside the
function. Remove the local import and leave the helper focused only on checking input
readability without changing its behavior.
There was a problem hiding this comment.
Commit 0292f1c addressed this comment by moving select from _has_pending_input to the module-level imports.
| ready, _, _ = select.select([descriptor], [], [], timeout) | ||
| return bool(ready) |
There was a problem hiding this comment.
Arrow keys can be misread as cancellation
select() is used to classify a single available ESC as a bare Escape, so a split ESC + [A returns False and CANCEL while [A remains buffered as later input — should we read with a short deadline and parse the accumulated bytes?
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/selector.py` around lines 186-187, fix `_has_pending_input`
and its Escape-handling logic so `select()` readiness is not treated as proof that the
input is a bare Escape. Read and accumulate the escape sequence with a short deadline,
then parse the collected bytes to distinguish `ESC` from fragmented cursor-key
sequences; ensure any incomplete or recognized sequence is handled without leaving
`[A`-style bytes buffered as later input.
There was a problem hiding this comment.
Commit 8836a3e addressed this comment by reading directly from the descriptor and combining a pending continuation with the initial ESC before parsing. However, it still performs only one follow-up read and does not robustly accumulate fragmented sequences with a deadline.
| def test_no_bytes_waiting__is_false(self): | ||
| read_fd, write_fd = os.pipe() | ||
| try: | ||
| assert selector._has_pending_input(read_fd, timeout=0.01) is False |
There was a problem hiding this comment.
Windows unit tests fail before assertions
Both tests pass os.pipe() descriptors to _has_pending_input(), which calls select.select(), so they raise OSError on Windows because select() supports only sockets and the suite fails before assertions — should we mark this test class POSIX-only or replace it with coverage for _read_key_windows()/msvcrt?
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_selector.py` around lines 290-301, the
`TestPendingInput` tests pass `os.pipe()` descriptors to `_has_pending_input()`, but
Windows `select()` does not support pipe descriptors and raises instead of asserting.
Mark this test class as POSIX-only while retaining Unix coverage, or replace it with
tests of the Windows `msvcrt` reader through a portable abstraction.
There was a problem hiding this comment.
Commit 0292f1c addressed this comment by skipping the TestPendingInput class on Windows, preventing unsupported pipe descriptors from reaching select.select().
Follow-up on the bot's review of the previous commit, which flagged the exact risk I had left open: a 50 ms `select()` could misread a slowly-delivered arrow sequence as a bare Escape, cancelling the picker instead of moving the cursor. That would be worse than the bug it fixed, because it is intermittent. What actually bounds the risk is the gap *within* one burst, not network latency: a terminal writes `\x1b[A` in a single write, and SSH delays the whole burst rather than spacing its bytes out, so the real gap is near zero. The window is now a named `ESCAPE_WINDOW = 0.12` carrying that reasoning, chosen because the costs are asymmetric — too large only delays a bare Escape by that much, while too small turns an arrow key into a cancellation. On Windows the question does not arise, and the answer is worth recording where someone will find it: `import termios` fails there, so `_key_reader()` returns the `msvcrt` reader and `_read_key_posix` — with `_has_pending_input` — is never called. That reader has no ambiguity to resolve either, because arrows arrive behind a `\x00`/`\xe0` prefix rather than behind Escape, so a bare Escape has always cancelled immediately there. The POSIX-only contract is now documented on the helper so it is not reused on a path where `select()` cannot take a pipe. Which is also the bot's fourth finding, and a real one: the tests I added call `_has_pending_input` over `os.pipe()`, and on Windows `select()` accepts only sockets, so they would error rather than fail. CI is `ubuntu-latest` only, which is precisely why it went green and I did not notice. Skipped on win32, with the reason spelled out. Also hoisted `select` to a module-level import, per the same review. Not added: a pty test for the ESC-versus-arrow decision. Two attempts hung — `sys.stdin.read(1)` on a buffered text stream blocks trying to fill its buffer, and a fork-based harness proved flaky. `_has_pending_input` is the decision point and has deterministic pipe-based coverage; a flaky test would be worse than none. 4823 passed, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…igure flow The skill pack is worth installing for people already running `opik configure` or `opik mcp configure` — that is the whole point, so they never have to reach for a separate tool. Outside that flow there is already a better answer than a second Opik command: `npx skills add comet-ml/opik-skills`, which covers 76+ clients to our five and can install per-project, which we cannot. So `opik skills configure|update|remove|status` is removed. The pack still installs as the recommended follow-up inside the MCP step, which is unchanged. Removing the command orphaned its whole lifecycle, and leaving that in place would have been dead code: `update_skills`, `uninstall_skills`, `manifest.collect_status` and friends, `SkillStatus`, `render_skills_status`, plus the helpers only they reached. All gone, with their tests — 1143 lines out, 19 in. A dead-code sweep over `configurator/skills` now reports nothing. That leaves one honest gap, and the docs say so rather than papering over it: `npx skills` cannot manage a pack Opik installed, because the two write different directories — Opik the shared `~/.agents/skills` plus a Claude Code link, `npx` each client's own directory, defaulting to project scope inside a project. So refreshing is "re-run `opik mcp configure`", which rewrites the pack from the latest published version, and removing is deleting it from `~/.agents/skills`. Docs: the MCP page gains a *Clients the CLI doesn't cover* section pointing at `npx skills` and stating the boundary — five clients natively, npx for the rest. `sdk_configuration.mdx` no longer references the removed command. Also verified and cited while researching this: `~/.agents/skills` really is read by Cursor and VS Code Copilot, which `roots.py` had asserted without a source. Both now carry one, along with the note that `.agents/skills` *without* the `~` is a project location Opik never writes. 4779 passed, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… manual setup Two bits of the page were pulling their weight the wrong way. The not-detected note said to "name it with `--ai-client claude-code`", which is advice a reader cannot act on: nothing on the page says what other values exist, so the example is either exactly right for you or useless. The flag's values are in `opik mcp configure --help`, which is the right place for them. The note now just points at Manual setup, which is what someone in that position actually needs. The npx escape hatch had grown its own `###` section, which oversold it — it is one command for a case the CLI does not cover, not a feature of the page. Folded into Manual setup, where "your client wasn't detected" is already the framing, at three lines instead of twenty. 541 lines, down from 694 at the start of the branch. All anchors still resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| For the **skill pack** on a client the CLI doesn't cover, the community | ||
| [`skills`](https://github.com/vercel-labs/skills) CLI knows the skill directories | ||
| for 76+ agents (needs Node.js): | ||
|
|
||
| ```bash | ||
| npx skills add comet-ml/opik-skills | ||
| ``` |
There was a problem hiding this comment.
The getting-started docs lead with npx skills add comet-ml/opik-skills, which executes unpinned third-party code with the user's privileges and conflicts with the PR's no-third-party-installer goal — should we make opik mcp configure or a built-in opik skills command primary, with npx skills as a pinned, integrity-verified fallback for unsupported clients?
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
`apps/opik-documentation/documentation/fern/docs-v2/prompt_engineering/mcp-server.mdx`
around lines 228-234, and the corresponding sections in
`observability/getting-started.mdx` and `prompt_engineering/getting-started.mdx`,
restructure the skill-install guidance so `opik mcp configure` (or another built-in
first-party command) is the primary documented path. Reserve `npx skills add
comet-ml/opik-skills` as an explicitly labeled fallback only for clients the Opik CLI
doesn't cover, pin it to a vetted, integrity-verified version, and avoid presenting
Node.js as a universal requirement.
There was a problem hiding this comment.
Commit 060f64d addressed this comment by documenting the built-in opik configure and opik mcp configure commands as the primary setup paths and limiting npx skills to clients the CLI does not cover. The fallback remains unpinned and Node.js-dependent, so the concern is only partially addressed.
Reported from real use: pressing the down arrow in the client picker ended the flow instead of moving the cursor. My own fix two commits ago caused it. `sys.stdin.read(1)` is buffered. An arrow key's whole `\x1b[B` burst lands in the text stream's userspace buffer and only the `\x1b` is handed back — after which `select()` on the *descriptor* correctly reports nothing pending, because the rest is sitting above the kernel where select cannot see it. So every arrow key looked like a bare Escape and cancelled. Mixing a buffered read with a readiness check on the raw descriptor was the mistake. The reader now uses `os.read` throughout, which keeps the descriptor the single source of truth: a bare Escape reads as one byte with nothing pending, an arrow reads as its whole sequence in one call, and a split sequence is completed by a second read inside the escape window. The decision is extracted into `_interpret(bytes) -> token`, which is the part that was untestable before. It is a pure function now, so the regression has real coverage: reintroducing the buffered read fails 4 of the new tests, and the fix passes all 58. Unknown sequences (Home, End, F-keys) return "" rather than CANCEL — an unmapped key must not close the picker either. Worth recording why the coverage looks like this rather than an end-to-end test: three attempts at a pty harness were flaky or hung — `rich.live` rendering into a pty, and `pty.fork()` behaving inconsistently here — and a hanging test is worse than none. The mocked-descriptor tests assert the shape of the bug (which call the reader makes, and what it does with a split burst) rather than restating the symptom, which is what actually guards it. 4799 passed, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| def test_arrow_arriving_split__still_reads_as_an_arrow(self): | ||
| """Two reads concatenated is the same input as one burst.""" | ||
| assert selector._interpret(b"\x1b" + b"[B") == selector.DOWN |
There was a problem hiding this comment.
Test name overstates split-read coverage
The test name and docstring claim to cover an arrow sequence split across two reads, but _interpret receives pre-concatenated bytes in one call, so it doesn't exercise that behavior — should we rename it for concatenated input or mock two reads through _read_key_posix?
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_selector.py` around lines 357-359, update
`test_arrow_arriving_split__still_reads_as_an_arrow` because it passes concatenated
bytes to `_interpret` in a single call, so it does not test two separate reads as its
name and docstring claim. Prefer driving `_read_key_posix` through the existing `_run`
helper with `b"\x1b"` and `b"[B"` as separate mocked reads, or rename the test and
docstring to describe concatenated input accurately.
| monkeypatch.setattr(selector.sys, "stdin", mock.Mock(fileno=lambda: 99)) | ||
| monkeypatch.setattr( | ||
| selector, "_has_pending_input", lambda descriptor, **kw: pending | ||
| ) | ||
| # termios/tty are imported inside the reader (they do not exist on |
There was a problem hiding this comment.
Descriptor regression goes undetected
The os.read and _has_pending_input mocks don't validate their fd arguments, so _read_key_posix() could use an unintended descriptor while the token assertions still pass — should we capture those calls and assert each receives sys.stdin.fileno() (99)?
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_selector.py` around lines 379-383, update
`TestReadKeyPosixUsesTheDescriptor._run` so the tests genuinely verify descriptor usage
rather than only token results. Capture the arguments passed to the mocked `os.read` and
assert its descriptor is `99`, and make the `_has_pending_input` mock record and assert
that it also receives `99`. Preserve the existing scripted-read behavior while adding
these side-effect assertions.
| pulls = iter(reads) | ||
| monkeypatch.setattr(selector.os, "read", lambda fd, n: next(pulls)) | ||
| return selector._read_key_posix() |
There was a problem hiding this comment.
Regression test does not verify descriptor usage
The os.read replacement ignores its fd argument, so tests pass when _read_key_posix() uses the wrong descriptor or buffered sys.stdin, and CI misses the arrow-key regression — should we record the descriptors, assert they are 99, and make the buffered stdin read fail if invoked?
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_selector.py` around lines 389-391, update
`TestReadKeyPosixUsesTheDescriptor._run` so the mocked `os.read` records each file
descriptor it receives and the helper asserts those descriptors are `99`, matching the
mocked `sys.stdin.fileno()`. Also make buffered `sys.stdin.read` raise if called,
ensuring these regression tests fail when `_read_key_posix()` uses the buffered stdin
path instead of `os.read`.
| """Read one keypress, telling a bare Escape from a cursor-key sequence. | ||
|
|
||
| Reads the descriptor directly rather than through ``sys.stdin``. The buffered | ||
| text stream pulls an arrow key's whole ``\x1b[B`` burst into its userspace | ||
| buffer and hands back only the ``\x1b`` — after which ``select()`` on the | ||
| descriptor sees nothing pending, because the rest is already buffered above | ||
| the kernel. That combination read every arrow key as a cancellation. Going | ||
| unbuffered keeps the descriptor the single source of truth. |
There was a problem hiding this comment.
Picker silently drops buffered input
os.read can return multiple pending bytes, but _interpret processes only the first ordinary byte and drops the rest, so pasted or rapid input is silently lost — should we document first-token handling or buffer subsequent keypresses?
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/selector.py` around lines 208-215, fix the key-reading
function so `os.read` results containing multiple keypresses are not silently discarded.
Add buffering for unread bytes, parse and return only one complete key token at a
time—including split escape sequences—and preserve the remaining bytes for
subsequent calls; update the docstring to describe this behavior.
| data = os.read(descriptor, _READ_CHUNK) | ||
| if data == b"\x1b" and _has_pending_input(descriptor): | ||
| # A bare Escape so far, but the continuation may still be in flight. | ||
| data += os.read(descriptor, _READ_CHUNK) |
There was a problem hiding this comment.
Rapid picker input loses keypresses
os.read can return multiple keypresses in one chunk, but _interpret consumes only data[2:3] and _read_keypress discards the remainder, so a trailing Space after an arrow sequence is lost and the selection is not applied — should we buffer unconsumed bytes, parse one key sequence per call, and feed leftovers into the next read?
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/selector.py` around lines 226-229, update `_read_keypress` so
a single `os.read` call cannot silently discard keypresses. Maintain a persistent
pending-byte buffer, parse and return exactly one complete key sequence per call, and
retain any trailing unread bytes (e.g. a Space or Enter following an arrow sequence) for
consumption on the next call, while preserving the existing bare-Escape, cursor-key, and
unknown-sequence behavior.
Python SDK E2E Tests Results (Python 3.10)296 tests 288 ✅ 3m 45s ⏱️ Results for commit 060f64d. ♻️ This comment has been updated with latest results. |
The flow we want is a user asking their agent to set Opik up. Tested by being one:
in this shell `sys.stdin.isatty()` is False and `is_interactive()` returns False,
so before this change every write path refused. `opik configure` aborted on the
deployment-type prompt — `-y` does not answer it — and `opik mcp configure`
refused outright, `--ai-client` included. Exactly one thing worked,
`opik configure --use_local -y`, and it never reached MCP or skills.
The interactive-only rule from earlier today was answering the wrong question. It
asked "can I prompt?" when the thing that matters is "did the user ask?". Those
come apart precisely here: an agent has no tty but a live instruction, while a CI
runner has no tty and no instruction. Both looked identical to `is_interactive()`.
So the rule is now about intent, and the flag is how intent is expressed:
named flag / client proceed, terminal or not
terminal, nothing named ask
no terminal, nothing named skip
CI stays protected, because a job that names nothing still writes nothing — which
is what the original regression was really about. Verified both directions:
opik configure -y --install-mcp --install-skills config + mcp + skills
opik mcp configure --ai-client cursor --skills mcp + skills
opik configure -y config only
opik mcp configure refuses, names --ai-client
`opik configure` also stopped asking the deployment type when it cannot: the
environment already answers it, so `OPIK_URL_OVERRIDE` and `OPIK_API_KEY` are read
for the shape and only a bare environment errors — naming what to set.
Every refusal on this path now names the remedy, because the caller we are aiming
at reads stderr and retries. `--install-mcp` used to die on whichever prompt came
first with a bare `Aborted!` (the project-name question, as it happens); the
command now fails fast saying to add `-y`. "Run it from a shell" is a dead end for
an agent, so it is gone.
This reverses part of the interactive-only commit, and 14 tests that encoded it
are inverted back with their names and reasons updated. Added coverage for the
flow itself: deployment inference per environment shape, the flag reaching the
installer as consent, an unflagged no-terminal run reaching nothing, and the
guard naming `-y`.
Not addressed, and worth a follow-up: a rejected cloud API key surfaces as
"API key missing" rather than "invalid", which is misleading when the key was
present but wrong. Pre-existing, and out of scope here.
4809 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| url = os.environ.get("OPIK_URL_OVERRIDE", "").strip() | ||
| if url: | ||
| if url_helpers.get_base_url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fcomet-ml%2Fopik%2Fpull%2Furl).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.
| "--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.
| 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(/sitelet?url=https%3A%2F%2Fgithub.com%2Fcomet-ml%2Fopik%2Fpull%2Furl).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>
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.