feat(cli): auto-upgrade skill and clean legacy hooks on start/restart - #15
Conversation
- Add runUpgradeMigrations() called from start/restart commands so `npm update` + `teams-bot restart` completes an upgrade automatically - syncInstalledSkill: version-stamped sync — only replaces the installed skill directory when the package version changes (.version marker) - cleanupLegacyHooks: removes stale SessionStart hook configs from settings.json that referenced the now-deleted session-start.sh - Move skill template from .claude/skills/ to skills/ so it won't be auto-discovered by Claude Code as a live skill - Remove stale .claude/hooks/ from package.json files array - Remove scope choice from install-skill (always installs globally) - Fix scope "2" destination path (was using projectDir instead of cwd) - Deduplicate cleanupLegacyHooks from uninstallSkill into shared function
There was a problem hiding this comment.
Pull request overview
This PR adds automatic upgrade behavior to the CLI so bot start/restart can migrate user config and keep the bundled /handoff skill in sync after package updates.
Changes:
- Add
runUpgradeMigrations()and invoke it fromstart/restartto perform upgrade-time maintenance. - Implement migrations to (1) remove legacy
SessionStarthook configs referencingsession-start.shand (2) version-stamp + sync the installed/handoffskill directory only when the package version changes. - Move the packaged skill template to
skills/handoff/and updatepackage.jsonpublish contents accordingly; add Vitest coverage for migrations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/upgrade-migrations.test.ts |
Adds test coverage for legacy hook cleanup and version-stamped skill sync behavior. |
src/cli/skill.ts |
Implements migrations (cleanupLegacyHooks, syncInstalledSkill) and updates install logic to use the new skills/ template location. |
src/cli/commands.ts |
Calls migrations on start and restart to make upgrades automatic. |
skills/handoff/SKILL.md |
Introduces the new packaged skill template location/content. |
package.json |
Updates published files list to include skills/ and remove legacy .claude/... entries. |
Comments suppressed due to low confidence (1)
src/cli/commands.ts:187
- runUpgradeMigrations() is executed before the healthz probe in startCommand(). If a migration throws (e.g., due to unexpected settings.json shape or filesystem permissions), the CLI will fail even when the bot is already running. Consider making migrations non-fatal (catch/log) so
teams-bot startremains reliable.
export async function startCommand(): Promise<void> {
const platform = detectPlatform();
runUpgradeMigrations();
// Check if already running
if (await probe("http://127.0.0.1:3978/healthz", 2000)) {
console.log("Bot is already running.");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("preserves non-legacy hooks", () => { | ||
| writeSettings(fakeHome, { | ||
| hooks: { | ||
| SessionStart: [ | ||
| { | ||
| hooks: [{ type: "command", command: "some-other-hook.sh" }], | ||
| }, | ||
| { | ||
| hooks: [ | ||
| { type: "command", command: "/path/session-start.sh" }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
This test case name is misleading: the second SessionStart group uses a command containing "session-start.sh", which the migration intentionally removes. Consider renaming the test to reflect that it preserves hooks not matching the legacy session-start.sh pattern, rather than implying session-start.sh is non-legacy.
| if (!hooks?.SessionStart) continue; | ||
| const groups = hooks.SessionStart as Array<Record<string, unknown>>; |
There was a problem hiding this comment.
cleanupLegacyHooks assumes hooks.SessionStart is an array and immediately calls .filter() on it. If a user’s settings.json has SessionStart set to a non-array value (e.g., an object/string), this will throw and can break start/restart (since migrations now run automatically). Add an Array.isArray(hooks.SessionStart) guard and skip (or normalize) when the shape is unexpected.
| if (!hooks?.SessionStart) continue; | |
| const groups = hooks.SessionStart as Array<Record<string, unknown>>; | |
| const sessionStart = hooks?.SessionStart; | |
| if (!Array.isArray(sessionStart)) continue; | |
| const groups = sessionStart as Array<Record<string, unknown>>; |
| if (!entry.isFile()) continue; | ||
| fs.copyFileSync(path.join(srcDir, entry.name), path.join(destDir, entry.name)); |
There was a problem hiding this comment.
copyDirSync is documented as replacing the destination directory contents with the source directory contents, but the implementation only copies top-level files and skips subdirectories. Either make this a recursive directory copy (so future skill templates with nested files work) or update the doc/comment + name to reflect that only flat files are copied.
| if (!entry.isFile()) continue; | |
| fs.copyFileSync(path.join(srcDir, entry.name), path.join(destDir, entry.name)); | |
| const srcPath = path.join(srcDir, entry.name); | |
| const destPath = path.join(destDir, entry.name); | |
| if (entry.isDirectory()) { | |
| copyDirSync(srcPath, destPath); | |
| continue; | |
| } | |
| if (!entry.isFile()) continue; | |
| fs.copyFileSync(srcPath, destPath); |
|
|
||
| export async function restartCommand(): Promise<void> { | ||
| const platform = detectPlatform(); | ||
| runUpgradeMigrations(); |
There was a problem hiding this comment.
runUpgradeMigrations() now runs as part of restartCommand(). Since it performs filesystem reads/writes, any thrown error will abort the restart flow before stopService/startService runs. Consider wrapping migrations in a try/catch here (or inside runUpgradeMigrations) and logging a warning so restart remains best-effort.
| runUpgradeMigrations(); | |
| try { | |
| await Promise.resolve(runUpgradeMigrations()); | |
| } catch (error) { | |
| console.warn( | |
| "Warning: upgrade migrations failed during restart; continuing with service restart.", | |
| error, | |
| ); | |
| } |
- Wrap runUpgradeMigrations() in try/catch in start/restart so migration errors don't block the bot from running - Add Array.isArray guard for SessionStart hooks - Make copyDirSync recursive for future-proofing
npm update+teams-bot restartcompletes an upgrade automatically