Skip to content

ci: add PR build/typecheck/test workflow + fix latent type errors #414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zknpr merged 1 commit into from
May 30, 2026
Merged

ci: add PR build/typecheck/test workflow + fix latent type errors #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 37 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
name: CI

# Runs on every PR to main and on pushes to main, so type errors and test
# failures are caught before merge (release.yml only runs on version tags).
on:
pull_request:
branches: [main]
push:
branches: [main]

concurrency:
group: ci-${{ github.ref }}
cancel-in-progress: true

Comment on lines +1 to +14
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add explicit permissions block to follow least privilege.

The workflow inherits default GITHUB_TOKEN permissions, which are overly broad. This workflow only needs read access to check out the repository.

🔒 Proposed fix to add minimal permissions 
 name: CI
 
+permissions:
+  contents: read
+
 # Runs on every PR to main and on pushes to main, so type errors and test
 # failures are caught before merge (release.yml only runs on version tags).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
name: CI
# Runs on every PR to main and on pushes to main, so type errors and test
# failures are caught before merge (release.yml only runs on version tags).
on:
pull_request:
branches: [main]
push:
branches: [main]
concurrency:
group: ci-${{ github.ref }}
cancel-in-progress: true
name: CI
permissions:
contents: read
# Runs on every PR to main and on pushes to main, so type errors and test
# failures are caught before merge (release.yml only runs on version tags).
on:
pull_request:
branches: [main]
push:
branches: [main]
concurrency:
group: ci-${{ github.ref }}
cancel-in-progress: true
🧰 Tools
🪛 zizmor (1.25.2)

[warning] 1-38: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block

(excessive-permissions)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 1 - 14, The workflow "name: CI"
currently relies on the default GITHUB_TOKEN permissions; add an explicit
top-level permissions block to grant least privilege for the checkout operation
(e.g., set GITHUB_TOKEN permissions to at most contents: read) so the job only
has repository read access; update the YAML near the top-level (in the same
document that contains "name: CI" and "on:") to include the permissions mapping
rather than using defaults.

jobs:
build-typecheck-test:
name: Build, typecheck & test
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Pin actions to commit hashes per security policy.

The workflow uses version tags (@v4) instead of commit hashes. Static analysis flags this as required by policy to prevent supply chain attacks if the action repositories are compromised.

🔒 Guidance for pinning to commit hashes

Replace version tags with full commit SHA hashes. For example:

-      - uses: actions/checkout@v4
+      - uses: actions/checkout@<commit-sha>  # v4.x.x
 
-      - uses: actions/setup-node@v4
+      - uses: actions/setup-node@<commit-sha>  # v4.x.x

Look up the latest commit hashes for the v4 releases of these actions in their respective repositories, and add a comment with the version number for future reference.

Also applies to: 22-22

🧰 Tools
🪛 zizmor (1.25.2)

[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml at line 20, Replace the floating tag references to
GitHub Actions with pinned commit SHAs: locate the "uses: actions/checkout@v4"
entry (and the other similar "uses:" entry around line 22) and replace the `@v4`
tag with the corresponding full commit SHA for that action's v4 release; include
a trailing comment noting the human-readable version (e.g., v4) for future
reference, and ensure both occurrences are updated so the workflow pins to
immutable commit hashes rather than floating tags.


- uses: actions/setup-node@v4
with:
node-version: 22
cache: npm

- name: Install dependencies
run: npm ci

- name: Build extension, worker & WASM
run: node scripts/build.mjs

- name: Typecheck (src + tests)
run: npx tsc --noEmit -p tsconfig.json

- name: Unit tests
run: npm test

Check warning

Code scanning / CodeQL

Workflow does not contain permissions Medium

Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read}
Comment on lines +17 to +37
2 changes: 1 addition & 1 deletion src/hostBridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ export class HostBridge implements ToastService {
return update;
});

if ('updateCellBatch' in dbOps) {
if (typeof dbOps.updateCellBatch === 'function') {
await dbOps.updateCellBatch(table, processedUpdates);
} else {
// Fallback: execute updates sequentially to avoid IPC overload and N+1 concurrency,
Expand Down
2 changes: 1 addition & 1 deletion tests/benchmarks/native_worker_ddl_batch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as path from 'path';

async function runBenchmark() {
const bundle = await createNativeDatabaseConnection(vscode.Uri.file(process.cwd()));
const loadResult = await bundle.loadDatabase({ buffer: new Uint8Array() });
const loadResult = await (bundle as any).loadDatabase({ buffer: new Uint8Array() });
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

high

The createNativeDatabaseConnection function returns a connection bundle that uses establishConnection rather than loadDatabase. Calling loadDatabase here will result in a runtime TypeError because the method does not exist on the native worker bundle.

Instead of casting to any and calling loadDatabase, use establishConnection with an in-memory database URI to match the correct API.

Suggested change
const loadResult = await (bundle as any).loadDatabase({ buffer: new Uint8Array() });
const loadResult = await bundle.establishConnection(vscode.Uri.file(':memory:'), 'bench_db');

const db = loadResult.databaseOps;

// create table
Expand Down
2 changes: 1 addition & 1 deletion tests/performance/index_drop_benchmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async function runBenchmark() {

try {
const wasmBinary = fs.readFileSync(path.resolve(__dirname, '../../node_modules/sql.js/dist/sql-wasm.wasm'));
const engineResult = await createDatabaseEngine({ wasmBinary });
const engineResult = await createDatabaseEngine({ wasmBinary } as any);
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

medium

Instead of casting the configuration object to any, you can satisfy the DatabaseInitConfig interface by providing the required content and maxSize properties with safe default values. This preserves type safety and avoids bypassing compiler checks.

Suggested change
const engineResult = await createDatabaseEngine({ wasmBinary } as any);
const engineResult = await createDatabaseEngine({ wasmBinary, content: null, maxSize: 0 });

const db = engineResult.operations as WasmDatabaseEngine;

const numIndexes = 50;
Expand Down
4 changes: 2 additions & 2 deletions tests/performance/native_undo_redo_benchmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ async function runBenchmark() {
const start = performance.now();

// 1. Undo (Adds columns back)
await databaseOps.undoModification(mod);
await databaseOps.undoModification(mod as any);

// 2. Redo (Drops columns again)
await databaseOps.redoModification(mod);
await databaseOps.redoModification(mod as any);
Comment on lines +51 to +54
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

medium

The mod object is missing the required description property of the ModificationEntry interface, which is why the compiler complained.

Instead of casting mod as any, you can spread mod and supply a dummy description to make it a fully valid ModificationEntry in a type-safe manner.

Suggested change
await databaseOps.undoModification(mod as any);
// 2. Redo (Drops columns again)
await databaseOps.redoModification(mod);
await databaseOps.redoModification(mod as any);
await databaseOps.undoModification({ ...mod, description: 'Undo column drop' });
// 2. Redo (Drops columns again)
await databaseOps.redoModification({ ...mod, description: 'Redo column drop' });


const end = performance.now();
console.log(`Undo + Redo took ${(end - start).toFixed(2)}ms`);
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/hostBridge.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('HostBridge', () => {
await bridge.saveFile('../../../etc/passwd', new Uint8Array([1, 2, 3]));

assert.strictEqual(showSaveDialogMock.mock.callCount(), 1);
const args = showSaveDialogMock.mock.calls[0].arguments[0];
const args = showSaveDialogMock.mock.calls[0].arguments[0] as any;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

medium

Instead of casting args to any, you can cast it to vscode.SaveDialogOptions which is the actual type expected by showSaveDialog. This preserves type safety and auto-completion.

Suggested change
const args = showSaveDialogMock.mock.calls[0].arguments[0] as any;
const args = showSaveDialogMock.mock.calls[0].arguments[0] as vscode.SaveDialogOptions;

// The defaultUri path should end with the base name 'passwd', not the traversed path
assert.ok(args.defaultUri.path.endsWith('/dbDir/passwd'), `Expected safe path, got ${args.defaultUri.path}`);

Expand Down Expand Up @@ -135,7 +135,7 @@ describe('HostBridge', () => {
};
const mockProvider = { webviews: new Map(), context: {} };
const bridge = new HostBridge(mockProvider as any, mockDocument as any);
bridge.ensureDatabaseInitialized = () => dbOps as any;
(bridge as any).ensureDatabaseInitialized = () => dbOps as any;

await bridge.deleteRows('table1', [1]);

Expand All @@ -162,7 +162,7 @@ describe('HostBridge', () => {
};
const mockProvider = { webviews: new Map(), context: {} };
const bridge = new HostBridge(mockProvider as any, mockDocument as any);
bridge.ensureDatabaseInitialized = () => dbOps as any;
(bridge as any).ensureDatabaseInitialized = () => dbOps as any;

await bridge.deleteColumns('table1', ['col1']);

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ if (!mockVscode.extensions) {
(mockVscode.workspace as any).registerFileSystemProvider = () => ({ dispose: () => {} });
(mockVscode.window as any).createOutputChannel = () => ({ dispose: () => {} });
(mockVscode.window as any).registerCustomEditorProvider = () => ({ dispose: () => {} });
(mockVscode.ConfigurationTarget as any) = { Global: 1 };
(mockVscode as any).ConfigurationTarget = { Global: 1 };
(mockVscode.workspace as any).getConfiguration = () => {
return {
get: () => ({}),
Expand Down Expand Up @@ -170,7 +170,7 @@ describe('main.ts', () => {
await main.activate(mockContext);

assert.strictEqual(configUpdateMock.mock.calls.length, 1);
const callArgs = configUpdateMock.mock.calls[0].arguments;
const callArgs = configUpdateMock.mock.calls[0].arguments as any[];
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

medium

Instead of casting callArgs to any[], you can cast it to a more specific tuple type like [string, Record<string, any>] which matches the signature of workspace.getConfiguration().update. This keeps the test type-safe.

Suggested change
const callArgs = configUpdateMock.mock.calls[0].arguments as any[];
const callArgs = configUpdateMock.mock.calls[0].arguments as [string, Record<string, any>];

assert.strictEqual(callArgs[0], 'patterns');
// The actual value in codebase may be different, we should check it
assert.ok(callArgs[1]['*.sqlite']);
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/modification_tracker_api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('Undo/Redo Logic Coverage', () => {
tracker.stepBack(); // Undo 2
assert.strictEqual(tracker.canStepForward, true);

tracker.record({ label: '3', description: '3', modificationType: 'row_update', targetTable: 't1' });
tracker.record({ label: '3', description: '3', modificationType: 'cell_update', targetTable: 't1' });
assert.strictEqual(tracker.canStepForward, false);
assert.strictEqual(tracker.entryCount, 2);
});
Expand Down Expand Up @@ -67,7 +67,7 @@ describe('Undo/Redo Logic Coverage', () => {
await tracker.createCheckpoint();

tracker.record({ label: '2', description: '2', modificationType: 'row_delete', targetTable: 't1' });
tracker.record({ label: '3', description: '3', modificationType: 'row_update', targetTable: 't1' });
tracker.record({ label: '3', description: '3', modificationType: 'cell_update', targetTable: 't1' });

assert.strictEqual(tracker.entryCount, 3);
assert.strictEqual(tracker.hasUncommittedChanges(), true);
Expand Down Expand Up @@ -97,7 +97,7 @@ describe('Undo/Redo Logic Coverage', () => {
tracker.record({ label: '1', description: '1', modificationType: 'row_insert', targetTable: 't1' });
await tracker.createCheckpoint(); // checkpointIndex = 1

tracker.record({ label: '2', description: '2', modificationType: 'row_update', targetTable: 't1' });
tracker.record({ label: '2', description: '2', modificationType: 'cell_update', targetTable: 't1' });
// timeline: ['1', '2'], checkpointIndex: 1

// Add 3rd entry, should evict '1'
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/sqlite-db.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ describe('WasmDatabaseEngine', () => {
}
} as unknown as Database;

const engine = new WasmDatabaseEngine(mockDb, 5000);
const engine = new WasmDatabaseEngine(mockDb as any, 5000);
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 30, 2026

Choose a reason for hiding this comment

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

medium

Instead of casting mockDb to any, you can dynamically extract the correct type of the first parameter of the WasmDatabaseEngine constructor using Parameters<typeof WasmDatabaseEngine>[0]. This avoids using any and maintains strict type safety.

Suggested change
const engine = new WasmDatabaseEngine(mockDb as any, 5000);
const engine = new WasmDatabaseEngine(mockDb as unknown as Parameters<typeof WasmDatabaseEngine>[0], 5000);


const consoleWarnOrig = console.warn;
let warnedMessage = '';
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/tableExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ describe('exportTableCommand Fallback', () => {

let errorMessageShown = '';
const originalShowErrorMessage = mockVscode.window.showErrorMessage;
mockVscode.window.showErrorMessage = async (msg: string) => {
(mockVscode.window as any).showErrorMessage = async (msg: string) => {
errorMessageShown = msg;
};

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/virtualFileSystem.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ describe('SQLiteFileSystemProvider', () => {

assert.strictEqual(dbOps.updateCell.mock.callCount(), 1);
assert.deepStrictEqual(dbOps.updateCell.mock.calls[0].arguments, ['users', 1, 'col', 'Hello World']);
assert.strictEqual(doc.recordExternalModification.mock.callCount(), 1);
assert.strictEqual((doc.recordExternalModification as any).mock.callCount(), 1);
});

it('should write binary content if not valid UTF-8', async () => {
Expand All @@ -253,7 +253,7 @@ describe('SQLiteFileSystemProvider', () => {

assert.strictEqual(dbOps.updateCell.mock.callCount(), 1);
assert.deepStrictEqual(dbOps.updateCell.mock.calls[0].arguments, ['users', 1, 'col', content]);
assert.strictEqual(doc.recordExternalModification.mock.callCount(), 1);
assert.strictEqual((doc.recordExternalModification as any).mock.callCount(), 1);
});
});

Expand Down
6 changes: 3 additions & 3 deletions tests/unit/webviewMessageHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe('WebviewMessageHandler', () => {
const hostBridge = {
echo: (val: unknown) => val
};
let sentMessage: unknown = null;
let sentMessage: any = null;
const postMessage = async (msg: unknown) => {
sentMessage = msg;
return true;
Expand Down Expand Up @@ -45,7 +45,7 @@ describe('WebviewMessageHandler', () => {

it('should handle unknown method', () => {
const hostBridge = {};
let sentMessage: unknown = null;
let sentMessage: any = null;
const postMessage = async (msg: unknown) => {
sentMessage = msg;
return true;
Expand All @@ -71,7 +71,7 @@ describe('WebviewMessageHandler', () => {
const hostBridge = {
echo: (val: unknown) => val
};
let sentMessage: unknown = null;
let sentMessage: any = null;
const postMessage = async (msg: unknown) => {
sentMessage = msg;
return true;
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/workerFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ describe('workerFactory error path tests', () => {
mockVscode.workspace.fs = {
readFile: async () => new Uint8Array(),
stat: async () => ({ size: 0 })
};
mockVscode.Uri.joinPath = () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' });
} as any;
(mockVscode.Uri as any).joinPath = () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' });
Comment on lines 87 to +91
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 30, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider using Object.defineProperty for readonly VS Code API fields.

The coding guidelines recommend using Object.defineProperty(obj, prop, { value, writable: true, configurable: true }) for setting readonly VS Code API fields in unit tests, rather than as any casts. This provides better consistency and clarity when working with readonly properties like workspace.fs.

As per coding guidelines, "Use Object.defineProperty(obj, prop, { value, writable: true, configurable: true }) for setting readonly VS Code API fields like vscode.env.uiKind in unit tests".

♻️ Proposed refactor using Object.defineProperty 
-    mockVscode.workspace.fs = {
-      readFile: async () => new Uint8Array(),
-      stat: async () => ({ size: 0 })
-    } as any;
-    (mockVscode.Uri as any).joinPath = () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' });
+    Object.defineProperty(mockVscode.workspace, 'fs', {
+      value: {
+        readFile: async () => new Uint8Array(),
+        stat: async () => ({ size: 0 })
+      },
+      writable: true,
+      configurable: true
+    });
+    Object.defineProperty(mockVscode.Uri, 'joinPath', {
+      value: () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' }),
+      writable: true,
+      configurable: true
+    });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mockVscode.workspace.fs = {
readFile: async () => new Uint8Array(),
stat: async () => ({ size: 0 })
};
mockVscode.Uri.joinPath = () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' });
} as any;
(mockVscode.Uri as any).joinPath = () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' });
Object.defineProperty(mockVscode.workspace, 'fs', {
value: {
readFile: async () => new Uint8Array(),
stat: async () => ({ size: 0 })
},
writable: true,
configurable: true
});
Object.defineProperty(mockVscode.Uri, 'joinPath', {
value: () => ({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' }),
writable: true,
configurable: true
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/workerFactory.test.ts` around lines 87 - 91, Replace the direct
assignment and casts to readonly VS Code API fields by defining properties via
Object.defineProperty: instead of assigning mockVscode.workspace.fs = { ... } as
any, call Object.defineProperty(mockVscode, 'workspace', { value: { fs: {
readFile: async () => new Uint8Array(), stat: async () => ({ size: 0 }) } },
writable: true, configurable: true }) or define just the 'fs' property on
mockVscode.workspace similarly so that mockVscode.workspace.fs is
writable/configurable; likewise replace (mockVscode.Uri as any).joinPath = () =>
({ scheme: 'file', fsPath: '/test/path/assets/sqlite3.wasm' }) with
Object.defineProperty(mockVscode.Uri, 'joinPath', { value: () => ({ scheme:
'file', fsPath: '/test/path/assets/sqlite3.wasm' }), writable: true,
configurable: true }) to avoid using as any and follow readonly API testing
conventions while keeping functions names mockVscode.workspace.fs and
mockVscode.Uri.joinPath as the targets.

});

afterEach(() => {
Expand Down
Loading