fix(verify): use scoped targetDir for Skills component paths#902
fix(verify): use scoped targetDir for Skills component paths#902decode2 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIn ChangesComponentSkills scope fix and tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 unit tests (beta)
Comment |
The ComponentSkills case in componentPathsWithWorkspaceScoped passed homeDir instead of the already-computed targetDir to skills.SkillPathForAgent. When --scope=workspace, the installer writes skills to the workspace directory but post-apply verification checked the global home directory, causing false failures. The install path at run.go:788 already uses targetDir correctly, so this aligns verify with install behavior. Also fixes TestComponentPathsOpenClawSkillsSkipsSDDPhaseSkills which passed an unused workspace dir but expected home paths — OpenClaw always uses workspaceDir when set, so the test now reflects that. Closes Gentleman-Programming#785
b0e43b3 to
2e130cc
Compare
Alan-TheGentleman
left a comment
There was a problem hiding this comment.
I’m requesting changes because the regression coverage stops at the internal path helper and does not assert the user-visible install --scope=workspace verification behavior from #785. Please add a focused test that runs the install path with workspace scope, no global skill files, and proves post-apply verification stays ready while checking workspace skill paths. Also run gofmt on the touched Go file before updating the PR.
…ntleman-Programming#785 Verifies that post-apply verification checks workspace paths (not home paths) when --scope=workspace is used and no global skill files exist. Also applies gofmt to internal/cli/run.go. Addresses review feedback from @Alan-TheGentleman
Summary
Fixes post-apply verification for \ComponentSkills\ when --scope=workspace\ is used. The verifier was checking skill paths in the global home directory instead of the workspace-scoped directory, causing 33 false failures.
Root Cause
In \componentPathsWithWorkspaceScoped\ (\internal/cli/run.go), the \ComponentSkills\ case passed \homeDir\ instead of the already-computed \ argetDir\ to \skills.SkillPathForAgent. Every other component case correctly uses \ argetDir.
The install path at line 788 already uses \ argetDir\ correctly:
\\go
targetDir := componentInjectionDirScoped(s.homeDir, s.workspaceDir, s.scope, adapter)
skills.Inject(targetDir, adapter, skillIDs)
\\
But the verify path used \homeDir:
\\go
path := skills.SkillPathForAgent(homeDir, adapter, skillID) // BUG
\\
Fix
One-line change: \homeDir\ → \ argetDir\ in the \ComponentSkills\ case.
Tests
Closes #785
Summary by CodeRabbit
Bug Fixes