Skip to content

Commit 610d0c6

Browse files
committed
fix(create-objectstack): count a skill as installed only when the lock records it synced from the package
The sync skips, exit 0, a skill directory it did not install (one from skills add) and leaves it in place, so neither the exit code nor the directory is the verdict. Claude-Session: https://claude.ai/code/session_01VoSxBQujKLZKPwK2u5ehQ6 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 026b6bc commit 610d0c6

4 files changed

Lines changed: 87 additions & 15 deletions

File tree

‎.changeset/22659-scaffold-skills-from-package.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,6 @@ Clause-②: no
88

99
- **The project depends on the catalog.** The blank template's `devDependencies` gain `@objectstack/skills`, at the same `^<version>` as every other `@objectstack/*` dependency, so a new project's dependency set changes: the skills are the ones written for the `@objectstack/spec` it installs.
1010
- **The install reads `node_modules`.** After `<pm> install`, the scaffolder runs `npx -y skills@1.7.2 experimental_sync --agent claude-code --copy -y`, which copies the installed package's catalog into `.claude/skills/` as real files, once, beside a `skills-lock.json` that names the package as every skill's source. The GitHub `npx skills add objectstack-ai/objectstack/skills …` command is gone from the run, its summary and the `AGENTS.md` and `README.md` it writes into the project.
11-
- **The summary says what happened.** It names the package version installed and prints the same command for re-installing after an upgrade of the `@objectstack/*` packages, and for any other agent runtime. When the install leaves no catalog to read, the run says so instead of reporting the skills installed.
11+
- **The summary says what happened.** It names the package version installed and prints the same command for re-installing after an upgrade of the `@objectstack/*` packages, and for any other agent runtime. When the install leaves no catalog to read, or a skill did not come from the package, the run says so instead of reporting the skills installed.
1212
- **The flags keep their meaning.** `--skip-skills` skips the skills step. `--skip-install` installs no skills, because nothing is in `node_modules` yet; the summary names `<pm> install` and then the sync command.
13-
- **To re-install skills in a project created earlier:** add `@objectstack/skills` to `devDependencies` at your `@objectstack/spec` range, run your package manager's install, then `npx skills@1.7.2 experimental_sync --agent claude-code --copy -y` (name your own runtime after `--agent`).
13+
- **To move a project created earlier onto the package:** add `@objectstack/skills` to `devDependencies` at your `@objectstack/spec` range and run your package manager's install. Remove the skills `skills add` installed — `npx skills remove objectstack-data -y` and so on for each `objectstack-*` skill, or `npx skills remove --all` if you installed no others — because the sync skips a skill it did not install and leaves the old copy in place. Then run `npx skills@1.7.2 experimental_sync --agent claude-code --copy -y` (name your own runtime after `--agent`).

‎packages/create-objectstack/src/index.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ import {
8686
listSkills,
8787
skillsInstallHint,
8888
skillsPackageDir,
89+
skillsSyncedFromPackage,
8990
} from './skills-install.js';
9091
import { renderVersionBanner } from './banner.js';
9192

@@ -390,10 +391,11 @@ function writeIfAbsent(filePath: string, contents: string) {
390391
// version installed, or `null` after a warning naming the remedy.
391392
//
392393
// The sync's exit code is not the verdict: with no package to read it prints
393-
// "Nothing to sync." and exits 0 (`skills-install.ts`). So both sides are read
394-
// here — a catalog in the package before the call, and every one of its skills
395-
// in the agent's directory after it — and a gap on either is said out loud
396-
// rather than summarised as "Installed".
394+
// "Nothing to sync." and exits 0, and it exits 0 having skipped a skill it did
395+
// not install (`skills-install.ts`). So both sides are read here — a catalog
396+
// in the package before the call, and every one of its skills in the agent's
397+
// directory, recorded in the lock as synced from the package, after it — and
398+
// a gap on either is said out loud rather than summarised as "Installed".
397399
function installSkillsFromPackage(
398400
targetDir: string,
399401
installed: boolean,
@@ -430,13 +432,17 @@ function installSkillsFromPackage(
430432
return null;
431433
}
432434

435+
// Both the files and the lock entry: the sync leaves an existing directory
436+
// it did not write (one from `skills add`) in place and still exits 0.
433437
const landed = new Set(listSkills(path.join(targetDir, DEFAULT_SKILLS_DIR)));
434-
const missing = catalog.filter((skill) => !landed.has(skill));
438+
const synced = skillsSyncedFromPackage(targetDir);
439+
const missing = catalog.filter((skill) => !landed.has(skill) || !synced.has(skill));
435440
if (missing.length > 0) {
436441
printWarning(
437442
`Skills installation incomplete: ${missing.length} of ${catalog.length} skill(s) in ` +
438-
`${SKILLS_PACKAGE}@${version} did not land in ${DEFAULT_SKILLS_DIR} as files ` +
439-
`(${missing.join(', ')}). Run manually:\n${hint}`,
443+
`${SKILLS_PACKAGE}@${version} are not in ${DEFAULT_SKILLS_DIR} as files synced from it ` +
444+
`(${missing.join(', ')}). The sync leaves a skill installed earlier with \`skills add\` ` +
445+
`in place: remove it (\`npx skills remove <name> -y\`), then run:\n${hint}`,
440446
);
441447
console.log('');
442448
return null;

‎packages/create-objectstack/src/scaffold-skills-single-copy.test.ts‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@
2222
// -y .agents/skills/<skill> links into node_modules/<package>/…
2323
// .claude/skills/<skill> links into .agents/skills/<skill>
2424
//
25-
// and with nothing to read it prints "Nothing to sync." and exits 0.
25+
// With nothing to read it prints "Nothing to sync." and exits 0; a skill whose
26+
// directory is already there and whose lock entry is not a sync's (one from
27+
// `skills add`) is SKIPPED, left as it was, and the run still exits 0.
2628
//
2729
// ## Why both CLIs are stubbed, and how the stubs are kept honest
2830
//
@@ -164,21 +166,32 @@ for (const dep of deps) {
164166
}
165167
}
166168
if (found.length === 0) { console.log('Nothing to sync.'); process.exit(0); }
169+
const lockPath = path.join(cwd, 'skills-lock.json');
170+
let prior = { version: 1, skills: {} };
171+
try { prior = JSON.parse(fs.readFileSync(lockPath, 'utf8')); } catch {}
172+
const synced = found.filter((skill) => {
173+
const taken = SHAPES[key].some((dest) => fs.existsSync(path.join(cwd, dest.dir, skill.name)));
174+
if (taken && prior.skills?.[skill.name]?.sourceType !== 'node_modules') {
175+
console.warn('Skipped ' + skill.name + ' from ' + skill.dep + ': installed with skills add');
176+
return false;
177+
}
178+
return true;
179+
});
167180
for (const dest of SHAPES[key]) {
168181
fs.mkdirSync(path.join(cwd, dest.dir), { recursive: true });
169-
for (const skill of found) {
182+
for (const skill of synced) {
170183
const target = path.join(cwd, dest.dir, skill.name);
171184
if (dest.kind === 'copy') fs.cpSync(skill.src, target, { recursive: true });
172185
else if (dest.kind === 'link-package') fs.symlinkSync(path.relative(path.dirname(target), skill.src), target);
173186
else fs.symlinkSync(dest.into + '/' + skill.name, target);
174187
}
175188
}
176-
const lock = { version: 1, skills: {} };
177-
for (const s of found) {
189+
const lock = { version: 1, skills: { ...prior.skills } };
190+
for (const s of synced) {
178191
lock.skills[s.name] = { source: s.dep, sourceType: 'node_modules', skillPath: s.skillPath, version: s.version };
179192
}
180193
fs.writeFileSync(path.join(cwd, 'skills-lock.json'), JSON.stringify(lock, null, 2) + '\\n');
181-
console.log('npx-stub: synced ' + found.length + ' skills');
194+
console.log('npx-stub: synced ' + synced.length + ' skills');
182195
`;
183196
}
184197

@@ -427,6 +440,33 @@ describe('without a package to read, no skills — and the run says how to get t
427440
expect(block).toContain(skillsInstallHint(DEFAULT_SKILLS_AGENT));
428441
});
429442

443+
// Scaffolding into the current directory is the one path onto existing
444+
// content. A skill that `skills add` put there earlier is SKIPPED by the
445+
// sync, exit 0, its directory still present — so neither the exit code nor
446+
// the directory can be the verdict; the lock entry naming the package is.
447+
it('a skill already there from `skills add` is reported, not counted as installed', () => {
448+
const project = path.join(TMP, 'here-app');
449+
const legacy = path.join(project, '.claude', 'skills', 'objectstack-data');
450+
fs.mkdirSync(legacy, { recursive: true });
451+
fs.writeFileSync(path.join(legacy, 'SKILL.md'), '---\nname: objectstack-data\n---\n\n# from main\n');
452+
fs.writeFileSync(
453+
path.join(project, 'skills-lock.json'),
454+
JSON.stringify({ version: 1, skills: { 'objectstack-data': { source: 'objectstack-ai/objectstack', sourceType: 'github' } } }) + '\n',
455+
);
456+
const receipt = path.join(TMP, 'here-app.npx-argv.log');
457+
const stdout = execFileSync(TSX, [INDEX_TS], {
458+
cwd: project,
459+
encoding: 'utf8',
460+
env: { ...process.env, NPX_STUB_RECEIPT: receipt, PATH: `${BIN}${path.delimiter}${process.env.PATH ?? ''}` },
461+
});
462+
expect(fs.readFileSync(receipt, 'utf8').trim().split('\n')).toHaveLength(1);
463+
expect(listSkills(path.join(project, DEFAULT_SKILLS_DIR))).toEqual(STUB_CATALOG);
464+
expect(fs.readFileSync(path.join(legacy, 'SKILL.md'), 'utf8')).toContain('# from main');
465+
expect(stdout).toContain('1 of 3 skill(s)');
466+
expect(stdout).toContain('(objectstack-data)');
467+
expect(stdout).not.toContain('Installed for');
468+
});
469+
430470
// The sync exits 0 with "Nothing to sync." when there is no package, so a
431471
// scaffolder that trusted the exit code would print "Installed".
432472
it('an install that leaves no catalog: the run says so, and never claims Installed', () => {

‎packages/create-objectstack/src/skills-install.ts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,12 @@
4545
// is the only one that ships any, so the synced set is exactly its catalog.
4646
// With no package to read it prints "Nothing to sync." and exits 0 — which is
4747
// why the scaffolder checks both sides itself (`listSkills` below) instead of
48-
// reading the exit code as "installed".
48+
// reading the exit code as "installed". It also exits 0 when it SKIPS a skill
49+
// whose directory is already there and was not put there by a sync — one from
50+
// `skills add`, say ("Skipped objectstack-data from @objectstack/skills:
51+
// installed with `skills add` …"), leaving that copy and its lock entry as
52+
// they were. So a directory being present proves nothing either; the lock
53+
// entry naming the package (`skillsSyncedFromPackage`) does.
4954
//
5055
// The CLI is pinned to the version measured. `experimental_` is that package's
5156
// own word for an interface it may change, and an unpinned `npx skills` would
@@ -143,6 +148,27 @@ export function installedSkillsVersion(projectDir: string): string | null {
143148
}
144149
}
145150

151+
/** The skills CLI's record of what it installed, at the project root. */
152+
export const SKILLS_LOCK_FILE = 'skills-lock.json';
153+
154+
/**
155+
* The skills the project's `skills-lock.json` records as synced from the
156+
* installed `@objectstack/skills` package (`source` the package,
157+
* `sourceType: node_modules`). Empty when there is no readable lock.
158+
*/
159+
export function skillsSyncedFromPackage(projectDir: string): Set<string> {
160+
try {
161+
const lock = JSON.parse(fs.readFileSync(path.join(projectDir, SKILLS_LOCK_FILE), 'utf8'));
162+
return new Set(
163+
Object.entries((lock?.skills ?? {}) as Record<string, { source?: unknown; sourceType?: unknown }>)
164+
.filter(([, entry]) => entry?.source === SKILLS_PACKAGE && entry?.sourceType === 'node_modules')
165+
.map(([name]) => name),
166+
);
167+
} catch {
168+
return new Set();
169+
}
170+
}
171+
146172
/**
147173
* The skills under `dir` — the names of its immediate subdirectories that
148174
* hold a `SKILL.md` — sorted. Empty when `dir` does not exist. A symlinked

0 commit comments

Comments
 (0)