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
9 changes: 9 additions & 0 deletions .changeset/sdk-revalidate-permissive-skip-needs-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@ifc-lite/extensions": minor
---

Fix `revalidateAgainstSdk` silently treating an unverifiable extension as fine after an SDK bump.

An extension whose declared `engines.ifcLiteSdk` range is too loose to evaluate (e.g. a wildcard like `2.x`) gets `compatibility.status: 'permissive'` — the range comparator's own docs describe this as "worth a re-test, even if the range technically passes." When such an extension has no declared tests (or its bundle bytes aren't available), the test run comes back `outcome: 'skipped'` — nothing actually confirmed it still works. `needsRepair` only included skipped rows whose status was `'outdated'`, so a permissive, self-unverifiable extension never surfaced in the repair queue after a major SDK bump. Since `'skipped'` can only occur for `'outdated'` or `'permissive'` rows (the `'compatible'` branch always resolves to `'pass'` without touching the test runner), `needsRepair` now includes every skipped row.

The rule now lives in one exported function, `needsSdkRepair`. The viewer's repair panel carried a second copy of the predicate to decide which rows get a Repair button, so widening only the queue side made the header ("N need fixing") count permissive, skipped extensions whose rows offered no way to fix them. Both sides call the shared function, and a rendering test pins the invariant the two copies were supposed to preserve: the header count equals the number of rows with a Repair button.
174 changes: 174 additions & 0 deletions apps/viewer/src/components/extensions/RepairQueuePanel.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

/**
* Rendering tests for `RepairQueuePanel` — the header/row invariant.
*
* The panel prints "<n> need fixing" from `summary.needsRepair.length`
* and decides per row whether to render a Repair button. Those two
* decisions used to be made by two separate copies of the same
* predicate, one in `revalidateAgainstSdk` and one in this file; when
* only the first was widened to cover permissive-but-unverifiable
* extensions, the header started counting rows the user had no way to
* act on. Both now call the exported `needsSdkRepair`, and this test
* pins the property that made the divergence a bug: the header count
* equals the number of actionable rows.
*
* happy-dom provides the DOM (registered by `@/test/setup-dom.js`,
* which must stay the first import); React 19's `createRoot` + `act()`
* drive the component for real. The host is a real
* `ExtensionHostService` subclass with only `revalidateForSdk`
* overridden — the genuine one needs IndexedDB and a QuickJS sandbox.
*/

import '@/test/setup-dom.js';
import { beforeEach, describe, it } from 'node:test';
import assert from 'node:assert/strict';
import { act } from 'react';
import { createRoot, type Root } from 'react-dom/client';
import { needsSdkRepair } from '@ifc-lite/extensions';
import type {
Compatibility,
RevalidationItem,
RevalidationSummary,
} from '@ifc-lite/extensions';
import { createBimContext } from '@ifc-lite/sdk';
import { ExtensionHostService } from '@/services/extensions/host.js';
import { ExtensionHostContext } from '@/sdk/ExtensionHostProvider.js';
import { RepairQueuePanel } from './RepairQueuePanel.js';

const SDK = '2.0.0';

class StubExtensionHost extends ExtensionHostService {
summary: RevalidationSummary = { sdk: SDK, items: [], needsRepair: [] };

constructor() {
super({
sdk: createBimContext({
transport: {
send: () => Promise.reject(new Error('SDK transport is not exercised by this test')),
subscribe: () => () => {},
close: () => {},
},
}),
});
}

override revalidateForSdk(): Promise<RevalidationSummary> {
return Promise.resolve(this.summary);
}
}

function item(
extensionId: string,
outcome: RevalidationItem['outcome'],
status: Compatibility,
): RevalidationItem {
return {
extensionId,
outcome,
compatibility: {
extensionId,
declared: status === 'permissive' ? '*' : '^1.0.0',
sdk: SDK,
status,
reason: `${status} range`,
},
};
}

const mounted: Array<{ root: Root; container: HTMLElement }> = [];

function renderPanel(host: ExtensionHostService): HTMLElement {
const container = document.createElement('div');
document.body.appendChild(container);
const root = createRoot(container);
act(() => {
root.render(
<ExtensionHostContext.Provider value={host}>
<RepairQueuePanel sdkVersion={SDK} />
</ExtensionHostContext.Provider>,
);
});
mounted.push({ root, container });
return container;
}

/** Click the panel's "Run check" button and let the promise settle. */
async function runCheck(container: HTMLElement): Promise<void> {
const button = [...container.querySelectorAll('button')].find((b) =>
b.textContent?.includes('Run check'),
);
assert.ok(button, 'expected a "Run check" button before any summary exists');
await act(async () => {
button.click();
await Promise.resolve();
});
}

/** The "<n> need fixing" number printed in the header. */
function headerCount(container: HTMLElement): number {
const match = /(\d+) need fixing/.exec(container.textContent ?? '');
assert.ok(match, `header must print a "need fixing" count; got: ${container.textContent}`);
return Number(match[1]);
}

/** Rows offering the user a Repair button. */
function repairButtonCount(container: HTMLElement): number {
return [...container.querySelectorAll('button')].filter((b) =>
b.textContent?.includes('Repair'),
).length;
}

describe('RepairQueuePanel header/row agreement', () => {
beforeEach(() => {
for (const { root, container } of mounted.splice(0)) {
act(() => {
root.unmount();
});
container.remove();
}
});

it('the header count equals the number of rows offering a Repair button', async () => {
const host = new StubExtensionHost();
const items = [
item('ext.pass', 'pass', 'compatible'),
item('ext.failed', 'fail', 'outdated'),
item('ext.skipped-outdated', 'skipped', 'outdated'),
// A permissive extension (wildcard range) with no declared tests:
// the queue counts it, so the row must offer the fix.
item('ext.skipped-permissive', 'skipped', 'permissive'),
];
host.summary = {
sdk: SDK,
items,
// Built the way `revalidateAgainstSdk` builds it, so the header
// reflects the real queue rule rather than a copy of it here.
needsRepair: items.filter(needsSdkRepair),
};

const container = renderPanel(host);
await runCheck(container);

assert.equal(headerCount(container), 3, 'fixture must exercise a non-empty repair queue');
assert.equal(
repairButtonCount(container),
headerCount(container),
'every extension counted in the header must have an actionable Repair button',
);
});

it('a queue with nothing to fix renders no Repair buttons', async () => {
const host = new StubExtensionHost();
const items = [item('ext.pass', 'pass', 'compatible')];
host.summary = { sdk: SDK, items, needsRepair: [] };

const container = renderPanel(host);
await runCheck(container);

assert.equal(headerCount(container), 0);
assert.equal(repairButtonCount(container), 0);
});
});
16 changes: 2 additions & 14 deletions apps/viewer/src/components/extensions/RepairQueuePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import { useCallback, useState } from 'react';
import { CheckCircle2, RefreshCcw, ShieldAlert, Wrench, X } from 'lucide-react';
import { needsSdkRepair } from '@ifc-lite/extensions';
import type { RevalidationItem, RevalidationSummary } from '@ifc-lite/extensions';
import { Button } from '@/components/ui/button';
import { ScrollArea } from '@/components/ui/scroll-area';
Expand Down Expand Up @@ -181,7 +182,7 @@ function RepairRow({
</div>
)}
</div>
{itemNeedsRepair(item) && (
{needsSdkRepair(item) && (
<Button size="sm" variant="outline" onClick={onRepair}>
<Wrench className="mr-1 h-3.5 w-3.5" />
Repair
Expand All @@ -192,19 +193,6 @@ function RepairRow({
);
}

/**
* Whether a row should show a Repair button. Mirrors the
* `needsRepair` filter in `revalidateAgainstSdk` exactly — a failed
* test OR a skipped extension whose declared range is outdated — so
* the header count and the actionable rows never disagree.
*/
function itemNeedsRepair(item: RevalidationItem): boolean {
return (
item.outcome === 'fail'
|| (item.outcome === 'skipped' && item.compatibility.status === 'outdated')
);
}

function buildRepairPrompt(item: RevalidationItem, sdk: string): string {
const failures = item.tests?.results.filter((r) => !r.passed) ?? [];
return [
Expand Down
1 change: 1 addition & 0 deletions packages/extensions/src/host/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export {
} from './sdk-version.js';
export {
revalidateAgainstSdk,
needsSdkRepair,
type RevalidationItem,
type RevalidationSummary,
type RevalidateOptions,
Expand Down
18 changes: 18 additions & 0 deletions packages/extensions/src/host/sdk-revalidate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,4 +136,22 @@ describe('revalidateAgainstSdk', () => {
expect(summary.items[0].compatibility.status).toBe('compatible');
expect(summary.items[1].compatibility.status).toBe('outdated');
});

it('flags a permissive, self-unverifiable extension for repair', async () => {
// Wildcard range -> compatibility.status 'permissive' (sdk-version.ts:
// "worth a re-test, even if the range technically passes"). With no
// declared tests the run is 'skipped', so nothing actually confirmed
// the extension still works across the major SDK bump — it must not
// be silently treated as fine.
const bundle = makeBundle({ declared: '2.x', withTests: false });
const summary = await revalidateAgainstSdk({
sdk: '3.0.0',
installed: [{ id: bundle.manifest.id, engines: bundle.manifest.engines, grants: [] }],
resolveBundle: () => bundle,
runtime,
});
expect(summary.items[0].compatibility.status).toBe('permissive');
expect(summary.items[0].outcome).toBe('skipped');
expect(summary.needsRepair).toHaveLength(1);
});
});
25 changes: 24 additions & 1 deletion packages/extensions/src/host/sdk-revalidate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,29 @@ export interface RevalidationSummary {
needsRepair: RevalidationItem[];
}

/**
* Whether a revalidation row belongs in the repair queue: a failed
* test run, or a row we could not self-verify at all.
*
* `skipped` only ever arises for 'outdated' or 'permissive' rows (the
* 'compatible' branch in `revalidateAgainstSdk` always resolves to
* 'pass' without touching the test runner), so any skip means we could
* not self-verify a row the compatibility check itself flagged as
* needing a re-test. Narrowing to 'outdated' silently dropped
* 'permissive' extensions (e.g. wildcard engine ranges) that crossed a
* major SDK bump with no declared tests to confirm they still work.
*
* This is the single definition of that rule. Both the
* `RevalidationSummary.needsRepair` bucket and the viewer's repair
* panel — which decides per row whether to render a Repair button —
* call it, so the queue count and the actionable rows cannot disagree.
* They previously each carried their own copy of the predicate, and
* updating one of them was exactly how they came apart.
*/
export function needsSdkRepair(item: RevalidationItem): boolean {
return item.outcome === 'fail' || item.outcome === 'skipped';
}

export interface RevalidateOptions {
/** SDK version we're moving TO. */
sdk: string;
Expand Down Expand Up @@ -114,7 +137,7 @@ export async function revalidateAgainstSdk(
});
}

const needsRepair = items.filter((i) => i.outcome === 'fail' || (i.outcome === 'skipped' && i.compatibility.status === 'outdated'));
const needsRepair = items.filter(needsSdkRepair);
return { sdk: opts.sdk, items, needsRepair };
}

Expand Down
1 change: 1 addition & 0 deletions scripts/api-surface.json
Original file line number Diff line number Diff line change
Expand Up @@ -1790,6 +1790,7 @@
"migrateManifest: function",
"migrateSavedScripts: function",
"mineSequences: function",
"needsSdkRepair: function",
"opMatchesScopeClaim: function",
"overallTier: function",
"overlayParagraphDiff: function",
Expand Down
Loading