OSAC-3957: Vendor osac-ai-skills from tools/bootstrap.sh - #307
OSAC-3957: Vendor osac-ai-skills from tools/bootstrap.sh#307tchughesiv wants to merge 13 commits into
Conversation
Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
install.sh creates real .*/skills directories; the fan-out umbrella must exist first so those writes land in skills/ through the symlinks. Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
@tchughesiv: This pull request references OSAC-3957 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe bootstrap process now provisions ChangesAI skills integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The stale-link removal behavior is covered by the smoke test, so no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Developer
participant Bootstrap as tools/bootstrap.sh
participant Wrapper as tools/link-agent-skills.sh
participant Skills as osac-ai-skills
Developer->>Bootstrap: run bootstrap
Bootstrap->>Skills: validate, clone, or update checkout
Bootstrap->>Wrapper: link agent skills
Wrapper->>Skills: resolve skills and fan-out script
Wrapper->>Wrapper: create and prune symlinks
Wrapper->>Skills: delegate fan-out command
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 9:12 PM UTC · Completed 9:29 PM UTC Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tools/bootstrap.sh`:
- Around line 43-53: Use one vendor validity contract across tools/bootstrap.sh
lines 43-53 and tools/link-agent-skills.sh lines 31-40: require the
project-local or home vendor directory to meet the same validity criteria,
including .git, before selecting or updating it. In bootstrap, validate the
project-local .osac-ai-skills directory before any git fetch/rebase and provide
recovery instructions when invalid; update the link-agent-skills selection logic
to apply the identical rule, or consistently support non-Git trees in both
locations.
In `@tools/test/link-agent-skills-consumer-smoke.sh`:
- Around line 10-26: Make link-agent-skills-consumer-smoke.sh self-contained by
adding minimal local fan-out fixture setup, or invoking tools/bootstrap.sh
before candidate discovery and fixture checks. Ensure a clean checkout can
execute the smoke test without prior bootstrap, while preserving the existing
WRAPPER and PROJECT_ROOT-capable fan-out validations.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3bfb205-458e-49fb-beab-4a49bf60da07
📒 Files selected for processing (5)
.gitignoreAGENTS.mdtools/bootstrap.shtools/link-agent-skills.shtools/test/link-agent-skills-consumer-smoke.sh
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Labels: PR modifies AGENTS.md documentation and adds developer tooling scripts Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Labels: PR modifies developer tooling scripts (tools/bootstrap.sh, tools/link-agent-skills.sh) and governance documentation (AGENTS.md) |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
Auto-dismissed: only Prow labels gate merging
Gate project-local .osac-ai-skills with osac_ai_skills_vendor_ok before git update, and embed a minimal fan-out stub so the consumer smoke runs on a clean checkout without a prior bootstrap. Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
Addressed bot review feedback in 9f497b4:
Left as-is / deferred:
|
fullsend-ai-review
Bot
added
the
documentation
|
🤖 Review · Commit: |
REPO_ROOT (this wrapper's own name, osac-workspace's reference wrapper calls it WORKSPACE_ROOT) and the exported PROJECT_ROOT are the same directory under two names. Add a comment explaining why: PROJECT_ROOT is a fixed cross-repo contract name the vendored osac-ai-skills fan-out expects (OSAC-3956), not a stylistic choice, so it isn't renamed to match REPO_ROOT. Addresses fullsend-ai-review naming-convention finding (Round 4) on OSAC-3957 / osac-project#307. Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 11:02 PM UTC · Completed 11:18 PM UTC Commit: |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: tchughesiv The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Pull request was converted to draft
Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
🤖 Finished Review · ✅ Success · Started 12:12 AM UTC · Completed 12:27 AM UTC Commit: |
| @@ -21,34 +80,31 @@ AI_WORKFLOWS_DIR="" | |||
| if [[ -d "${HOME}/.ai-workflows" ]]; then | |||
There was a problem hiding this comment.
[low] error-handling-gap
The ai-workflows resolution block checks only [[ -d "${HOME}/.ai-workflows" ]] before calling update_git_repo, without validating it is a usable git checkout. If ~/.ai-workflows is an empty or non-git directory, git fetch fails with a confusing 'Fetch failed' message. This is a pre-existing pattern (not introduced by this PR) but remains a gap compared to the osac_ai_skills_vendor_ok() gate used for the new vendor.
Suggested fix: Add a validation check (e.g., [[ -d "${HOME}/.ai-workflows/.git" ]]) before calling update_git_repo on the ai-workflows directory.
|
|
||
| # Fetch + rebase a vendored git checkout onto origin/main. Warns and continues | ||
| # on failure rather than exiting — a stale vendor is recoverable manually, and | ||
| # this runs before any skill-discovery step that depends on it. |
There was a problem hiding this comment.
[low] edge-case
Path resolution asymmetry: ~/.osac-ai-skills is resolved via readlink -f (line 39), but ${PROJECT_ROOT}/.osac-ai-skills is used directly (line 42). Since PROJECT_ROOT is already canonicalized via realpath, the practical impact is minimal.
| # | ||
| # When $OSAC_AI_SKILLS_VENDOR_DIR is set, it is authoritative and is not | ||
| # re-validated against the two-candidate search below: bootstrap.sh sets it | ||
| # to the exact directory it already resolved (or just cloned), specifically |
There was a problem hiding this comment.
[low] fail-open
resolve_osac_ai_skills_dir() validates OSAC_AI_SKILLS_VENDOR_DIR only for structural presence (skills/ directory and executable tools/link-agent-skills.sh), then execs that script at line 156. Consistent with the pre-existing trust model where install.sh is already executed from unverified clones.
|
|
||
| ARGS=("$@") | ||
| if [[ ${#ARGS[@]} -eq 0 ]]; then | ||
| ARGS=(--all --with-ai-workflows) |
There was a problem hiding this comment.
[low] error-handling-idiom
resolve_osac_ai_skills_dir returns its result via stdout (captured by command substitution), while sibling bootstrap.sh sets variables directly. The stdout-as-return-value pattern is idiomatic shell but diverges from the sibling script's convention.
Backports two hardening fixes discovered during osac-project/osac#307's review (the mono-repo's independent port of this same vendoring pattern) that this reordering fix's original scope didn't cover: - Gate the project-local ./.osac-ai-skills branch with the same osac_ai_skills_vendor_ok() check already used for ~/.osac-ai-skills, so a stale or non-git leftover directory fails with an actionable message instead of a confusing `git fetch` error. - Export OSAC_AI_SKILLS_VENDOR_DIR from bootstrap.sh into tools/link-agent-skills.sh so the wrapper uses the exact vendor directory bootstrap.sh already resolved/updated/cloned, instead of independently re-resolving one via its own weaker, HOME-first search -- which could otherwise silently link skills from a stale vendor bootstrap.sh had already rejected. Adds a regression test (test_vendor_override_env_var_is_authoritative) covering both the override-wins-over-a-different-valid-vendor case and the invalid-override-fails-loudly case. Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
… clone (#213) * fix(OSAC-4039): reorder bootstrap.sh skill-linking to survive a fresh clone ai-workflows' install.sh creates .claude/skills (and .cursor/skills, .gemini/skills) as real directories via mkdir -p when nothing exists yet, then link-agent-skills.sh --all refuses to replace a real directory with the ../skills umbrella symlink -- aborting the whole bootstrap under set -euo pipefail. Never bites on an already- bootstrapped clone (install.sh's mkdir -p is a no-op against an existing symlinked directory), only a from-scratch one. Run link-agent-skills.sh --all before install.sh so the umbrella symlinks exist first; install.sh's targeted ln -sfn calls into .claude/skills/<workflow> then transparently follow the symlink. Verified end-to-end on a fresh clone (zero errors/warnings, --verify passes). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com> * fix(OSAC-4039): harden osac-ai-skills vendor resolution in bootstrap.sh Backports two hardening fixes discovered during osac-project/osac#307's review (the mono-repo's independent port of this same vendoring pattern) that this reordering fix's original scope didn't cover: - Gate the project-local ./.osac-ai-skills branch with the same osac_ai_skills_vendor_ok() check already used for ~/.osac-ai-skills, so a stale or non-git leftover directory fails with an actionable message instead of a confusing `git fetch` error. - Export OSAC_AI_SKILLS_VENDOR_DIR from bootstrap.sh into tools/link-agent-skills.sh so the wrapper uses the exact vendor directory bootstrap.sh already resolved/updated/cloned, instead of independently re-resolving one via its own weaker, HOME-first search -- which could otherwise silently link skills from a stale vendor bootstrap.sh had already rejected. Adds a regression test (test_vendor_override_env_var_is_authoritative) covering both the override-wins-over-a-different-valid-vendor case and the invalid-override-fails-loudly case. Assisted-by: Cursor <noreply@cursor.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com> --------- Signed-off-by: Tommy Hughes <tohughes@redhat.com>
|
Follow-up flag from OSAC-4006 review (osac-workspace#214):
-dir=$(printf '%s' "$input" | jq -r '.workspace.project_dir');
+dir=$(printf '%s' "$input" | jq -er '.workspace.project_dir | strings | select(length > 0)');Fixed in |
2 participants
OSAC-3957: osac/tools/bootstrap.sh vendors osac-ai-skills and runs fan-out
Jira: https://redhat.atlassian.net/browse/OSAC-3957
Story type: Task
ADR: 0001 Decision item 2(a)
Summary
Extend
osac/tools/bootstrap.shso a freshosac/clone vendorsosac-ai-skills, materializes OSAC skills, and links Claude/Cursor/Gemini discovery dirs without a second checkout ofosac-workspaceor a manual skills clone. Mirrors the OSAC-3956 consumer pattern (osac-workspace#208,osac-ai-skills#2).Changes
tools/link-agent-skills.sh— consumer wrapper: resolve vendoredosac-ai-skills, materialize per-skill absolute symlinks intoskills/, exec vendored fan-out withPROJECT_ROOTtools/test/link-agent-skills-consumer-smoke.sh— bash smoke for missing vendor, three-harness links, refuse real dirs, prune stale linkstools/bootstrap.sh— vendorosac-ai-skills(prefer usable~/.osac-ai-skills), then link, thenai-workflowsinstall.sh(link-before-install required on fresh clones).gitignore— ignore.osac-ai-skills/AGENTS.md— document AI bootstrap / skill authorship inosac-ai-skillsOut of scope: declarative sibling clones /
enhancement-proposals(OSAC-3958), pinned-copy for bots (OSAC-3959).Testing
bash tools/test/link-agent-skills-consumer-smoke.sh(4 contracts)git worktree+ emptyHOMEfulltools/bootstrap.sh— three harnesses readcreate-pr;--verifypassesAcceptance Criteria
osac/+bootstrap.shproduces working skill symlinks for Claude, Cursor, and Geminiosac-ai-skills(orosac-workspace) is requiredSummary by CodeRabbit
New Features
Documentation
Tests
Chores