feat: add skill install/update command - #164
Open
rsonghuster wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds the ChangesSkill installation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Fc CLI
participant Fc as Fc
participant Skill as Skill
participant Installer as installer
participant FileSystem as File system
CLI->>Fc: invoke skill install or update
Fc->>Skill: dispatch command
Skill->>Installer: select tools and scopes
Installer->>FileSystem: resolve and copy bundled s-fc3 skill
FileSystem-->>Installer: return target status
Installer-->>Skill: return installation results
Skill-->>CLI: log summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.23.2)docs/CONTRIB.mdmarkdownlint-cli2 v0.23.2 (markdownlint v0.41.1) 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@__tests__/e2e/skill/run-windows.ps1`:
- Line 4: Update the test script’s native-command error handling around each s
invocation so nonzero exit codes terminate execution. Check $LASTEXITCODE after
every s command and throw on failure, or enable
$PSNativeCommandUseErrorActionPreference with an equivalent throwing wrapper;
preserve the existing stop-on-error behavior.
In `@__tests__/ut/commands/skill/skill.test.ts`:
- Line 40: Update the inputs helper in the skill tests so the args object uses
the formatter-compliant parenthesized cast form `({ args } as any)`, preserving
its existing behavior.
In `@docs/architecture.md`:
- Line 34: Rename the Mermaid node ID for “skill 技能安装” in the architecture
diagram so it no longer conflicts with the existing `V` node for “FC 函数计算,” then
update the edge at the affected connection to reference the new skill node ID
while preserving all other diagram relationships.
In `@package.json`:
- Around line 8-9: Update the prewatch and prebuild scripts to avoid fs.cpSync,
which is unsupported by the declared Node.js 16 runtime, by using a Node
16-compatible recursive copy implementation; alternatively, update the project’s
declared Node.js version if Node 16 is no longer supported.
In `@src/index.ts`:
- Around line 209-213: Update the skill method to await
super.handlePreRun(inputs, false) before constructing Skill, preserving the
false argument so the command remains a local no-credential operation and uses
the shared Base preprocessing path.
In `@src/subCommands/skill/index.ts`:
- Around line 46-58: Update parseTools so the default all-tools behavior applies
only when raw is undefined, while an explicitly provided value such as "," is
validated after trimming and throws an error when no tool names remain. Preserve
existing unknown-tool validation and deduplication for non-empty requests.
In `@src/subCommands/skill/installer.ts`:
- Around line 75-78: Update the replacement flow around resolveSkillSource and
the target-copy logic to stage the source in a sibling temporary directory
before removing target. Ensure the staged copy completes successfully before
replacing target, clean up staging on success or failure, and handle source and
target resolving to the same directory without deleting the source.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8051f966-982c-4b0d-bbea-42db56ca527e
📒 Files selected for processing (20)
.agents/skills/s-fc3/README.md.agents/skills/s-fc3/SKILL.mdCLAUDE.md__tests__/e2e/ci-mac-linux.sh__tests__/e2e/ci-windows.ps1__tests__/e2e/ci.sh__tests__/e2e/skill/run__tests__/e2e/skill/run-windows.ps1__tests__/ut/commands/skill/skill.test.tsdocs/CONTRIB.mddocs/architecture.mdmakefilepackage.jsonpublish.yamlsrc/commands-help/index.tssrc/commands-help/skill.tssrc/index.tssrc/subCommands/skill/constants.tssrc/subCommands/skill/index.tssrc/subCommands/skill/installer.ts
| S[version 版本] | ||
| T[model 模型] | ||
| U[s2tos3 转换] | ||
| V[skill 技能安装] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a unique Mermaid node ID for the skill module.
Line 34 defines V for skill, but the existing resource section defines V for FC 函数计算. Mermaid uses the same node for both declarations. Line 54 therefore connects B to the wrong node. Rename the skill node and update its edge.
Proposed fix
- V[skill 技能安装]
+ VV[skill 技能安装]
...
- B --> V
+ B --> VVAlso applies to: 54-54
🤖 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 `@docs/architecture.md` at line 34, Rename the Mermaid node ID for “skill 技能安装”
in the architecture diagram so it no longer conflicts with the existing `V` node
for “FC 函数计算,” then update the edge at the affected connection to reference the
new skill node ID while preserving all other diagram relationships.
Comment on lines
+8
to
+9
| "prewatch": "node -e \"const fs=require('fs');fs.mkdirSync('dist',{recursive:true});fs.copyFileSync('src/schema.json','dist/schema.json');fs.cpSync('.agents/skills/s-fc3','dist/skills/s-fc3',{recursive:true})\"", | ||
| "prebuild": "node -e \"const fs=require('fs');fs.rmSync('dist',{recursive:true,force:true});fs.mkdirSync('dist',{recursive:true});fs.copyFileSync('src/schema.json','dist/schema.json');fs.cpSync('.agents/skills/s-fc3','dist/skills/s-fc3',{recursive:true})\"", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f 'package\.json|\.nvmrc|\.tool-versions|Dockerfile.*|.*\.(yml|yaml)$' . \
-x rg -n '"engines"|node-version|setup-node|FROM node:' {}Repository: devsapp/fc3
Length of output: 676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json =="
cat -n package.json | sed -n '1,120p'
echo
echo "== node fs.cpSync availability from locally installed Node.js, if present =="
node - <<'JS' | sed -n '1,80p'
console.log(process.version);
const fs = require('fs');
console.log(typeof fs.cpSync);
console.log('has cpSync:', Object.prototype.hasOwnProperty.call(fs, 'cpSync') || typeof fs.cpSync === 'function');
JS
echo
echo "== lockfiles =="
git ls-files | grep -Ei '(^|/)package-lock\.json$|(^|/)pnpm-lock\.yaml$|(^|/)yarn\.lock$|(^|/)(npm|pack)?age\.json$'Repository: devsapp/fc3
Length of output: 4582
Use a supported copy mechanism for Node.js 16.
fs.cpSync is not available in this project’s declared Node runtime; declare the actual supported Node.js version or replace fs.cpSync in prewatch and prebuild with an implementation compatible with Node 16.
🤖 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 `@package.json` around lines 8 - 9, Update the prewatch and prebuild scripts to
avoid fs.cpSync, which is unsupported by the declared Node.js 16 runtime, by
using a Node 16-compatible recursive copy implementation; alternatively, update
the project’s declared Node.js version if Node 16 is no longer supported.
Comment on lines
+209
to
+213
| public async skill(inputs: IInputs) { | ||
| const skill = new Skill(inputs); | ||
| logger.debug(`skill subCommand: ${skill.subCommand}`); | ||
| return await skill[skill.subCommand](); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run the shared pre-processing step.
Call await super.handlePreRun(inputs, false) before constructing Skill. The false value preserves the local no-credential operation while keeping the command on the common Base pre-run path.
Proposed fix
public async skill(inputs: IInputs) {
+ await super.handlePreRun(inputs, false);
const skill = new Skill(inputs);As per coding guidelines, src/**/*.ts must “share common preprocessing through base.ts.”
📝 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
| public async skill(inputs: IInputs) { | |
| const skill = new Skill(inputs); | |
| logger.debug(`skill subCommand: ${skill.subCommand}`); | |
| return await skill[skill.subCommand](); | |
| } | |
| public async skill(inputs: IInputs) { | |
| await super.handlePreRun(inputs, false); | |
| const skill = new Skill(inputs); | |
| logger.debug(`skill subCommand: ${skill.subCommand}`); | |
| return await skill[skill.subCommand](); | |
| } |
🤖 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/index.ts` around lines 209 - 213, Update the skill method to await
super.handlePreRun(inputs, false) before constructing Skill, preserving the
false argument so the command remains a local no-credential operation and uses
the shared Base preprocessing path.
Source: Coding guidelines
Comment on lines
+46
to
+58
| private parseTools(raw?: string): string[] { | ||
| if (_.isEmpty(raw)) { | ||
| return [...ALL_TOOLS]; | ||
| } | ||
| const requested = raw | ||
| .split(',') | ||
| .map((t) => t.trim().toLowerCase()) | ||
| .filter(Boolean); | ||
| const unknown = requested.filter((t) => !ALL_TOOLS.includes(t)); | ||
| if (unknown.length > 0) { | ||
| throw new Error(`Unknown tool(s): ${unknown.join(', ')}. Supported: ${ALL_TOOLS.join(', ')}`); | ||
| } | ||
| return _.uniq(requested); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an explicit empty tool list.
--tools , passes Line 47, then Lines 50-53 produce an empty list. run() performs no installation and reports success with zero writes.
Return all tools only when raw is undefined. Throw an error when an explicit value contains no tool name.
Proposed fix
- if (_.isEmpty(raw)) {
+ if (raw === undefined) {
return [...ALL_TOOLS];
}
const requested = raw
.split(',')
.map((t) => t.trim().toLowerCase())
.filter(Boolean);
+ if (requested.length === 0) {
+ throw new Error(`No tools specified. Supported: ${ALL_TOOLS.join(', ')}`);
+ }📝 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
| private parseTools(raw?: string): string[] { | |
| if (_.isEmpty(raw)) { | |
| return [...ALL_TOOLS]; | |
| } | |
| const requested = raw | |
| .split(',') | |
| .map((t) => t.trim().toLowerCase()) | |
| .filter(Boolean); | |
| const unknown = requested.filter((t) => !ALL_TOOLS.includes(t)); | |
| if (unknown.length > 0) { | |
| throw new Error(`Unknown tool(s): ${unknown.join(', ')}. Supported: ${ALL_TOOLS.join(', ')}`); | |
| } | |
| return _.uniq(requested); | |
| private parseTools(raw?: string): string[] { | |
| if (raw === undefined) { | |
| return [...ALL_TOOLS]; | |
| } | |
| const requested = raw | |
| .split(',') | |
| .map((t) => t.trim().toLowerCase()) | |
| .filter(Boolean); | |
| if (requested.length === 0) { | |
| throw new Error(`No tools specified. Supported: ${ALL_TOOLS.join(', ')}`); | |
| } | |
| const unknown = requested.filter((t) => !ALL_TOOLS.includes(t)); | |
| if (unknown.length > 0) { | |
| throw new Error(`Unknown tool(s): ${unknown.join(', ')}. Supported: ${ALL_TOOLS.join(', ')}`); | |
| } | |
| return _.uniq(requested); |
🤖 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/subCommands/skill/index.ts` around lines 46 - 58, Update parseTools so
the default all-tools behavior applies only when raw is undefined, while an
explicitly provided value such as "," is validated after trimming and throws an
error when no tool names remain. Preserve existing unknown-tool validation and
deduplication for non-empty requests.
Comment on lines
+75
to
+78
| // Replace the target atomically-ish: remove then copy, so stale files never linger. | ||
| fs.removeSync(target); | ||
| fs.ensureDirSync(path.dirname(target)); | ||
| fs.copySync(source, target, { dereference: true }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stage the replacement before removing the current target.
When --project --tools agents runs from the repository root, resolveSkillSource() can return the same .agents/skills/s-fc3 directory as target. Line 76 then deletes source, and Line 78 fails because the source no longer exists.
Copy to a sibling staging directory first. Replace the target only after the staged copy succeeds. This also preserves an existing installation when copySync fails.
Proposed replacement flow
- fs.removeSync(target);
fs.ensureDirSync(path.dirname(target));
- fs.copySync(source, target, { dereference: true });
+ const stagingRoot = fs.mkdtempSync(path.join(path.dirname(target), `.${SKILL_NAME}-`));
+ const stagedTarget = path.join(stagingRoot, SKILL_NAME);
+ try {
+ fs.copySync(source, stagedTarget, { dereference: true });
+ fs.removeSync(target);
+ fs.moveSync(stagedTarget, target);
+ } finally {
+ fs.removeSync(stagingRoot);
+ }📝 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
| // Replace the target atomically-ish: remove then copy, so stale files never linger. | |
| fs.removeSync(target); | |
| fs.ensureDirSync(path.dirname(target)); | |
| fs.copySync(source, target, { dereference: true }); | |
| // Replace the target atomically-ish: remove then copy, so stale files never linger. | |
| fs.ensureDirSync(path.dirname(target)); | |
| const stagingRoot = fs.mkdtempSync(path.join(path.dirname(target), `.${SKILL_NAME}-`)); | |
| const stagedTarget = path.join(stagingRoot, SKILL_NAME); | |
| try { | |
| fs.copySync(source, stagedTarget, { dereference: true }); | |
| fs.removeSync(target); | |
| fs.moveSync(stagedTarget, target); | |
| } finally { | |
| fs.removeSync(stagingRoot); | |
| } |
🤖 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/subCommands/skill/installer.ts` around lines 75 - 78, Update the
replacement flow around resolveSkillSource and the target-copy logic to stage
the source in a sibling temporary directory before removing target. Ensure the
staged copy completes successfully before replacing target, clean up staging on
success or failure, and handle source and target resolving to the same directory
without deleting the source.
Add a new `fc3 skill` subcommand that installs the bundled s-fc3 skill into mainstream agent tools (claude/codex/cursor/qoder/agents), at global (user home) or project scope. `install` skips existing targets unless `--force`; `update` always overwrites. Pure local operation, no cloud credentials required. - src/subCommands/skill: constants (tool→dir map), installer (source resolution + copy), index (arg parse + install/update dispatch) - register command in src/index.ts, commands-help, and publish.yaml - bundle .agents/skills/s-fc3 into dist/skills/s-fc3 via prebuild/prewatch - unit tests (14) + sandboxed offline E2E (bash + pwsh), wired into ci scripts and a `make test-skill` target - docs: architecture, CONTRIB, CLAUDE.md, skill README/SKILL.md Signed-off-by: ls147258 <ls147258@alibaba-inc.com>
- e2e: drive the local build via a minimal offline s.yaml + `s skill ...` instead of `s cli <abs-path> ...`. On Windows the CLI joins the absolute component path into its per-run log dir, which breaks when cwd (temp on C:) and the repo (D:) live on different drives, surfacing as "command cannot be found". Fixes windows-ci. - test: apply f2elint formatting to skill.test.ts. Fixes check-format. Signed-off-by: ls147258 <ls147258@alibaba-inc.com>
rsonghuster
force-pushed
the
feat/skill-install-command
branch
from
August 9, 2026 13:42
a510ad7 to
f539928
Compare
PowerShell parses `--tools claude,codex` as an array and passes it to the
native s exe joined by a space ("claude codex"), which parseTools rejected as
an unknown tool. Split --tools on commas OR whitespace, and quote the value in
run-windows.ps1. Adds a unit test for the whitespace form.
Signed-off-by: ls147258 <ls147258@alibaba-inc.com>
The finally block restored cwd to $fc3_dir (repo root) instead of the dir the script started in (`skill`). Since PowerShell's location is process-wide, this leaked to ci-windows.ps1, whose `cd ..` then landed one level too high and broke the subsequent `cd nodejs`.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
__tests__/e2e/skill/run-windows.ps1 (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an approved PowerShell verb for the helper.
PSScriptAnalyzer reports
Skill-PathunderPSUseApprovedVerbs. Rename it toGet-SkillPathand update all call sites.Proposed rename
-function Skill-Path($root, $tool) { +function Get-SkillPath($root, $tool) {🤖 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 `@__tests__/e2e/skill/run-windows.ps1` around lines 57 - 59, Rename the Skill-Path helper to Get-SkillPath to use an approved PowerShell verb, and update every call site to reference Get-SkillPath while preserving its existing path construction behavior.Source: Linters/SAST tools
🤖 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 `@__tests__/e2e/skill/run-windows.ps1`:
- Around line 27-38: Update Write-SYaml to escape apostrophes in fc3_dir by
replacing each single quote with two single quotes before interpolating it into
the single-quoted YAML component value. Preserve the existing path and YAML
structure for paths without apostrophes.
- Around line 88-92: Expand the --tools filter test near the existing codex
assertions to verify that every non-selected tool is absent: claude, cursor,
qoder, and agents. Retain the positive codex assertion and use the existing
Skill-Path, Assert-File, and Assert-Missing helpers.
In `@__tests__/ut/commands/skill/skill.test.ts`:
- Around line 101-105: The update tests currently verify only the reported
status, not replacement of target contents. In
__tests__/ut/commands/skill/skill.test.ts lines 101-105, add a stale marker to
the qoder project target after the initial install and assert that
installOne(..., 'update', ...) removes it; in lines 149-153, add the same
stale-marker setup to the cursor global target and assert that Skill.update()
removes it.
---
Nitpick comments:
In `@__tests__/e2e/skill/run-windows.ps1`:
- Around line 57-59: Rename the Skill-Path helper to Get-SkillPath to use an
approved PowerShell verb, and update every call site to reference Get-SkillPath
while preserving its existing path construction behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c960855c-e8e2-4e57-b2a7-d2d3a3503cd0
📒 Files selected for processing (5)
__tests__/e2e/skill/run__tests__/e2e/skill/run-windows.ps1__tests__/ut/commands/skill/skill.test.tsdocs/CONTRIB.mdsrc/subCommands/skill/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/subCommands/skill/index.ts
- docs/CONTRIB.md
- tests/e2e/skill/run
Comment on lines
+27
to
+38
| # Drop a minimal offline s.yaml that loads the local build. The component path | ||
| # is single-quoted so backslashes stay literal in YAML. | ||
| function Write-SYaml($dir) { | ||
| $yaml = @" | ||
| edition: 3.0.0 | ||
| name: skill-e2e | ||
| resources: | ||
| fc3: | ||
| component: '$fc3_dir' | ||
| props: {} | ||
| "@ | ||
| Set-Content -Path (Join-Path $dir "s.yaml") -Value $yaml -Encoding utf8 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape $fc3_dir before embedding it in YAML.
A single quote in a valid Windows path is not escaped. For example, D:\O'Brien\fc3 produces invalid YAML. Replace each ' with '' before interpolation.
Proposed fix
function Write-SYaml($dir) {
+ $yamlPath = $fc3_dir -replace "'", "''"
$yaml = @"
...
- component: '$fc3_dir'
+ component: '$yamlPath'
...📝 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
| # Drop a minimal offline s.yaml that loads the local build. The component path | |
| # is single-quoted so backslashes stay literal in YAML. | |
| function Write-SYaml($dir) { | |
| $yaml = @" | |
| edition: 3.0.0 | |
| name: skill-e2e | |
| resources: | |
| fc3: | |
| component: '$fc3_dir' | |
| props: {} | |
| "@ | |
| Set-Content -Path (Join-Path $dir "s.yaml") -Value $yaml -Encoding utf8 | |
| # Drop a minimal offline s.yaml that loads the local build. The component path | |
| # is single-quoted so backslashes stay literal in YAML. | |
| function Write-SYaml($dir) { | |
| $yamlPath = $fc3_dir -replace "'", "''" | |
| $yaml = @" | |
| edition: 3.0.0 | |
| name: skill-e2e | |
| resources: | |
| fc3: | |
| component: '$yamlPath' | |
| props: {} | |
| "@ | |
| Set-Content -Path (Join-Path $dir "s.yaml") -Value $yaml -Encoding utf8 | |
| } |
🤖 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 `@__tests__/e2e/skill/run-windows.ps1` around lines 27 - 38, Update Write-SYaml
to escape apostrophes in fc3_dir by replacing each single quote with two single
quotes before interpolating it into the single-quoted YAML component value.
Preserve the existing path and YAML structure for paths without apostrophes.
Comment on lines
+88
to
+92
| Write-Host "=== --tools filter installs only the requested tools ===" | ||
| Set-Location $filterRoot | ||
| s skill install --project --tools codex | ||
| Assert-File (Skill-Path $filterRoot "codex") | ||
| Assert-Missing (Join-Path $filterRoot ".cursor\skills\s-fc3") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that every non-selected tool is absent.
The test proves only that codex is installed and cursor is absent. It does not detect unexpected installations of claude, qoder, or agents. Check every tool except codex.
Proposed assertion loop
- Assert-Missing (Join-Path $filterRoot ".cursor\skills\s-fc3")
+ foreach ($t in $tools) {
+ if ($t -ne "codex") {
+ Assert-Missing (Join-Path $filterRoot ".$t\skills\s-fc3")
+ }
+ }📝 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
| Write-Host "=== --tools filter installs only the requested tools ===" | |
| Set-Location $filterRoot | |
| s skill install --project --tools codex | |
| Assert-File (Skill-Path $filterRoot "codex") | |
| Assert-Missing (Join-Path $filterRoot ".cursor\skills\s-fc3") | |
| Write-Host "=== --tools filter installs only the requested tools ===" | |
| Set-Location $filterRoot | |
| s skill install --project --tools codex | |
| Assert-File (Skill-Path $filterRoot "codex") | |
| foreach ($t in $tools) { | |
| if ($t -ne "codex") { | |
| Assert-Missing (Join-Path $filterRoot ".$t\skills\s-fc3") | |
| } | |
| } |
🤖 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 `@__tests__/e2e/skill/run-windows.ps1` around lines 88 - 92, Expand the --tools
filter test near the existing codex assertions to verify that every non-selected
tool is absent: claude, cursor, qoder, and agents. Retain the positive codex
assertion and use the existing Skill-Path, Assert-File, and Assert-Missing
helpers.
Comment on lines
+101
to
+105
| test('update overwrites an existing target and reports "updated"', () => { | ||
| installOne(source, 'qoder', 'project', 'install', false); | ||
| const result = installOne(source, 'qoder', 'project', 'update', false); | ||
| expect(result.status).toBe('updated'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify that update replaces the target content.
Both tests assert only status === 'updated'. Add a stale marker after the initial install, then assert that update removes it. This prevents a false pass when the implementation reports an update without replacing the target directory.
__tests__/ut/commands/skill/skill.test.ts#L101-L105: add a stale marker to theqoderproject target and assert thatinstallOne(..., 'update', ...)removes it.__tests__/ut/commands/skill/skill.test.ts#L149-L153: add a stale marker to thecursorglobal target and assert thatSkill.update()removes it.
📍 Affects 1 file
__tests__/ut/commands/skill/skill.test.ts#L101-L105(this comment)__tests__/ut/commands/skill/skill.test.ts#L149-L153
🤖 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 `@__tests__/ut/commands/skill/skill.test.ts` around lines 101 - 105, The update
tests currently verify only the reported status, not replacement of target
contents. In __tests__/ut/commands/skill/skill.test.ts lines 101-105, add a
stale marker to the qoder project target after the initial install and assert
that installOne(..., 'update', ...) removes it; in lines 149-153, add the same
stale-marker setup to the cursor global target and assert that Skill.update()
removes it.
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.
概述
新增
fc3 skill命令，把随组件打包的s-fc3skill 一键安装/更新到主流 Agent 工具目录。纯本地操作，无需云凭证。支持工具：
claude/codex/cursor/qoder/agents，统一约定安装到<工具目录>/skills/s-fc3/。用法
install：目标已存在则跳过（除非--force）update：始终覆盖os.homedir()，--project取process.cwd()改动
功能
src/subCommands/skill/：constants.ts（工具→目录映射）、installer.ts（源解析 + 拷贝，纯函数）、index.ts（参数解析与install/update派发）src/index.ts加skill()（不走handlePreRun，本地命令）、commands-help、publish.yamlprebuild/prewatch把.agents/skills/s-fc3拷进dist/skills/s-fc3，随 npm 包发布；运行时按dist/skills→ 仓库.agents→ cwd.agents顺序解析测试
--force覆盖、update 覆盖、工具过滤、双 scope）__tests__/e2e/skill/，沙箱、离线、无凭证）：run(bash) +run-windows.ps1，接入ci.sh/ci-mac-linux.sh/ci-windows.ps1及make test-skill文档
docs/architecture.md、docs/CONTRIB.md、CLAUDE.md.agents/skills/s-fc3/README.md、SKILL.md新增「安装本 Skill」章节测试计划
npm run typechecknpm test（1136 passing / 2 skipped，含新增 14）npm run build（dist/skills/s-fc3已产出）make test-skill（本地实跑 E2E 全部通过，含全局HOME沙箱安装）Summary by CodeRabbit
New Features
skill installandskill updatecommands for the bundled skill across supported agent tools.Tests