fix(skills): detect installed built-in catalog entries - #3656
Conversation
📝 WalkthroughWalkthroughThis PR fixes installed-skill detection for built-in catalog entries in the skills explorer and changes workflow URL installs to succeed as a no-op when the target skill is already present, with matching frontend and Rust test updates. Build scripts are updated to discover cargo and rustc bin paths dynamically instead of assuming they are on PATH. ChangesSkills install parity
Build toolchain resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Sequence DiagramssequenceDiagram
participant CatalogRegistry
participant SkillsExplorer
participant InstallKeyUtils
participant InstalledWorkflows
CatalogRegistry->>SkillsExplorer: catalog entries
InstalledWorkflows->>SkillsExplorer: installed workflows
SkillsExplorer->>InstallKeyUtils: catalogInstallKeys(entry)
SkillsExplorer->>InstallKeyUtils: workflowInstallKeys(workflow)
InstallKeyUtils->>SkillsExplorer: normalized keys (Set)
SkillsExplorer->>InstallKeyUtils: isCatalogEntryInstalled(entry, keys)
InstallKeyUtils->>SkillsExplorer: boolean (installed state)
SkillsExplorer->>CatalogRegistry: render tile with installed state
sequenceDiagram
participant Client
participant install_workflow_from_url
participant install_workflow_from_url_with_home
participant FileSystem
participant WorkflowDiscovery
Client->>install_workflow_from_url: install request
install_workflow_from_url->>install_workflow_from_url_with_home: workspace_dir, params, home
install_workflow_from_url_with_home->>WorkflowDiscovery: discover before (with home)
install_workflow_from_url_with_home->>FileSystem: check target directory
alt SKILL.md exists
FileSystem-->>install_workflow_from_url_with_home: file found
install_workflow_from_url_with_home-->>install_workflow_from_url: Ok(already installed, no new skills)
else no SKILL.md but dir exists
FileSystem-->>install_workflow_from_url_with_home: file not found
install_workflow_from_url_with_home-->>install_workflow_from_url: Err(collision)
else dir does not exist
FileSystem-->>install_workflow_from_url_with_home: not found
install_workflow_from_url_with_home->>WorkflowDiscovery: discover after (with home)
install_workflow_from_url_with_home-->>install_workflow_from_url: Ok(new skills)
end
install_workflow_from_url-->>Client: outcome
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 433962edc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function workflowInstallKeys(skill: WorkflowSummary): string[] { | ||
| return [ | ||
| slugifyInstallKey(skill.id), | ||
| slugifyInstallKey(skill.name), |
There was a problem hiding this comment.
Including skill.name in installedKeys means an unrelated local or project skill with the same frontmatter display name as a catalog entry is treated as installed even when its on-disk slug/id is different. For example, a user-created skill with id: apple-notes-copy and name: Apple Notes will hide the Install action for built-in/apple-notes, though the registry installer would create ~/.openhuman/skills/apple-notes; compare only canonical slugs/path-derived keys or otherwise verify the directory slug.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/openhuman/workflows/ops_install.rs (1)
271-296: ⚡ Quick winUpdate install Rustdoc to reflect idempotent existing-directory behavior.
The runtime now returns success when
<slug>/SKILL.mdalready exists, but the function contract text above still states collision is fatal/no silent overwrite. Aligning docs with behavior will prevent caller/test assumptions from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/workflows/ops_install.rs` around lines 271 - 296, The Rustdoc comments for the install_workflow_from_url function need to be updated to reflect the idempotent behavior implemented in the code. Currently, the documentation describes collision as fatal with no silent overwrite, but the actual implementation now returns success (Ok outcome) when the target directory already exists with a valid SKILL.md file. Update the function's documentation comments to describe this idempotent behavior: that the function safely handles the case where the skill is already installed by returning success with appropriate messaging, rather than treating it as a fatal error condition.app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx (1)
305-335: ⚡ Quick winAdd one assertion for dialog-level installed parity in this built-in case.
This test validates tile state, but not the updated detail-dialog installed gating path. After opening the tile, assert the dialog does not render an
Installaction for the already-installed built-in entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx` around lines 305 - 335, The test for the "Installed" badge only validates tile-level state but does not check the detail dialog. After the existing assertions that verify the Installed badge is shown in the tile and the install button is not present in the tile, add code to click or open the tile to display the detail dialog, then add an assertion to verify that the Install action is also not rendered in the dialog for the already-installed built-in entry. You can use the same pattern as the existing tile assertions, checking that within the dialog context, the registry-install-built-in/apple-notes testId is not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/skills/SkillsExplorerTab.tsx`:
- Around line 51-66: The install-key derivation in SkillsExplorerTab’s
catalogInstallKeys and workflowInstallKeys is too broad because it uses
entry.name and skill.name, which can cause unrelated items with the same display
name to be treated as already installed. Remove the name-based keys and keep
only stable identifiers sourced from entry.id, lastPathSegment(entry.id),
parentPathSegment(entry.docs_path), parentPathSegment(entry.download_url), and
skill.id / parentPathSegment(skill.location), so install-state matching relies
on unique, stable slug/id fields only.
---
Nitpick comments:
In `@app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx`:
- Around line 305-335: The test for the "Installed" badge only validates
tile-level state but does not check the detail dialog. After the existing
assertions that verify the Installed badge is shown in the tile and the install
button is not present in the tile, add code to click or open the tile to display
the detail dialog, then add an assertion to verify that the Install action is
also not rendered in the dialog for the already-installed built-in entry. You
can use the same pattern as the existing tile assertions, checking that within
the dialog context, the registry-install-built-in/apple-notes testId is not
present.
In `@src/openhuman/workflows/ops_install.rs`:
- Around line 271-296: The Rustdoc comments for the install_workflow_from_url
function need to be updated to reflect the idempotent behavior implemented in
the code. Currently, the documentation describes collision as fatal with no
silent overwrite, but the actual implementation now returns success (Ok outcome)
when the target directory already exists with a valid SKILL.md file. Update the
function's documentation comments to describe this idempotent behavior: that the
function safely handles the case where the skill is already installed by
returning success with appropriate messaging, rather than treating it as a fatal
error condition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89891647-1656-41d7-a499-69c7125b821c
📒 Files selected for processing (4)
app/src/components/skills/SkillsExplorerTab.tsxapp/src/components/skills/__tests__/SkillsExplorerTab.test.tsxsrc/openhuman/workflows/ops_install.rssrc/openhuman/workflows/ops_tests.rs
|
Updated in fc0cbaa: removed display-name based installed matching, added a same-name/different-slug regression test, added dialog-level installed gating coverage, and updated install Rustdoc for idempotent already-installed behavior.\n\nValidation: cargo fmt --check passed; git diff --check passed. Local Vitest could not run because node_modules/vitest are not installed and this machine is on Node 22 while the app requires Node >=24. |
|
I opened a helper PR with fixes for the current CI failures: YonganZhang#2 What it changes:
Validation run locally:
I also attempted the two targeted Rust tests locally, but both timed out during Windows Rust compilation after 240s, so CI is the authoritative verification for those. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e4526df6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return [ | ||
| slugifyInstallKey(entry.id), | ||
| slugifyInstallKey(lastPathSegment(entry.id)), | ||
| slugifyInstallKey(parentPathSegment(entry.docs_path)), |
There was a problem hiding this comment.
For Hermes entries, docs_path is shaped like bundled/<category>/<category>-<skill> (the parser test for apple-notes uses bundled/apple/apple-apple-notes), so parentPathSegment(entry.docs_path) adds only the category (apple, devops, etc.) to the catalog's install keys. If a user has an installed/local workflow whose slug is one of those categories, every registry entry in that category is treated as installed and the Install action/detail button is hidden even though the specific skill is absent; derive the skill slug from the last docsPath segment (stripping the category prefix) or avoid this key.
Useful? React with 👍 / 👎.
coderabbitai
Bot
added
the
agent
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a48b1e79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| slugifyInstallKey(entry.id), | ||
| slugifyInstallKey(lastPathSegment(entry.id)), | ||
| slugifyInstallKey(parentPathSegment(entry.docs_path)), | ||
| slugifyInstallKey(parentPathSegment(entry.download_url)), |
There was a problem hiding this comment.
For catalog entries whose download_url is not guaranteed to be <slug>/SKILL.md, this adds unrelated path components as install keys. For example, download_url_from_source_url can turn a GitHub root blob into https://raw.githubusercontent.com/org/repo/main/SKILL.md (src/openhuman/skill_registry/ops.rs:570-575), so parentPathSegment(entry.download_url) becomes main; any user with an unrelated installed skill slug main will see that catalog entry as Installed and lose both install buttons. Restrict this key to URL shapes known to encode the skill slug, or avoid matching on the download URL parent.
Useful? React with 👍 / 👎.
# Conflicts: # app/scripts/e2e-web-build.sh # scripts/test-rust-e2e.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test-rust-e2e.sh`:
- Around line 141-144: The CARGO_BIN variable is initialized with a fallback
mechanism at lines 131-135, but there is no validation to ensure it is actually
set and executable if all discovery methods fail. Add an explicit error check
immediately after line 135 that verifies CARGO_BIN is not empty and is
executable, and if the check fails, exit with a clear error message that helps
users understand that cargo/rustc discovery failed. This will prevent confusing
errors when CARGO_BIN is later used in the bash invocations at lines 141 and
144.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3b89f58-9018-442c-beab-4e1ad77e1807
📒 Files selected for processing (3)
app/.prettierignoreapp/scripts/e2e-web-build.shscripts/test-rust-e2e.sh
✅ Files skipped from review due to trivial changes (1)
- app/.prettierignore
🚧 Files skipped from review as they are similar to previous changes (1)
- app/scripts/e2e-web-build.sh
| bash "$SCRIPT_DIR/ci-cancel-aware.sh" "$CARGO_BIN" test --manifest-path Cargo.toml --test "$suite" -- "${EXTRA_ARGS[@]}" | ||
| else | ||
| echo "[rust-e2e] cargo test --manifest-path Cargo.toml --test $suite" | ||
| "$SCRIPT_DIR/ci-cancel-aware.sh" cargo test --manifest-path Cargo.toml --test "$suite" | ||
| echo "[rust-e2e] $CARGO_BIN test --manifest-path Cargo.toml --test $suite" | ||
| bash "$SCRIPT_DIR/ci-cancel-aware.sh" "$CARGO_BIN" test --manifest-path Cargo.toml --test "$suite" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify CARGO_BIN initialization and error handling in the E2E test script
# Show the CARGO_BIN resolution logic (lines 131-144)
echo "=== CARGO_BIN resolution logic ==="
sed -n '131,144p' scripts/test-rust-e2e.sh
echo ""
echo "=== Checking for SCRIPT_DIR definition ==="
rg -n 'SCRIPT_DIR=' scripts/test-rust-e2e.sh | head -5
echo ""
echo "=== Checking for error handling when cargo/rustc not found ==="
rg -n -A2 -B2 'CARGO_BIN.*command -v|rustc.*not found|cargo.*not found' scripts/test-rust-e2e.shRepository: tinyhumansai/openhuman
Length of output: 1122
Add error handling if cargo/rustc discovery fails.
CARGO_BIN is properly initialized with a multi-step fallback (lines 131-135): env var override → rustc directory → command -v cargo, and SCRIPT_DIR is correctly defined at line 26. However, if both rustc and cargo discovery fail, CARGO_BIN could be empty when passed to the cargo invocations (lines 141, 144), resulting in a confusing error message. Add an explicit check after line 135 to fail with a clear error message if CARGO_BIN is not set or executable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-rust-e2e.sh` around lines 141 - 144, The CARGO_BIN variable is
initialized with a fallback mechanism at lines 131-135, but there is no
validation to ensure it is actually set and executable if all discovery methods
fail. Add an explicit error check immediately after line 135 that verifies
CARGO_BIN is not empty and is executable, and if the check fails, exit with a
clear error message that helps users understand that cargo/rustc discovery
failed. This will prevent confusing errors when CARGO_BIN is later used in the
bash invocations at lines 141 and 144.
3 participants
Summary
Closes #3585.
Tests
Not run
Summary by CodeRabbit
Release Notes
Bug Fixes
SKILL.mdalready exists, avoiding unnecessary failures and overwrites.Tests
Chores
rustc/cargobefore building and running tests.