Add agent skill installer prompt to CLI login and post-install#805
Open
Add agent skill installer prompt to CLI login and post-install#805
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new CLI module to detect local AI agents and install the osmo-agent skill (via npx), invokes an interactive prompt after device-code login, integrates a terminal-only post-install step for Linux/macOS installers, and adds unit/integration tests and Bazel targets. Changes
Sequence DiagramsequenceDiagram
participant User
participant Login as Login Flow
participant Installer as Skill Installer
participant AgentDetect as Agent Detection
participant TTY as Terminal/TTY
participant NPX as NPX/Registry
participant FS as File System
User->>Login: complete device-code login
Login->>Installer: prompt_skill_installation()
Installer->>TTY: is_interactive_terminal()
alt Not interactive
Installer-->>User: skip prompt
else Interactive
Installer->>AgentDetect: detect_agents()
AgentDetect->>FS: check agent config dirs
AgentDetect-->>Installer: detected agents
alt no agents
Installer-->>User: return
else agents exist
Installer->>FS: check prompt-declined marker
alt declined
Installer-->>User: return
else
Installer->>AgentDetect: is_skill_installed() per agent
alt installed everywhere
Installer-->>User: return
else missing
Installer->>TTY: prompt choices (install now / not now / don't ask)
TTY-->>Installer: user choice
alt INSTALL_NOW
Installer->>NPX: find_npx()
alt npx not found
Installer-->>User: print Node.js + manual npx instructions
else npx found
Installer->>NPX: run npx skills add nvidia/osmo
NPX-->>Installer: result
Installer-->>User: print success/failure message
end
else INSTALL_NEVER
Installer->>FS: save declined marker
Installer-->>User: return
else INSTALL_NOT_NOW
Installer-->>User: return
end
end
end
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jiaenren
commented
Apr 4, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #805 +/- ##
==========================================
+ Coverage 42.88% 43.20% +0.32%
==========================================
Files 203 204 +1
Lines 26934 27355 +421
Branches 7619 7833 +214
==========================================
+ Hits 11551 11820 +269
- Misses 15276 15424 +148
- Partials 107 111 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/agent_skill.py`:
- Around line 1-17: Add a module-level pylint waiver for long lines by inserting
the comment "# pylint: disable=line-too-long" at the top of the file so the long
SPDX copyright/header line in the module header is exempt; ensure the comment is
placed before or immediately after the SPDX header block so pylint ignores the
header-only line-length violation.
- Line 177: Remove the unnecessary f-string in the print call that currently
reads print(f"\n AI Agent Integration\n") to avoid the F541 lint error;
replace it with a plain string literal (e.g., print("\n AI Agent
Integration\n")). Update the print statement where it appears in agent_skill.py
so no f-prefix is used.
- Around line 218-231: The uninstall runs before verifying npx is available, so
using --force can remove a working install and then fail; change the flow in the
CLI handler (check args.force, uninstall_skill, find_npx,
agents_needing_install) so that find_npx() is called and npx_path validated
before calling uninstall_skill(agents), and only call uninstall_skill after a
successful install attempt (or at minimum after confirming npx is present);
update logic around args.force and agents_needing_install to perform the
preflight npx check first and defer/unlink uninstall_skill until the replacement
install is known to succeed.
- Around line 86-91: The subprocess.run invocation calling npx (using npx_path
and SKILLS_REGISTRY_PACKAGE) can hang or run a local malicious bin because npx
inherits project context and prompts; modify the call to (1) add the
non-interactive flag '--yes' to the args list, (2) pass stdin=subprocess.DEVNULL
to prevent waiting for user input, and (3) run with a sanitized environment/cwd
by copying os.environ, removing any project node_modules/.bin entries from PATH
(or set cwd to an empty temp dir) before passing env=..., so the command
resolves the registry package instead of local bins and cannot block on prompts
while still respecting the existing timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a399cd4-20d1-4f45-991c-756a099be77a
📒 Files selected for processing (4)
src/cli/BUILDsrc/cli/agent_skill.pysrc/cli/login.pysrc/cli/main_parser.py
src/cli/agent_skill.py
Outdated
| return | ||
|
|
||
| agent_names = ", ".join(a.name for a in agents) | ||
| print(f"\n AI Agent Integration\n") |
There was a problem hiding this comment.
Drop the empty f-string.
Ruff will flag this as F541.
🧰 Tools
🪛 Ruff (0.15.9)
[error] 177-177: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/agent_skill.py` at line 177, Remove the unnecessary f-string in the
print call that currently reads print(f"\n AI Agent Integration\n") to avoid
the F541 lint error; replace it with a plain string literal (e.g., print("\n
AI Agent Integration\n")). Update the print statement where it appears in
agent_skill.py so no f-prefix is used.
src/cli/agent_skill.py
Outdated
Comment on lines
+218
to
+231
| if not args.force: | ||
| agents_needing_install = [a for a in agents if not is_skill_installed(a)] | ||
| if not agents_needing_install: | ||
| print("OSMO agent skill is already installed in all detected agents.") | ||
| return | ||
| else: | ||
| uninstall_skill(agents) | ||
| agents_needing_install = agents | ||
|
|
||
| npx_path = find_npx() | ||
| if npx_path is None: | ||
| print("npx not found. Install Node.js or run manually:") | ||
| print(" npm install -g skills && skills add nvidia/osmo") | ||
| return |
There was a problem hiding this comment.
Preflight before tearing down a working install.
With --force, Line 224 removes the existing skill before Lines 227-231 confirm that npx is available. If that check fails, the command leaves a previously working installation removed. Move the npx preflight ahead of the uninstall, and ideally only tear down once the replacement install succeeds.
🛡️ Minimal fix
+ npx_path = find_npx()
+ if npx_path is None:
+ print("npx not found. Install Node.js or run manually:")
+ print(" npm install -g skills && skills add nvidia/osmo")
+ return
+
if not args.force:
agents_needing_install = [a for a in agents if not is_skill_installed(a)]
if not agents_needing_install:
print("OSMO agent skill is already installed in all detected agents.")
return
else:
uninstall_skill(agents)
agents_needing_install = agents
-
- npx_path = find_npx()
- if npx_path is None:
- print("npx not found. Install Node.js or run manually:")
- print(" npm install -g skills && skills add nvidia/osmo")
- return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not args.force: | |
| agents_needing_install = [a for a in agents if not is_skill_installed(a)] | |
| if not agents_needing_install: | |
| print("OSMO agent skill is already installed in all detected agents.") | |
| return | |
| else: | |
| uninstall_skill(agents) | |
| agents_needing_install = agents | |
| npx_path = find_npx() | |
| if npx_path is None: | |
| print("npx not found. Install Node.js or run manually:") | |
| print(" npm install -g skills && skills add nvidia/osmo") | |
| return | |
| npx_path = find_npx() | |
| if npx_path is None: | |
| print("npx not found. Install Node.js or run manually:") | |
| print(" npm install -g skills && skills add nvidia/osmo") | |
| return | |
| if not args.force: | |
| agents_needing_install = [a for a in agents if not is_skill_installed(a)] | |
| if not agents_needing_install: | |
| print("OSMO agent skill is already installed in all detected agents.") | |
| return | |
| else: | |
| uninstall_skill(agents) | |
| agents_needing_install = agents |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/agent_skill.py` around lines 218 - 231, The uninstall runs before
verifying npx is available, so using --force can remove a working install and
then fail; change the flow in the CLI handler (check args.force,
uninstall_skill, find_npx, agents_needing_install) so that find_npx() is called
and npx_path validated before calling uninstall_skill(agents), and only call
uninstall_skill after a successful install attempt (or at minimum after
confirming npx is present); update logic around args.force and
agents_needing_install to perform the preflight npx check first and defer/unlink
uninstall_skill until the replacement install is known to succeed.
This was referenced Apr 6, 2026
jiaenren
commented
Apr 6, 2026
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/805/index.html |
jiaenren
had a problem deploying
to
testbot-respond
GitHub Actions
Failure
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
c9d71cb to
6a17a41
Compare
svc-osmo-ci
had a problem deploying
to
internal-ci
GitHub Actions
Failure
svc-osmo-ci
had a problem deploying
to
internal-ci
GitHub Actions
Failure
svc-osmo-ci
had a problem deploying
to
testbot-respond
GitHub Actions
Error
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
5aeed78 to
7d11ff2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/cli/agent_skill_installer.py (2)
1-17:⚠️ Potential issue | 🟡 MinorMove the pylint waiver out of the docstring.
# pylint: disable=line-too-longinside the module string is just text, so the SPDX header line is still lintable. Put the waiver on its own comment line before the docstring.As per coding guidelines, "`**/*.py`: If copyright lines exceed 100 characters, add `# pylint: disable=line-too-long` comment instead of breaking into multiple lines."Suggested fix
-""" -SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long +# pylint: disable=line-too-long +""" +SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 1 - 17, The module-level pylint waiver is currently inside the top docstring and treated as text; remove `# pylint: disable=line-too-long` from inside the triple-quoted string and add it as its own comment line immediately above the module docstring so the linter sees the waiver for the file-level SPDX header and the docstring remains unchanged.
203-207:⚠️ Potential issue | 🟠 MajorDon't let
npxprompt again or resolve a project-localskillsbinary.npm documents that
npxprompts before installing missing packages unless--yesis passed, and that local bins stay on the executed process PATH. Running this from the caller's current directory can therefore either stop for a second confirmation or pick up a project-localskillscommand instead of the registry package. Please add--yesand run from a neutral cwd, withstdin=subprocess.DEVNULLas a guard. (docs.npmjs.com)Suggested fix
result = subprocess.run( - [npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE], + [npx_path, "--yes", "skills", "add", SKILLS_REGISTRY_PACKAGE], timeout=timeout, check=False, + stdin=subprocess.DEVNULL, + cwd=Path.home(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 203 - 207, The subprocess.run call that invokes npx ([npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE]) can prompt or pick up a project-local binary; update that subprocess invocation to include the "--yes" flag in the args, set cwd to a neutral directory (e.g., tempfile.gettempdir()) to avoid the caller's CWD affecting PATH resolution, and add stdin=subprocess.DEVNULL to prevent npx from prompting; keep existing timeout and check settings and reference the same npx_path and SKILLS_REGISTRY_PACKAGE variables when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 223-226: The early-skip condition currently treats "skill already
installed in any agent" as sufficient to suppress prompting; change that check
to require that every detected agent has the skill (use all(...) over the
detected-agents installed check) so a new agent without the skill still triggers
the prompt, and apply the same fix to the second identical check around the
256-258 block; update the function docstring and add a unit test that covers the
mixed-state case (one agent installed, one not) to ensure the prompt appears
only when not all agents have the skill.
In `@src/cli/login.py`:
- Around line 108-111: The optional skill prompt currently wraps
agent_skill_installer.prompt_skill_installation() in a broad except Exception
which does not catch KeyboardInterrupt; add an explicit except KeyboardInterrupt
(or except KeyboardInterrupt: logger.debug(..., exc_info=True) / pass)
immediately before the existing broad except to allow Ctrl+C to cancel the
prompt gracefully without causing the CLI to exit non-zero, keeping the existing
logger.debug for other Exceptions.
In `@src/cli/packaging/linux/installer.sh`:
- Around line 141-145: The installer currently swallows failures from the
command "npx skills add nvidia/osmo" via "|| true", which can mislead users;
remove the "|| true", capture the exit status of the "npx skills add
nvidia/osmo" invocation in the Linux installer.sh (the block containing the
command and the subsequent echo lines), and branch on that status: on success
print the existing removal instruction, on failure print a clear
retry/diagnostic message (e.g., instructing to retry the add, check
network/registry, or run the command manually) but do not abort the overall
installer; apply the identical change to the macOS postinstall copy so both
platforms behave the same.
In `@src/cli/packaging/macos/postinstall`:
- Around line 56-91: The interactive prompt currently always runs; change the
block that uses INSTALL_SKILL and runs "npx skills add nvidia/osmo" to first
gate on the same three conditions used in the Python flow: 1) detect supported
agent directories exist (check for the same agent directory patterns used
elsewhere in the repo), 2) verify the skill is not already installed (e.g., run
a non-failing check like "npx skills list" or attempt to detect the osmo skill
before prompting), and 3) ensure the persisted decline marker is not present
(use the same decline marker file name/path the Python flow uses). Only prompt
and run "npx skills add nvidia/osmo" when all three checks pass; otherwise skip
prompting. Apply the identical change to the Linux installer copy so both remain
in sync.
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 1-17: Move the pylint waiver out of the module docstring in
src/cli/tests/test_agent_skill_installer.py by adding a standalone comment line
"# pylint: disable=line-too-long" immediately before the triple-quoted module
docstring so the SPDX header remains inside the docstring but the pylint
directive is effective; do not keep the waiver inside the string literal and
ensure no other string lines are modified.
---
Duplicate comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 1-17: The module-level pylint waiver is currently inside the top
docstring and treated as text; remove `# pylint: disable=line-too-long` from
inside the triple-quoted string and add it as its own comment line immediately
above the module docstring so the linter sees the waiver for the file-level SPDX
header and the docstring remains unchanged.
- Around line 203-207: The subprocess.run call that invokes npx ([npx_path,
"skills", "add", SKILLS_REGISTRY_PACKAGE]) can prompt or pick up a project-local
binary; update that subprocess invocation to include the "--yes" flag in the
args, set cwd to a neutral directory (e.g., tempfile.gettempdir()) to avoid the
caller's CWD affecting PATH resolution, and add stdin=subprocess.DEVNULL to
prevent npx from prompting; keep existing timeout and check settings and
reference the same npx_path and SKILLS_REGISTRY_PACKAGE variables when making
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e1d64a0-a407-4ac9-a2c5-9720bdd907b0
📒 Files selected for processing (7)
src/cli/BUILDsrc/cli/agent_skill_installer.pysrc/cli/login.pysrc/cli/packaging/linux/installer.shsrc/cli/packaging/macos/postinstallsrc/cli/tests/BUILDsrc/cli/tests/test_agent_skill_installer.py
✅ Files skipped from review due to trivial changes (1)
- src/cli/tests/BUILD
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/BUILD
Comment on lines
+223
to
+226
| """Prompt user to install the osmo-agent skill for detected AI coding agents. | ||
|
|
||
| Silently returns if: not a TTY, no agents detected, skill already | ||
| installed in any agent, or user previously chose "Don't ask again". |
There was a problem hiding this comment.
Skip only when every detected agent already has the skill.
Right now one installed agent suppresses the prompt for all detected agents. If someone installed the skill for Claude earlier and later adds Codex, this block will never offer to install for Codex. This should be all(...), and the docstring/tests should cover the mixed-state case.
Suggested fix
- Silently returns if: not a TTY, no agents detected, skill already
- installed in any agent, or user previously chose "Don't ask again".
+ Silently returns if: not a TTY, no agents detected, skill already
+ installed in every detected agent, or user previously chose "Don't ask again".
@@
- if any(is_skill_installed(a) for a in agents):
+ if all(is_skill_installed(agent) for agent in agents):
logger.debug("Skipping agent skill prompt: skill already installed")
returnAlso applies to: 256-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/agent_skill_installer.py` around lines 223 - 226, The early-skip
condition currently treats "skill already installed in any agent" as sufficient
to suppress prompting; change that check to require that every detected agent
has the skill (use all(...) over the detected-agents installed check) so a new
agent without the skill still triggers the prompt, and apply the same fix to the
second identical check around the 256-258 block; update the function docstring
and add a unit test that covers the mixed-state case (one agent installed, one
not) to ensure the prompt appears only when not all agents have the skill.
Comment on lines
+56
to
+91
| if [ -t 0 ]; then | ||
| echo "" | ||
| echo "######################################################################" | ||
| echo "OSMO Agent Skill" | ||
| echo "" | ||
| echo "Install the OSMO agent skill for AI coding agents" | ||
| echo "(Claude Code, Codex, etc.) to enable natural language" | ||
| echo "commands for managing compute resources and workflows." | ||
| echo "######################################################################" | ||
| echo "" | ||
|
|
||
| read -p "Install the OSMO agent skill? [Y/n]: " INSTALL_SKILL | ||
| INSTALL_SKILL="${INSTALL_SKILL:-y}" | ||
| if [ "$INSTALL_SKILL" = "y" ] || [ "$INSTALL_SKILL" = "Y" ]; then | ||
| if command -v npx &> /dev/null; then | ||
| npx skills add nvidia/osmo || true | ||
| echo "" | ||
| echo " To remove the agent skill: npx skills remove osmo-agent" | ||
| echo "" | ||
| else | ||
| echo "" | ||
| echo " npx not found. Install Node.js to continue:" | ||
| echo "" | ||
| if command -v brew &> /dev/null; then | ||
| echo " brew install node" | ||
| else | ||
| echo " https://nodejs.org/en/download" | ||
| fi | ||
| echo "" | ||
| echo " Then run:" | ||
| echo "" | ||
| echo " npx skills add nvidia/osmo" | ||
| echo "" | ||
| fi | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Honor the same agent/decline gating in this installer prompt.
This block currently runs on every interactive install. It never checks whether any supported agent directories were detected, whether the skill is already installed, or whether the persisted decline marker already exists, so upgrades will keep re-prompting users who already installed the skill or explicitly opted out in the login flow. Please gate this path on the same conditions as the Python flow, and mirror the change in the Linux copy that is supposed to stay in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/packaging/macos/postinstall` around lines 56 - 91, The interactive
prompt currently always runs; change the block that uses INSTALL_SKILL and runs
"npx skills add nvidia/osmo" to first gate on the same three conditions used in
the Python flow: 1) detect supported agent directories exist (check for the same
agent directory patterns used elsewhere in the repo), 2) verify the skill is not
already installed (e.g., run a non-failing check like "npx skills list" or
attempt to detect the osmo skill before prompting), and 3) ensure the persisted
decline marker is not present (use the same decline marker file name/path the
Python flow uses). Only prompt and run "npx skills add nvidia/osmo" when all
three checks pass; otherwise skip prompting. Apply the identical change to the
Linux installer copy so both remain in sync.
Comment on lines
+1
to
+17
| """ | ||
| SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
|
|
||
| SPDX-License-Identifier: Apache-2.0 | ||
| """ |
There was a problem hiding this comment.
Move the pylint waiver out of the docstring here too.
This has the same problem as the production module: # pylint: disable=line-too-long is inside the string literal, so it does not suppress the SPDX header line. Put it on its own comment line before the docstring.
Suggested fix
-"""
-SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long
+# pylint: disable=line-too-long
+"""
+SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """ | |
| SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long | |
| Licensed under the Apache License, Version 2.0 (the "License"); | |
| you may not use this file except in compliance with the License. | |
| You may obtain a copy of the License at | |
| http://www.apache.org/licenses/LICENSE-2.0 | |
| Unless required by applicable law or agreed to in writing, software | |
| distributed under the License is distributed on an "AS IS" BASIS, | |
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| See the License for the specific language governing permissions and | |
| limitations under the License. | |
| SPDX-License-Identifier: Apache-2.0 | |
| """ | |
| # pylint: disable=line-too-long | |
| """ | |
| SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| Licensed under the Apache License, Version 2.0 (the "License"); | |
| you may not use this file except in compliance with the License. | |
| You may obtain a copy of the License at | |
| http://www.apache.org/licenses/LICENSE-2.0 | |
| Unless required by applicable law or agreed to in writing, software | |
| distributed under the License is distributed on an "AS IS" BASIS, | |
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| See the License for the specific language governing permissions and | |
| limitations under the License. | |
| SPDX-License-Identifier: Apache-2.0 | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/tests/test_agent_skill_installer.py` around lines 1 - 17, Move the
pylint waiver out of the module docstring in
src/cli/tests/test_agent_skill_installer.py by adding a standalone comment line
"# pylint: disable=line-too-long" immediately before the triple-quoted module
docstring so the SPDX header remains inside the docstring but the pylint
directive is effective; do not keep the waiver inside the string literal and
ensure no other string lines are modified.
7d11ff2 to
ab3c33f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/cli/agent_skill_installer.py (3)
222-258:⚠️ Potential issue | 🟠 MajorSkill installation check should use
all()instead ofany().Line 256 uses
any(is_skill_installed(a) for a in agents)which skips the prompt if any agent has the skill. This means if a user installed the skill for Claude and later adds Codex, the prompt will never appear for Codex. The check should useall()to only skip when every detected agent has the skill installed.The docstring at lines 225-226 should also be updated to reflect this behavior.
🔧 Suggested fix
"""Prompt user to install the osmo-agent skill for detected AI coding agents. Silently returns if: not a TTY, no agents detected, skill already - installed in any agent, or user previously chose "Don't ask again". + installed in every detected agent, or user previously chose "Don't ask again". """ ... - if any(is_skill_installed(a) for a in agents): + if all(is_skill_installed(agent) for agent in agents): logger.debug("Skipping agent skill prompt: skill already installed") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 222 - 258, The prompt_skill_installation function currently skips showing the install prompt if any detected agent already has the skill; change the check to use all(...) instead of any(...) so it only returns when every detected agent has the skill (update the line that reads any(is_skill_installed(a) for a in agents) to all(...)). Also update the function docstring to state the prompt silently returns when the skill is already installed in every detected agent. Ensure references to is_skill_installed and prompt_skill_installation remain intact.
1-17:⚠️ Potential issue | 🟡 MinorThe
pylint: disablecomment inside the docstring may not be effective.The
# pylint: disable=line-too-longon line 2 is placed inside the docstring, which means it's part of the string literal rather than a Python comment. Pylint may not recognize it. Move it to a standalone comment line before the docstring.🔧 Suggested fix
+# pylint: disable=line-too-long """ -SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long +SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License");As per coding guidelines, "
**/*.py: If copyright lines exceed 100 characters, add# pylint: disable=line-too-longcomment instead of breaking into multiple lines."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 1 - 17, The pylint disable directive is inside the module docstring and thus won't be parsed; move the "# pylint: disable=line-too-long" out of the triple-quoted string and place it as a standalone comment immediately above the docstring in this module (refer to the module-level docstring at the top of src/cli/agent_skill_installer.py) so pylint recognizes it while preserving the current copyright text.
200-214:⚠️ Potential issue | 🟠 Major
npxsubprocess call may hang on interactive prompts and inherit project context.The
subprocess.runcall doesn't include--yesto auto-confirm package installation, doesn't setcwdto avoid localnode_modulesshadowing, and doesn't redirect stdin. This can cause the process to hang waiting for user confirmation or execute unintended local binaries.🔧 Suggested fix
def install_skill_via_registry(npx_path: str, timeout: int = 120) -> bool: """Install osmo-agent skill via skills.sh registry interactively. Returns True on success.""" try: result = subprocess.run( - [npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE], + [npx_path, "--yes", "skills", "add", SKILLS_REGISTRY_PACKAGE], + stdin=subprocess.DEVNULL, + cwd=Path.home(), timeout=timeout, check=False, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 200 - 214, The install_skill_via_registry function's subprocess.run call can hang or pick up local binaries; modify the call in install_skill_via_registry to pass the non-interactive flag ("--yes") to the npx args, set cwd to a neutral directory (e.g., os.path.abspath(os.sep) or a temp dir) to avoid local node_modules shadowing, and redirect stdin to subprocess.DEVNULL so prompts can't block; keep timeout handling and existing exception catches intact.src/cli/tests/test_agent_skill_installer.py (1)
1-17:⚠️ Potential issue | 🟡 MinorMove
pylint: disablecomment outside the docstring.Same issue as the production module - the pylint directive inside the docstring literal won't be recognized by pylint.
🔧 Suggested fix
+# pylint: disable=line-too-long """ -SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long +SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.As per coding guidelines, "
**/*.py: If copyright lines exceed 100 characters, add# pylint: disable=line-too-longcomment instead of breaking into multiple lines."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/tests/test_agent_skill_installer.py` around lines 1 - 17, The module-level docstring currently contains the "# pylint: disable=line-too-long" directive which pylint will not recognize; remove that directive from inside the triple-quoted docstring and add it as a standalone comment immediately after the closing triple quotes (i.e., place "# pylint: disable=line-too-long" at module scope right below the docstring) so the linter will honor it for this file.
🧹 Nitpick comments (2)
src/cli/agent_skill_installer.py (1)
288-292: UseSKILLS_REGISTRY_PACKAGEconstant in failure message for consistency.Line 292 hardcodes
"nvidia/osmo"while line 290 correctly uses theSKILL_PACKAGE_NAMEconstant. Use the constant for maintainability.♻️ Suggested fix
else: - print("\nInstallation failed. Try manually: npx skills add nvidia/osmo\n") + print(f"\nInstallation failed. Try manually: npx skills add {SKILLS_REGISTRY_PACKAGE}\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 288 - 292, The failure message is hardcoded to "nvidia/osmo"; update the else branch in the install flow (the install_skill_via_registry call handling) to use the SKILLS_REGISTRY_PACKAGE constant instead of the literal string so messages remain consistent with SKILL_PACKAGE_NAME usage; locate the print in the same block that checks install_skill_via_registry and replace the hardcoded package name with SKILLS_REGISTRY_PACKAGE.src/cli/tests/test_agent_skill_installer.py (1)
433-445: Test doesn't verify theany()vsall()behavior for partial installations.This test mocks
is_skill_installedto returnTruefor all agents, which passes with eitherany()orall(). Consider adding a test case for the mixed state (one agent installed, one not) to verify the intended behavior.🧪 Suggested additional test
def test_shows_prompt_when_skill_installed_in_only_some_agents(self): """prompt_skill_installation should show prompt when skill is NOT in all agents.""" agent1 = agent_skill.AgentDirectory( name="Agent1", config_directory=Path("/test1"), skill_directory=Path("/test1/skills")) agent2 = agent_skill.AgentDirectory( name="Agent2", config_directory=Path("/test2"), skill_directory=Path("/test2/skills")) def selective_install(agent): return agent.name == "Agent1" # Only first agent has skill with mock.patch("src.cli.agent_skill_installer.is_interactive_terminal", return_value=True), \ mock.patch("src.cli.agent_skill_installer._is_prompt_declined", return_value=False), \ mock.patch("src.cli.agent_skill_installer.detect_agents", return_value=[agent1, agent2]), \ mock.patch("src.cli.agent_skill_installer.is_skill_installed", side_effect=selective_install), \ mock.patch("src.cli.agent_skill_installer._prompt_install_choice", return_value=agent_skill.INSTALL_NOT_NOW) as mock_choice, \ mock.patch("builtins.print"): agent_skill.prompt_skill_installation() mock_choice.assert_called_once() # Prompt should have been shown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/tests/test_agent_skill_installer.py` around lines 433 - 445, Add a test that verifies prompt_skill_installation shows the prompt when the skill is installed on only some agents: create two AgentDirectory instances (e.g., Agent1 and Agent2), mock detect_agents to return both, mock is_skill_installed with a side_effect that returns True for one agent and False for the other, patch is_interactive_terminal and _is_prompt_declined as in the existing test, patch _prompt_install_choice to return agent_skill.INSTALL_NOT_NOW and assert that _prompt_install_choice (or the prompt call) is invoked exactly once; this ensures prompt_skill_installation uses the intended any()/all() logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 670-687: The test contains a local import "from src.cli import
login as login_module" inside
TestLoginKeyboardInterrupt.test_keyboard_interrupt_caught_gracefully; move that
import to the module top with the other imports (e.g., add "from src.cli import
login as login_module" after the module docstring/import block) and remove the
in-test import, keeping the test using login_module and its _login function
unchanged (TestLoginKeyboardInterrupt and
test_keyboard_interrupt_caught_gracefully should reference login_module and call
login_module._login as before).
---
Duplicate comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 222-258: The prompt_skill_installation function currently skips
showing the install prompt if any detected agent already has the skill; change
the check to use all(...) instead of any(...) so it only returns when every
detected agent has the skill (update the line that reads
any(is_skill_installed(a) for a in agents) to all(...)). Also update the
function docstring to state the prompt silently returns when the skill is
already installed in every detected agent. Ensure references to
is_skill_installed and prompt_skill_installation remain intact.
- Around line 1-17: The pylint disable directive is inside the module docstring
and thus won't be parsed; move the "# pylint: disable=line-too-long" out of the
triple-quoted string and place it as a standalone comment immediately above the
docstring in this module (refer to the module-level docstring at the top of
src/cli/agent_skill_installer.py) so pylint recognizes it while preserving the
current copyright text.
- Around line 200-214: The install_skill_via_registry function's subprocess.run
call can hang or pick up local binaries; modify the call in
install_skill_via_registry to pass the non-interactive flag ("--yes") to the npx
args, set cwd to a neutral directory (e.g., os.path.abspath(os.sep) or a temp
dir) to avoid local node_modules shadowing, and redirect stdin to
subprocess.DEVNULL so prompts can't block; keep timeout handling and existing
exception catches intact.
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 1-17: The module-level docstring currently contains the "# pylint:
disable=line-too-long" directive which pylint will not recognize; remove that
directive from inside the triple-quoted docstring and add it as a standalone
comment immediately after the closing triple quotes (i.e., place "# pylint:
disable=line-too-long" at module scope right below the docstring) so the linter
will honor it for this file.
---
Nitpick comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 288-292: The failure message is hardcoded to "nvidia/osmo"; update
the else branch in the install flow (the install_skill_via_registry call
handling) to use the SKILLS_REGISTRY_PACKAGE constant instead of the literal
string so messages remain consistent with SKILL_PACKAGE_NAME usage; locate the
print in the same block that checks install_skill_via_registry and replace the
hardcoded package name with SKILLS_REGISTRY_PACKAGE.
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 433-445: Add a test that verifies prompt_skill_installation shows
the prompt when the skill is installed on only some agents: create two
AgentDirectory instances (e.g., Agent1 and Agent2), mock detect_agents to return
both, mock is_skill_installed with a side_effect that returns True for one agent
and False for the other, patch is_interactive_terminal and _is_prompt_declined
as in the existing test, patch _prompt_install_choice to return
agent_skill.INSTALL_NOT_NOW and assert that _prompt_install_choice (or the
prompt call) is invoked exactly once; this ensures prompt_skill_installation
uses the intended any()/all() logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7291684a-5704-4474-a8b3-309fabb33c63
📒 Files selected for processing (7)
src/cli/BUILDsrc/cli/agent_skill_installer.pysrc/cli/login.pysrc/cli/packaging/linux/installer.shsrc/cli/packaging/macos/postinstallsrc/cli/tests/BUILDsrc/cli/tests/test_agent_skill_installer.py
✅ Files skipped from review due to trivial changes (2)
- src/cli/tests/BUILD
- src/cli/BUILD
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/packaging/macos/postinstall
ecolternv
reviewed
Apr 8, 2026
| @@ -0,0 +1,825 @@ | |||
| """ | |||
| SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long | |||
src/cli/agent_skill_installer.py
Outdated
| @@ -0,0 +1,292 @@ | |||
| """ | |||
| SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long | |||
| if args.method == 'code': | ||
| service_client.login_manager.device_code_login(url, args.device_endpoint) | ||
| try: | ||
| agent_skill_installer.prompt_skill_installation() |
Contributor
There was a problem hiding this comment.
It would be nice to have a standalone command for this incase I decided I did want to install the skills, even If I had previously said "Dont ask me again".
osmo skills install
osmo skills uninstall
| @@ -0,0 +1,292 @@ | |||
| """ | |||
Contributor
There was a problem hiding this comment.
Why do we need both this file, and installer.sh/postinstall? it seems like they do the same thing.
Can we just have installer/postinstall call this file? LIke after they install the osmo cli, they can then just run osmo skills install or something
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
ab3c33f to
3925265
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/cli/agent_skill_installer.py (3)
1-3:⚠️ Potential issue | 🟡 MinorMove the pylint directive outside the docstring.
Line 2 embeds
# pylint: disable=line-too-longinside the string literal, so it won’t be applied by pylint.Proposed fix
+# pylint: disable=line-too-long """ -SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.As per coding guidelines, "
**/*.py: If copyright lines exceed 100 characters, add# pylint: disable=line-too-longcomment instead of breaking into multiple lines."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 1 - 3, The pylint directive is currently inside the module docstring and therefore ignored; move "# pylint: disable=line-too-long" out of the docstring so pylint will honor it — i.e., end the triple-quoted module docstring as shown, then add a standalone module comment line containing "# pylint: disable=line-too-long" immediately after the closing quotes (affecting the module), ensuring no other code is changed.
225-227:⚠️ Potential issue | 🟠 MajorPrompt suppression should require all detected agents to already have the skill.
Line 256 uses
any(...), which skips prompting if just one detected agent is installed. That prevents prompting for newly detected agents still missing the skill.Proposed fix
- Silently returns if: not a TTY, no agents detected, skill already - installed in any agent, or user previously chose "Don't ask again". + Silently returns if: not a TTY, no agents detected, skill already + installed in all detected agents, or user previously chose "Don't ask again". @@ - if any(is_skill_installed(a) for a in agents): + if all(is_skill_installed(agent) for agent in agents): logger.debug("Skipping agent skill prompt: skill already installed") returnAlso applies to: 256-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 225 - 227, The prompt suppression currently uses any(...) which returns true if any detected agent already has the skill and thus skips prompting; change that check to require all detected agents to already have the skill by replacing any(...) with all(...) wherever the "skill already installed" condition is evaluated (e.g., the any(...) call around detected_agents/installed checks at lines ~256-258 in agent_skill_installer.py), and apply the same all(...) logic to the "Don't ask again" suppression branch so the installer only suppresses the prompt when every detected agent already has the skill.
203-207:⚠️ Potential issue | 🟠 MajorHarden the
npxsubprocess call to avoid prompt blocking/context hijack.Line 204 currently invokes
npxwithout--yes, without stdin isolation, and in the caller’s CWD. This can block on npm confirmation prompts or resolve command context from an unintended project directory.Proposed fix
result = subprocess.run( - [npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE], + [npx_path, "--yes", "skills", "add", SKILLS_REGISTRY_PACKAGE], timeout=timeout, check=False, + stdin=subprocess.DEVNULL, + cwd=Path.home(), )According to official npm/npx docs, does omitting `--yes` in `npx <cmd>` allow interactive confirmation prompts, and can execution resolve from local project context/binaries?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 203 - 207, Update the subprocess.run call that invokes npx (the call using npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE) to make it non-interactive and CWD-safe: add the "--yes" flag to the argument list, set stdin=subprocess.DEVNULL to prevent interactive prompts from hijacking the caller, and pass an explicit neutral cwd (e.g., the user home or root directory) so resolution won't use the caller's current project directory; keep timeout and check=False as before.src/cli/tests/test_agent_skill_installer.py (1)
1-3:⚠️ Potential issue | 🟡 MinorThe pylint line-length waiver is still inside the docstring.
Line 2 keeps the directive in the string literal, so pylint won’t apply it.
Proposed fix
+# pylint: disable=line-too-long """ -SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # pylint: disable=line-too-long +SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.As per coding guidelines, "
**/*.py: If copyright lines exceed 100 characters, add# pylint: disable=line-too-longcomment instead of breaking into multiple lines."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/tests/test_agent_skill_installer.py` around lines 1 - 3, Move the pylint directive out of the module docstring: remove the "# pylint: disable=line-too-long" token from inside the top-level triple-quoted string (the SPDX copyright line) and place it as a standalone comment on the following line (or immediately above the docstring) so pylint sees it as a file-level directive rather than text in the module docstring.src/cli/packaging/macos/postinstall (1)
56-97:⚠️ Potential issue | 🟠 MajorThis installer prompt still ignores existing install/decline state.
Lines 56-97 always prompt in interactive sessions; they don’t mirror the Python gating (agent detected, not already installed, and not previously declined). This will re-prompt users on upgrades even after opting out or already installing.
🧹 Nitpick comments (1)
src/cli/tests/test_agent_skill_installer.py (1)
304-308: Strengthen subprocess assertions to lock in the safer npx invocation contract.These assertions currently only check command + timeout +
check=False. Once the install call is hardened, tests should also assert non-interactive/safe execution args (e.g.,--yes,stdin=subprocess.DEVNULL, and controlledcwd) to prevent regressions.Also applies to: 344-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/tests/test_agent_skill_installer.py` around lines 304 - 308, The current mock_run.assert_called_once_with checks only the command, timeout and check flags; update the assertions for mock_run (the calls that install agent_skill.SKILLS_REGISTRY_PACKAGE) to enforce the safer npx invocation contract by including the non-interactive flag "--yes" in the command list, and by asserting stdin is subprocess.DEVNULL and cwd is the expected working directory (and keep timeout=120, check=False). Apply the same strengthened assertion pattern for both occurrences that currently live near the mock_run assertions (the one referencing agent_skill.SKILLS_REGISTRY_PACKAGE at lines ~304-308 and the similar one at ~344-348).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 1-3: The pylint directive is currently inside the module docstring
and therefore ignored; move "# pylint: disable=line-too-long" out of the
docstring so pylint will honor it — i.e., end the triple-quoted module docstring
as shown, then add a standalone module comment line containing "# pylint:
disable=line-too-long" immediately after the closing quotes (affecting the
module), ensuring no other code is changed.
- Around line 225-227: The prompt suppression currently uses any(...) which
returns true if any detected agent already has the skill and thus skips
prompting; change that check to require all detected agents to already have the
skill by replacing any(...) with all(...) wherever the "skill already installed"
condition is evaluated (e.g., the any(...) call around detected_agents/installed
checks at lines ~256-258 in agent_skill_installer.py), and apply the same
all(...) logic to the "Don't ask again" suppression branch so the installer only
suppresses the prompt when every detected agent already has the skill.
- Around line 203-207: Update the subprocess.run call that invokes npx (the call
using npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE) to make it
non-interactive and CWD-safe: add the "--yes" flag to the argument list, set
stdin=subprocess.DEVNULL to prevent interactive prompts from hijacking the
caller, and pass an explicit neutral cwd (e.g., the user home or root directory)
so resolution won't use the caller's current project directory; keep timeout and
check=False as before.
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 1-3: Move the pylint directive out of the module docstring: remove
the "# pylint: disable=line-too-long" token from inside the top-level
triple-quoted string (the SPDX copyright line) and place it as a standalone
comment on the following line (or immediately above the docstring) so pylint
sees it as a file-level directive rather than text in the module docstring.
---
Nitpick comments:
In `@src/cli/tests/test_agent_skill_installer.py`:
- Around line 304-308: The current mock_run.assert_called_once_with checks only
the command, timeout and check flags; update the assertions for mock_run (the
calls that install agent_skill.SKILLS_REGISTRY_PACKAGE) to enforce the safer npx
invocation contract by including the non-interactive flag "--yes" in the command
list, and by asserting stdin is subprocess.DEVNULL and cwd is the expected
working directory (and keep timeout=120, check=False). Apply the same
strengthened assertion pattern for both occurrences that currently live near the
mock_run assertions (the one referencing agent_skill.SKILLS_REGISTRY_PACKAGE at
lines ~304-308 and the similar one at ~344-348).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa712e42-7bfc-4445-bf3a-41dad36fecc1
📒 Files selected for processing (7)
src/cli/BUILDsrc/cli/agent_skill_installer.pysrc/cli/login.pysrc/cli/packaging/linux/installer.shsrc/cli/packaging/macos/postinstallsrc/cli/tests/BUILDsrc/cli/tests/test_agent_skill_installer.py
✅ Files skipped from review due to trivial changes (2)
- src/cli/BUILD
- src/cli/tests/BUILD
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
3925265 to
9879fc9
Compare
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
9879fc9 to
ccaa1a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/cli/agent_skill_installer.py (2)
223-259:⚠️ Potential issue | 🟠 MajorSkip prompt only when every detected agent has the skill installed.
The current
any()check at line 257 suppresses the prompt if the skill is installed for any detected agent. This means if a user installed the skill for Claude Code earlier and later adds Codex, the prompt will never offer to install for Codex. The logic should useall()instead.🔧 Proposed fix
"""Prompt user to install the osmo-agent skill for detected AI coding agents. Silently returns if: not a TTY, no agents detected, skill already - installed in any agent, or user previously chose "Don't ask again". + installed in every detected agent, or user previously chose "Don't ask again". """- if any(is_skill_installed(a) for a in agents): + if all(is_skill_installed(agent) for agent in agents): logger.debug("Skipping agent skill prompt: skill already installed") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 223 - 259, The prompt_skill_installation function currently skips prompting if any detected agent already has the skill; change that logic to only skip when all detected agents have the skill. Specifically, in prompt_skill_installation replace the any(is_skill_installed(a) for a in agents) check with all(is_skill_installed(a) for a in agents) so the prompt will still run when at least one detected agent is missing the skill.
201-215:⚠️ Potential issue | 🟠 MajorAdd
--yesflag and isolation to preventnpxfrom hanging or running local bins.The
subprocess.runcall tonpxis missing the--yesflag, which can cause it to hang waiting for user confirmation when installing missing packages (since there's no TTY passthrough). Additionally, withoutcwd=Path.home(), a malicious localnode_modules/.bin/skillscould be executed instead of the registry package.🔧 Proposed fix
def install_skill_via_registry(npx_path: str, timeout: int = 120) -> bool: """Install osmo-agent skill via skills.sh registry interactively. Returns True on success.""" try: result = subprocess.run( - [npx_path, "skills", "add", SKILLS_REGISTRY_PACKAGE], + [npx_path, "--yes", "skills", "add", SKILLS_REGISTRY_PACKAGE], timeout=timeout, check=False, + cwd=Path.home(), ) if result.returncode != 0: logger.warning("skills add failed with exit code %d", result.returncode) return False return True except (OSError, subprocess.TimeoutExpired) as error: logger.warning("Failed to run npx skills add: %s", error) return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/agent_skill_installer.py` around lines 201 - 215, install_skill_via_registry currently calls npx without the --yes flag and from the current working directory, which can hang waiting for confirmation and risk running a local node_modules/.bin/skills; update the subprocess.run invocation in install_skill_via_registry to include the "--yes" argument in the argument list and set cwd=Path.home() (import Path from pathlib if not already) so npx runs non-interactively and from the user's home directory to avoid executing local bins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 341-353: The uninstall block in _uninstall_command uses
subprocess.run([...]) without the --yes flag or cwd isolation, which can hang or
use a shadowed local npx; update the subprocess.run call that invokes [npx_path,
"skills", "remove", SKILL_PACKAGE_NAME] to include the "--yes" argument (e.g.
[npx_path, "skills", "remove", "--yes", SKILL_PACKAGE_NAME]) and pass
cwd=Path.home() while keeping timeout=120 and check=False so the remove is
non-interactive and executed from the home directory.
---
Duplicate comments:
In `@src/cli/agent_skill_installer.py`:
- Around line 223-259: The prompt_skill_installation function currently skips
prompting if any detected agent already has the skill; change that logic to only
skip when all detected agents have the skill. Specifically, in
prompt_skill_installation replace the any(is_skill_installed(a) for a in agents)
check with all(is_skill_installed(a) for a in agents) so the prompt will still
run when at least one detected agent is missing the skill.
- Around line 201-215: install_skill_via_registry currently calls npx without
the --yes flag and from the current working directory, which can hang waiting
for confirmation and risk running a local node_modules/.bin/skills; update the
subprocess.run invocation in install_skill_via_registry to include the "--yes"
argument in the argument list and set cwd=Path.home() (import Path from pathlib
if not already) so npx runs non-interactively and from the user's home directory
to avoid executing local bins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70bce84d-f08b-45d7-b660-d2b778c349cf
📒 Files selected for processing (8)
src/cli/BUILDsrc/cli/agent_skill_installer.pysrc/cli/login.pysrc/cli/main_parser.pysrc/cli/packaging/linux/installer.shsrc/cli/packaging/macos/postinstallsrc/cli/tests/BUILDsrc/cli/tests/test_agent_skill_installer.py
✅ Files skipped from review due to trivial changes (4)
- src/cli/packaging/macos/postinstall
- src/cli/main_parser.py
- src/cli/tests/BUILD
- src/cli/tests/test_agent_skill_installer.py
src/cli/agent_skill_installer.py
Outdated
Comment on lines
+341
to
+353
| try: | ||
| result = subprocess.run( | ||
| [npx_path, "skills", "remove", SKILL_PACKAGE_NAME], | ||
| timeout=120, | ||
| check=False, | ||
| ) | ||
| if result.returncode == 0: | ||
| print("\nAgent skill removed.\n") | ||
| else: | ||
| print(f"\nRemoval failed. Try manually: npx skills remove {SKILL_PACKAGE_NAME}\n") | ||
| except (OSError, subprocess.TimeoutExpired) as error: | ||
| logger.warning("Failed to run npx skills remove: %s", error) | ||
| print(f"\nRemoval failed. Try manually: npx skills remove {SKILL_PACKAGE_NAME}\n") |
There was a problem hiding this comment.
Apply the same --yes and cwd isolation to the uninstall command.
For consistency and safety, the subprocess.run call in _uninstall_command should also include --yes and cwd=Path.home() to prevent hanging or local bin shadowing.
🔧 Proposed fix
try:
result = subprocess.run(
- [npx_path, "skills", "remove", SKILL_PACKAGE_NAME],
+ [npx_path, "--yes", "skills", "remove", SKILL_PACKAGE_NAME],
timeout=120,
check=False,
+ cwd=Path.home(),
)🧰 Tools
🪛 Ruff (0.15.9)
[error] 342-342: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/agent_skill_installer.py` around lines 341 - 353, The uninstall block
in _uninstall_command uses subprocess.run([...]) without the --yes flag or cwd
isolation, which can hang or use a shadowed local npx; update the subprocess.run
call that invokes [npx_path, "skills", "remove", SKILL_PACKAGE_NAME] to include
the "--yes" argument (e.g. [npx_path, "skills", "remove", "--yes",
SKILL_PACKAGE_NAME]) and pass cwd=Path.home() while keeping timeout=120 and
check=False so the remove is non-interactive and executed from the home
directory.
ccaa1a1 to
1b185db
Compare
Detect AI coding agents (Claude Code, Codex, Agent Skills) and offer to install the OSMO agent skill via `npx skills add nvidia/osmo`. Commands: - `osmo skills install` — direct install (supports -y, -g, --agent, --copy) - `osmo skills install --prompt` — 3-choice menu (Yes / Not now / Don't ask again) - `osmo skills uninstall` — remove skill (supports -y, -g, --agent) Auto-prompt after device-code login (`osmo login`) with detection guards: TTY check, agent detection, skill-installed check, decline marker check. Post-install scripts (macOS/Linux) delegate to `osmo skills install --prompt`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
83 tests covering: agent detection, skill status (global + project scope), npx discovery, decline marker persistence, Node.js install instructions, npx command execution, TTY detection, interactive menu fallback, cursor restore, setup_parser, install/uninstall commands with flag forwarding, KeyboardInterrupt handling, and 19 integration scenarios for the full prompt lifecycle including CLI-login cross-interaction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jiaenren
force-pushed
the
jiaenr/agent-skill-detection
branch
from
1b185db to
50d94f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add AI agent skill detection and installation to the OSMO CLI via
npx skills add nvidia/osmo.Commands:
osmo skills install— direct installosmo skills install --prompt— 3-choice menu (Yes / Not now / Don't ask again)osmo skills uninstall— remove skillFlags (pass-through to
npx skills):-y, --yes— skip npx confirmation prompts-g, --global— install/remove globally instead of project-level-a, --agent <agents>— target specific agents (e.g.claude-code cursor)--copy— copy files instead of symlinking (install only)Auto-prompt after login:
osmo login)Post-install integration:
.pkgpostinstall and Linux installer callosmo skills install --promptOther details:
~/.config/osmo/; "Not now" re-prompts next login~/.claude/skills/) and project-scope pathsbrew install nodeif brew available, otherwise links to nodejs.org)--log-level debugFiles:
agent_skill_installer.py— new module (detection, prompt, npx wrapper, subcommand)login.py— callsprompt_skill_installation()after device-code loginmain_parser.py— registersskillssubcommandpostinstall/installer.sh— simplified toosmo skills install --prompttest_agent_skill_installer.py— 83 unit + integration testsIssue - None
Checklist
Tests
osmo login - npx available
osmo login - npx not available
Post install with npx
Post install without npx
Summary by CodeRabbit