Skip to content

Commit ab2dfd9

Browse files
authored
Merge pull request #170 from gblanc-1a/bugfix/scaffolding_fixes_comment
fix: scaffolding improvements and fork PR comment support
2 parents ccee049 + 72b9bf5 commit ab2dfd9

24 files changed

+1451
-244
lines changed

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,6 @@ post-compile-fixes-cross-platform.js
155155

156156
# Cache directories
157157
.cache/
158+
159+
# Claude local settings file
160+
.claude/settings.local.json

AGENTS.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,8 @@ src/
188188
| `src/services/RepositoryScopeService.ts` | Repository scope file placement |
189189
| `src/adapters/*` | Source implementations (github, gitlab, http, local, awesome-copilot) |
190190
| `src/storage/RegistryStorage.ts` | Persistent paths and JSON layout |
191+
| `src/services/MigrationRegistry.ts` | globalState-based migration tracker |
192+
| `src/migrations/` | Migration scripts (one file per migration) |
191193
| `src/commands/*` | Command handlers wiring UI to services |
192194

193195
---
@@ -242,6 +244,16 @@ Installs support `user`, `workspace`, and `repository` scopes. Repository scope
242244
### Error Handling
243245
Use `Logger.getInstance()`. Throw errors with clear messages. Commands catch and show via VS Code notifications.
244246

247+
### Migrations
248+
Use `MigrationRegistry` (globalState-backed) for tracking data migrations. Each migration is a named entry with `pending`/`completed`/`skipped` status. Define migration logic in `src/migrations/`. Wire migrations into `extension.ts` activation via `runMigrations()`. Lockfile migrations use dual-read (try new + legacy ID) since lockfiles are Git-shared. Mark all migration-related code with `@migration-cleanup(migration-name)` comments so cleanup sites can be found with `grep -r "@migration-cleanup"`.
249+
250+
### Migration Cleanup
251+
When a migration is no longer needed, search for all related code with:
252+
```bash
253+
grep -r "@migration-cleanup(migration-name)" src/ test/
254+
```
255+
This finds every file with dual-read fallback, legacy functions, and migration logic that can be removed.
256+
245257
---
246258

247259
## Integration Points

CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
@AGENTS.md

docs/contributor-guide/architecture/installation-flow.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,8 @@ This ensures:
273273

274274
**Legacy format**: Older lockfiles may contain hub-prefixed sourceIds (`hub-{hubId}-{sourceId}`). These continue to work for backward compatibility—sources are resolved by matching the sourceId in the `sources` section.
275275

276+
**Case normalization (v2)**: Source IDs generated after this version use fully case-insensitive URL normalization (host + path lowercased). Older source IDs preserved path case. The extension uses dual-read: when matching source IDs, it checks both current and legacy formats. Lockfile entries with old-format IDs continue to work and migrate organically when bundles are updated. Local data (config.json, cache) is migrated automatically on activation via `MigrationRegistry`. All migration-related code is tagged with `@migration-cleanup(sourceId-normalization-v2)` for future removal.
277+
276278
### Hub Key Generation
277279

278280
Hub entries in the lockfile use URL-based keys instead of user-defined hub IDs:

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -790,12 +790,12 @@
790790
"lint:fix": "eslint src --ext ts --fix",
791791
"test": "npm run test:all",
792792
"test:one": "node scripts/run-single-test.js",
793-
"test:unit": "npx mocha --ui tdd --require ./test/mocha.setup.js --require ./test/unit.setup.js 'test-dist/test/{adapters,commands,config,helpers,services,storage,ui,unit,utils}/**/*.test.js' --timeout 5000",
793+
"test:unit": "npx mocha --ui tdd --require ./test/mocha.setup.js --require ./test/unit.setup.js 'test-dist/test/**/*.test.js' --ignore 'test-dist/test/suite/**/*.test.js' --timeout 5000",
794794
"test:integration": "npm run compile-tests && node ./test/runExtensionTests.js",
795795
"test:all": "npm run compile-tests && npm run test:unit && npm run test:integration",
796796
"test:coverage": "npm run compile-tests && c8 npm run test:all",
797797
"test:coverage:report": "c8 report --reporter=html --reporter=text --reporter=json",
798-
"test:coverage:unit": "npm run compile-tests && c8 --reporter=html --reporter=text mocha --ui tdd --require ./test/mocha.setup.js --require ./test/unit.setup.js 'test-dist/test/{adapters,commands,config,helpers,services,storage,ui,unit,utils}/**/*.test.js' --timeout 5000",
798+
"test:coverage:unit": "npm run compile-tests && c8 --reporter=html --reporter=text mocha --ui tdd --require ./test/mocha.setup.js --require ./test/unit.setup.js 'test-dist/test/**/*.test.js' --ignore 'test-dist/test/suite/**/*.test.js' --timeout 5000",
799799
"test:coverage:integration": "npm run compile-tests && c8 --reporter=html --reporter=text npm run test:integration",
800800
"coverage:clean": "rimraf coverage .nyc_output .c8_output",
801801
"ignore:status": "node scripts/package-helpers.js status",

src/commands/ScaffoldCommand.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { Logger } from '../utils/logger';
55
import { TemplateEngine, TemplateContext } from '../services/TemplateEngine';
66
import { FileUtils } from '../utils/fileUtils';
77
import { generateSanitizedId } from '../utils/bundleNameUtils';
8+
import { NpmCliWrapper } from '../utils/NpmCliWrapper';
89

910

1011
export enum ScaffoldType {
@@ -546,13 +547,14 @@ export class ScaffoldCommand {
546547
* Handle post-scaffold actions: npm install and folder opening
547548
*/
548549
private static async handlePostScaffoldActions(typeLabel: string, targetPath: vscode.Uri): Promise<void> {
550+
const npmWrapper = NpmCliWrapper.getInstance();
551+
await npmWrapper.installWithProgress(targetPath.fsPath);
549552
// The @prompt-registry/collection-scripts package is now available on npmjs
550553
const setupChoice = await vscode.window.showInformationMessage(
551-
`${typeLabel} scaffolded successfully! You can now run npm install to install dependencies.`,
554+
`${typeLabel} scaffolded successfully!`,
552555
'Open Folder',
553556
'Skip'
554557
);
555-
556558
if (setupChoice === 'Open Folder') {
557559
await vscode.commands.executeCommand('vscode.openFolder', targetPath);
558560
}

src/extension.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ import {
4242
import { ApmRuntimeManager } from './services/ApmRuntimeManager';
4343
import { OlafRuntimeManager } from './services/OlafRuntimeManager';
4444
import { SetupStateManager, SetupState } from './services/SetupStateManager';
45+
import { MigrationRegistry } from './services/MigrationRegistry';
46+
import { runSourceIdNormalizationMigration } from './migrations/sourceIdNormalizationMigration';
4547

4648
// Module-level variable to store the extension instance for deactivation
4749
let extensionInstance: PromptRegistryExtension | undefined;
@@ -137,6 +139,9 @@ export class PromptRegistryExtension {
137139
// Initialize Registry Manager
138140
await this.registryManager.initialize();
139141

142+
// Run data migrations (idempotent, skips if already completed)
143+
await this.runMigrations();
144+
140145
// Register commands
141146
this.registerCommands();
142147

@@ -221,6 +226,21 @@ export class PromptRegistryExtension {
221226
}
222227
}
223228

229+
/**
230+
* Run data migrations (idempotent).
231+
* Migrations use MigrationRegistry (globalState) to track completion.
232+
*/
233+
private async runMigrations(): Promise<void> {
234+
try {
235+
const migrationRegistry = MigrationRegistry.getInstance(this.context);
236+
const registryStorage = this.registryManager.getStorage();
237+
await runSourceIdNormalizationMigration(registryStorage, migrationRegistry);
238+
} catch (error) {
239+
this.logger.warn('Migration failed (non-fatal)', error as Error);
240+
// Don't fail activation if migrations fail
241+
}
242+
}
243+
224244
/**
225245
* Register all extension commands
226246
*/
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
/**
2+
* @migration-cleanup(sourceId-normalization-v2): Remove entire file once all lockfiles are migrated
3+
*
4+
* Source ID Normalization Migration (sourceId-normalization-v2)
5+
*
6+
* Migrates local data (config.json sources, source cache files, installation records)
7+
* from legacy source IDs (host-only lowercase) to v2 source IDs (full URL lowercase).
8+
*
9+
* Lockfiles are NOT rewritten here -- they are Git-committed and shared across teams.
10+
* Lockfile entries migrate organically when bundles are installed/updated.
11+
* Dual-read fallback in RepositoryActivationService and RegistryManager handles the gap.
12+
*/
13+
14+
import * as fs from 'fs';
15+
import * as path from 'path';
16+
import { promisify } from 'util';
17+
import { RegistryStorage } from '../storage/RegistryStorage';
18+
import { MigrationRegistry } from '../services/MigrationRegistry';
19+
import {
20+
generateHubSourceId,
21+
generateLegacyHubSourceId
22+
} from '../utils/sourceIdUtils';
23+
import { Logger } from '../utils/logger';
24+
25+
const rename = promisify(fs.rename);
26+
const readFile = promisify(fs.readFile);
27+
const writeFile = promisify(fs.writeFile);
28+
const readdir = promisify(fs.readdir);
29+
30+
export const MIGRATION_NAME = 'sourceId-normalization-v2';
31+
32+
/**
33+
* Run the source ID normalization migration.
34+
* Idempotent: uses MigrationRegistry to ensure it only runs once.
35+
*/
36+
export async function runSourceIdNormalizationMigration(
37+
storage: RegistryStorage,
38+
migrationRegistry: MigrationRegistry
39+
): Promise<void> {
40+
await migrationRegistry.runMigration(MIGRATION_NAME, async () => {
41+
const logger = Logger.getInstance();
42+
const paths = storage.getPaths();
43+
44+
// Step 1: Migrate config.json sources
45+
const idMap = await migrateConfigSources(storage, logger);
46+
47+
if (idMap.size === 0) {
48+
logger.info('No sources require ID migration');
49+
return;
50+
}
51+
52+
logger.info(`Migrating ${idMap.size} source ID(s): ${[...idMap.entries()].map(([o, n]) => `${o} -> ${n}`).join(', ')}`);
53+
54+
// Step 2: Rename source cache files
55+
await migrateSourceCacheFiles(paths.sourcesCache, idMap, logger);
56+
57+
// Step 3: Update installation records that reference old sourceIds
58+
await migrateInstallationRecords(paths.userInstalled, idMap, logger);
59+
await migrateInstallationRecords(paths.installed, idMap, logger);
60+
});
61+
}
62+
63+
/**
64+
* Migrate source IDs in config.json.
65+
* Returns a map of oldId -> newId for sources that were migrated.
66+
*/
67+
async function migrateConfigSources(
68+
storage: RegistryStorage,
69+
logger: Logger
70+
): Promise<Map<string, string>> {
71+
const idMap = new Map<string, string>();
72+
const sources = await storage.getSources();
73+
let changed = false;
74+
75+
for (const source of sources) {
76+
// Only hub-generated IDs (format: {type}-{12hexchars}) need migration
77+
if (!isHubGeneratedId(source.id)) {
78+
continue;
79+
}
80+
81+
// Compute what the new ID should be
82+
const newId = generateHubSourceId(source.type, source.url, {
83+
branch: source.config?.branch,
84+
collectionsPath: source.config?.collectionsPath
85+
});
86+
87+
// If the stored ID differs from the new format, it's a legacy ID
88+
if (source.id !== newId) {
89+
// Verify it's actually the legacy form (not some other mismatch)
90+
const legacyId = generateLegacyHubSourceId(source.type, source.url, {
91+
branch: source.config?.branch,
92+
collectionsPath: source.config?.collectionsPath
93+
});
94+
95+
if (legacyId && source.id === legacyId) {
96+
logger.info(`Migrating source '${source.name}': ${source.id} -> ${newId}`);
97+
idMap.set(source.id, newId);
98+
source.id = newId;
99+
changed = true;
100+
}
101+
}
102+
}
103+
104+
if (changed) {
105+
// Save the updated config through RegistryStorage
106+
const config = await storage.loadConfig();
107+
config.sources = sources;
108+
await storage.saveConfig(config);
109+
}
110+
111+
return idMap;
112+
}
113+
114+
/**
115+
* Check if a source ID looks like a hub-generated ID (format: {type}-{12hexchars})
116+
*/
117+
function isHubGeneratedId(id: string): boolean {
118+
return /^[a-z]+-[a-f0-9]{12}$/.test(id);
119+
}
120+
121+
/**
122+
* Rename source cache files from old sanitized ID to new sanitized ID.
123+
*/
124+
async function migrateSourceCacheFiles(
125+
cacheDir: string,
126+
idMap: Map<string, string>,
127+
logger: Logger
128+
): Promise<void> {
129+
for (const [oldId, newId] of idMap) {
130+
const oldFile = path.join(cacheDir, `${sanitize(oldId)}.json`);
131+
const newFile = path.join(cacheDir, `${sanitize(newId)}.json`);
132+
133+
try {
134+
if (fs.existsSync(oldFile) && !fs.existsSync(newFile)) {
135+
await rename(oldFile, newFile);
136+
logger.debug(`Renamed cache file: ${sanitize(oldId)}.json -> ${sanitize(newId)}.json`);
137+
}
138+
} catch (error) {
139+
logger.warn(`Failed to rename cache file for ${oldId}`, error as Error);
140+
}
141+
}
142+
}
143+
144+
/**
145+
* Update sourceId references in installation record JSON files.
146+
*/
147+
async function migrateInstallationRecords(
148+
installDir: string,
149+
idMap: Map<string, string>,
150+
logger: Logger
151+
): Promise<void> {
152+
let files: string[];
153+
try {
154+
files = await readdir(installDir);
155+
} catch {
156+
return; // directory doesn't exist
157+
}
158+
159+
for (const file of files) {
160+
if (!file.endsWith('.json')) {
161+
continue;
162+
}
163+
164+
const filepath = path.join(installDir, file);
165+
try {
166+
const data = await readFile(filepath, 'utf-8');
167+
const record = JSON.parse(data);
168+
169+
if (record.sourceId && idMap.has(record.sourceId)) {
170+
record.sourceId = idMap.get(record.sourceId);
171+
await writeFile(filepath, JSON.stringify(record, null, 2), 'utf-8');
172+
logger.debug(`Updated sourceId in installation record: ${file}`);
173+
}
174+
} catch (error) {
175+
logger.warn(`Failed to migrate installation record ${file}`, error as Error);
176+
}
177+
}
178+
}
179+
180+
/**
181+
* Sanitize an ID for filenames (mirrors RegistryStorage.sanitizeFilename logic).
182+
*/
183+
function sanitize(id: string): string {
184+
return id.replace(/[^A-Za-z0-9._-]/g, '_').substring(0, 200);
185+
}

src/services/AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ Services contain business logic, separated from UI and commands.
1515
| `LockfileManager` | Manages `prompt-registry.lock.json` for repository scope |
1616
| `ScopeConflictResolver` | Prevents same bundle at both user and repository scope |
1717
| `RepositoryActivationService` | Handles lockfile detection on workspace open |
18+
| `MigrationRegistry` | Tracks completed data migrations via `context.globalState` |
1819
| `LocalModificationWarningService` | Detects local file changes before updates |
1920
| `HubManager` | Hub configurations and profiles |
2021
| `McpServerManager` | MCP server lifecycle |

0 commit comments

Comments
 (0)