Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions packages/core/src/extension/extension-store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,80 @@ describe('ExtensionStore', () => {
).toMatchObject({ effective: 'enabled', source: 'workspace_override' });
});

it('re-keys a policy to a new id for the same name after an id-formula change', async () => {
const store = makeStore();
const oldId = 'a'.repeat(64);
const newId = 'b'.repeat(64);
await store.ensureInitialized([{ id: oldId, name: 'dotnet' }]);
await store.setDefaultActivation({ id: oldId, name: 'dotnet' }, 'disabled');
await store.setWorkspaceActivation(
{ id: oldId, name: 'dotnet' },
'/workspace/a',
'enabled',
);

const snapshot = await store.ensureInitialized([
{ id: newId, name: 'dotnet' },
]);

expect(snapshot.extensions[oldId]).toBeUndefined();
expect(snapshot.extensions[newId]).toMatchObject({
name: 'dotnet',
defaultActivation: 'disabled',
workspaceOverrides: { '/workspace/a': 'enabled' },
});
});

it('re-keys across a case mismatch and normalizes the stored name', async () => {
const store = makeStore();
const oldId = 'a'.repeat(64);
const newId = 'b'.repeat(64);
await store.ensureInitialized([{ id: oldId, name: 'DotNet' }]);
await store.setDefaultActivation({ id: oldId, name: 'DotNet' }, 'disabled');

const snapshot = await store.ensureInitialized([
{ id: newId, name: 'dotnet' },
]);

expect(snapshot.extensions[oldId]).toBeUndefined();
expect(snapshot.extensions[newId]).toMatchObject({
name: 'dotnet',
defaultActivation: 'disabled',
});
});

it('re-keys only the orphaned policy when a sibling plugin installs fresh', async () => {
const store = makeStore();
const repoOnlyId = 'a'.repeat(64);
const dotnetId = 'b'.repeat(64);
const dotnetTestId = 'c'.repeat(64);
await store.ensureInitialized([{ id: repoOnlyId, name: 'dotnet' }]);

const snapshot = await store.ensureInitialized([
{ id: dotnetId, name: 'dotnet' },
{ id: dotnetTestId, name: 'dotnet-test' },
]);

expect(snapshot.extensions[repoOnlyId]).toBeUndefined();
expect(snapshot.extensions[dotnetId]?.name).toBe('dotnet');
expect(snapshot.extensions[dotnetTestId]?.name).toBe('dotnet-test');
});

it('does not re-key a policy still owned by another loaded extension', async () => {
const store = makeStore();
const demoId = 'a'.repeat(64);
const otherId = 'b'.repeat(64);
await store.ensureInitialized([{ id: demoId, name: 'demo' }]);

const snapshot = await store.ensureInitialized([
{ id: demoId, name: 'demo' },
{ id: otherId, name: 'other' },
]);

expect(snapshot.extensions[demoId]?.name).toBe('demo');
expect(snapshot.extensions[otherId]?.name).toBe('other');
});

it('uses an inherit mask when clearing an override matched by a legacy rule', async () => {
await fsp.writeFile(
enablementPath,
Expand Down
26 changes: 26 additions & 0 deletions packages/core/src/extension/extension-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,32 @@ export class ExtensionStore {
const legacy = await this.readLegacyProjection();
if (existing) {
let changed = false;
// An id-formula change (e.g. #7568 added the plugin name to the hash)
// leaves installed extensions pointing at ids the store has never
// seen. Without this re-key, the blocks below would mint a fresh
// default policy under the new id — resetting activation state — and
// strand the old policy as an orphan that later trips the
// name-conflict guard on update/uninstall. Names are unique in the
// store (enforced case-insensitively on every commit), so a loaded
// identity whose id is unknown safely claims the policy stored under
// its name, provided no loaded extension still owns that entry.
const loadedIds = new Set(extensions.map((identity) => identity.id));
for (const identity of extensions) {
assertIdentity(identity);
if (existing.extensions[identity.id]) continue;
const staleEntry = Object.entries(existing.extensions).find(
([id, policy]) =>
!loadedIds.has(id) &&
policy.name.toLowerCase() === identity.name.toLowerCase(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The re-key migration uses case-insensitive name matching (toLowerCase()) but no test exercises a case mismatch between the stored policy name and the incoming identity name.

Failure scenario: a future refactor that simplifies to policy.name === identity.name would silently break migration for extensions stored under different casing — the stale policy would be orphaned and activation state reset with no error.

Consider adding a test that seeds the store with { id: oldId, name: 'DotNet' }, sets activation state, then calls ensureInitialized([{ id: newId, name: 'dotnet' }]) and asserts re-key with name: 'dotnet' and state preserved.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in ec51b8are-keys across a case mismatch and normalizes the stored name: seeds { id: oldId, name: 'DotNet' } with defaultActivation: 'disabled', calls ensureInitialized([{ id: newId, name: 'dotnet' }]), and asserts the policy moves to the new id with state preserved and name normalized to 'dotnet'. Store suite 59/59.

中文

已在 ec51b8a 补上——re-keys across a case mismatch and normalizes the stored name:以 DotNet 名字播种 disabled 状态的 policy,用 dotnet 身份触发 re-key,断言 policy 迁移到新 id、状态保留、名字归一为 dotnet。store 套件 59/59。

);
if (staleEntry) {
const [staleId, policy] = staleEntry;
delete existing.extensions[staleId];
policy.name = identity.name;
existing.extensions[identity.id] = policy;
changed = true;
}
}
if (existing.legacyProjectionHash !== projectionHash(legacy)) {
if (!(await this.legacyProjectionIsNewerThanState())) {
try {
Expand Down
35 changes: 35 additions & 0 deletions packages/core/src/extension/extensionManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2780,6 +2780,41 @@ describe('extension tests', () => {
hashValue('https://gitlab.company.com/team/extension-repo'),
);
});

it('gives plugins from the same repository distinct ids (#7568)', () => {
const metadataFor = (pluginName: string) => ({
type: 'git' as const,
source: 'https://github.com/dotnet/skills',
pluginName,
});
const dotnetId = getExtensionId(
{ name: 'dotnet', version: '1.0.0' },
metadataFor('dotnet'),
);
const dotnetTestId = getExtensionId(
{ name: 'dotnet-test', version: '1.0.0' },
metadataFor('dotnet-test'),
);

expect(dotnetId).toBe(
hashValue('https://github.com/dotnet/skills:dotnet'),
);
expect(dotnetTestId).toBe(
hashValue('https://github.com/dotnet/skills:dotnet-test'),
);
expect(dotnetId).not.toBe(dotnetTestId);
});

it('keeps the repo-only id when no plugin name is recorded', () => {
const config: ExtensionConfig = { name: 'solo-ext', version: '1.0.0' };
const metadata = {
type: 'git' as const,
source: 'https://github.com/owner/solo',
};
expect(getExtensionId(config, metadata)).toBe(
hashValue('https://github.com/owner/solo'),
);
});
});
});

Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/extension/extensionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2624,6 +2624,14 @@ export function getExtensionId(
} else {
idValue = installMetadata?.source ?? config.name;
}
// A marketplace repo can host several plugins; hashing only the repo URL
// collapses them into one id and the second install trips the store's
// id-ownership check (#7568). Suffix the plugin name so each plugin from
// the same source gets its own id. Installs recorded under the old
// repo-only id are re-keyed by ExtensionStore.ensureInitialized.
if (installMetadata?.pluginName) {
idValue += `:${installMetadata.pluginName}`;
}
return hashValue(idValue);
}

Expand Down
Loading