#71: Add project symlink install walking skeleton#76
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements project-level symlink installation for Beislið skills. It adds ChangesProject-Level Symlink Install
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/install_lib.sh (1)
533-546: ⚡ Quick winThis status metric is counting links, not skills.
_count_project_skill_linksincrements once per host directory, so a healthy install reports3 * <skill count>ininstalled_skill_links. If that is intentional, the field/docs should say “link count”; otherwise dedupe per skill so project status matches the promised installed-skill count.Also applies to: 594-595
🤖 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/install_lib.sh` around lines 533 - 546, _count_project_skill_links currently increments for each host directory (dir) that contains a symlink, causing triple-counting; change it to count unique skills only by tracking seen skill names (e.g., a local associative array/set keyed by skill) so each skill basename (variable skill found from skill_dir) is counted once even if symlinked in multiple host dirs; update the loop using skill_dir/skill/expected to mark seen skills and only increment when a skill is first observed.
🤖 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/install_lib.sh`:
- Around line 489-497: The missing-workflow warning is skipped when
_ensure_project_metadata_dir sets manifest_active=0 for a file/symlink .beislid;
change the conditional that prints the note so it checks the actual presence of
"$project/.beislid/workflow.md" regardless of manifest_active. Concretely,
update the if that now reads `if [[ "$manifest_active" == 1 && ! -f
"$project/.beislid/workflow.md" ]]; then` to a check that only tests the absence
of the workflow file (e.g., `if [[ ! -f "$project/.beislid/workflow.md" ]];
then`) so the note about missing workflow is always emitted when the workflow
file is unavailable while leaving _ensure_project_metadata_dir and
manifest_active semantics elsewhere unchanged.
---
Nitpick comments:
In `@scripts/install_lib.sh`:
- Around line 533-546: _count_project_skill_links currently increments for each
host directory (dir) that contains a symlink, causing triple-counting; change it
to count unique skills only by tracking seen skill names (e.g., a local
associative array/set keyed by skill) so each skill basename (variable skill
found from skill_dir) is counted once even if symlinked in multiple host dirs;
update the loop using skill_dir/skill/expected to mark seen skills and only
increment when a skill is first observed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b8af616-0456-4676-b3c4-9830a3e6a5d8
📒 Files selected for processing (8)
.gitignoreREADME.mdbin/beisliddocs/configuration.mddocs/how-to-use.mdinstall.shscripts/install_lib.shscripts/test_install.sh
Owner
Author
|
Also fixed the status-count nit: project status now reports unique installed skills ( |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install_lib.sh (1)
557-557: 💤 Low valueConsider splitting declaration and assignment (SC2155).
Shellcheck flags this because
localmasks the return value of the command substitution. While_project_manifest_pathis just aprintfthat effectively can't fail, separating declaration and assignment is defensive against future refactoring.♻️ Proposed fix
+ local manifest + manifest="$(_project_manifest_path "$project")" - local manifest="$(_project_manifest_path "$project")"🤖 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/install_lib.sh` at line 557, ShellCheck SC2155 warns about combining `local` with command substitution; modify the code in the install script so you declare then assign `manifest` separately: replace `local manifest="$(_project_manifest_path "$project")"` with a two-step sequence `local manifest` followed by `manifest="$(_project_manifest_path "$project")"`, referencing the `_project_manifest_path` command substitution and the `manifest` variable to avoid masking the command's return value.
🤖 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.
Nitpick comments:
In `@scripts/install_lib.sh`:
- Line 557: ShellCheck SC2155 warns about combining `local` with command
substitution; modify the code in the install script so you declare then assign
`manifest` separately: replace `local manifest="$(_project_manifest_path
"$project")"` with a two-step sequence `local manifest` followed by
`manifest="$(_project_manifest_path "$project")"`, referencing the
`_project_manifest_path` command substitution and the `manifest` variable to
avoid masking the command's return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4b3c89f-e766-4f8d-9545-d13e773cd25b
📒 Files selected for processing (2)
scripts/install_lib.shscripts/test_install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/test_install.sh
This was referenced May 10, 2026
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
1 participant
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.
Summary
beislid install project [path]andbeislid status project [path]install.sh --project [path]compatibility routing.agents,.claude, and.codexskill dirs.beislid/project-install.jsonwith source/version/commit, targets, and countsSafety
--force.beislid/workflow.mdis missingVerification
bash -n install.sh bin/beislid scripts/install_lib.sh scripts/test_install.shpython3 scripts/validate_skills.pybash scripts/test_install.sh— 49 passed, 0 failedpython3 tests/agent-smoke/run.py gate ship-it --hosts claude,codex --timeout 900 --changed-only— passed on claude and codexCloses #71.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores