From eb7ae18f1407afbd50abf7efca19a4f397fd6acd Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 18:10:22 +0000 Subject: [PATCH] fix(tooling): address review follow-ups Keep concurrent example checks portable and adequately budgeted, retain fast cleanup coverage in PR gates, and document Semaphore ordering accurately. --- docs/effect-conventions.md | 9 ++- package.json | 2 +- .../tests/examples-check-script.test.ts | 60 +++++++++++++++ ...runtime-playground-capture-cleanup.test.ts | 75 +++++++++++++++++++ .../tests/runtime-playground-capture.test.ts | 72 +----------------- rstest.integration-tests.ts | 9 ++- scripts/run-examples-check.mjs | 30 ++++++++ 7 files changed, 179 insertions(+), 78 deletions(-) create mode 100644 packages/agent-bundle/tests/examples-check-script.test.ts create mode 100644 packages/workbench/tests/runtime-playground-capture-cleanup.test.ts create mode 100644 scripts/run-examples-check.mjs diff --git a/docs/effect-conventions.md b/docs/effect-conventions.md index 65d1b4463..2af53825e 100644 --- a/docs/effect-conventions.md +++ b/docs/effect-conventions.md @@ -124,9 +124,12 @@ end-to-end. Helped: -- `Semaphore.make(1)` + `withPermit` is a drop-in for the hand-rolled serial - queues (FIFO waiters), including the process-wide per-project lease mutex - map in the epoch store. +- `Semaphore.make(1)` + `withPermit` replaces hand-rolled mutual exclusion + where admission order is not observable, including the process-wide + per-project lease mutex map in the epoch store. It bounds access and + releases the permit when the guarded effect exits; the public API does not + guarantee FIFO waiter admission. Keep an explicit queue and ordering test + wherever submission order is part of the contract. - `Deferred` gives coalesced rebuild waiters one shared completion; `Effect.onExit` sits exactly where a `.finally` drain hook sat. - `Effect.forEach(..., { concurrency: 'unbounded' })` with per-element diff --git a/package.json b/package.json index bf2ae7410..0c25cdcb2 100644 --- a/package.json +++ b/package.json @@ -43,7 +43,7 @@ "example:audiobook": "pnpm build && pnpm --filter @agent-bundle-example/audiobook-curator dev", "example:mcp-app": "pnpm build && pnpm --filter @agent-bundle-example/mcp-app dev", "example:skills": "pnpm build && pnpm --filter @agent-bundle-example/skills-starter dev", - "examples:check": "pnpm build && AGENT_BUNDLE_TEST_TIME_SCALE=${AGENT_BUNDLE_TEST_TIME_SCALE:-2} pnpm --filter './examples/*' --workspace-concurrency=3 check" + "examples:check": "pnpm build && node scripts/run-examples-check.mjs" }, "devDependencies": { "@arethetypeswrong/cli": "0.18.5", diff --git a/packages/agent-bundle/tests/examples-check-script.test.ts b/packages/agent-bundle/tests/examples-check-script.test.ts new file mode 100644 index 000000000..33906ca83 --- /dev/null +++ b/packages/agent-bundle/tests/examples-check-script.test.ts @@ -0,0 +1,60 @@ +import { execFile as executeFile } from 'node:child_process'; +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { promisify } from 'node:util'; + +import { expect, it } from '@rstest/core'; + +const execFile = promisify(executeFile); +const workspaceRoot = process.cwd(); +const examplesCheckScript = join(workspaceRoot, 'scripts', 'run-examples-check.mjs'); + +it('runs pnpm without shell syntax and floors the example time scale at two', async () => { + const fixtureRoot = await mkdtemp(join(tmpdir(), 'agent-bundle-examples-check-')); + const fakePnpm = join(fixtureRoot, 'fake-pnpm.mjs'); + const capturePath = join(fixtureRoot, 'capture.json'); + await writeFile(fakePnpm, ` +import { writeFile } from 'node:fs/promises'; + +await writeFile(process.env.FAKE_PNPM_CAPTURE, JSON.stringify({ + args: process.argv.slice(2), + timeScale: process.env.AGENT_BUNDLE_TEST_TIME_SCALE, +})); +`, 'utf8'); + + try { + for (const [requestedScale, expectedScale] of [ + [undefined, '2'], + ['1', '2'], + ['invalid', '2'], + ['4', '4'], + ] as const) { + const environment: NodeJS.ProcessEnv = { + ...process.env, + FAKE_PNPM_CAPTURE: capturePath, + npm_execpath: fakePnpm, + }; + if (requestedScale === undefined) { + delete environment.AGENT_BUNDLE_TEST_TIME_SCALE; + } else { + environment.AGENT_BUNDLE_TEST_TIME_SCALE = requestedScale; + } + + await execFile(process.execPath, [examplesCheckScript], { + cwd: workspaceRoot, + env: environment, + }); + const capture = JSON.parse(await readFile(capturePath, 'utf8')) as { + readonly args: readonly string[]; + readonly timeScale: string; + }; + expect(capture).toEqual({ + args: ['--filter', './examples/*', '--workspace-concurrency=3', 'check'], + timeScale: expectedScale, + }); + } + } finally { + await rm(fixtureRoot, { force: true, recursive: true }); + } +}); diff --git a/packages/workbench/tests/runtime-playground-capture-cleanup.test.ts b/packages/workbench/tests/runtime-playground-capture-cleanup.test.ts new file mode 100644 index 000000000..51d74b4f3 --- /dev/null +++ b/packages/workbench/tests/runtime-playground-capture-cleanup.test.ts @@ -0,0 +1,75 @@ +import { mkdtemp, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { expect, test } from '@rstest/core'; + +// @ts-expect-error The executable capture script is intentionally imported as the cleanup-boundary test seam. +import { atomically, cleanupCaptureResources, captureFailureAfterCleanup, formatCaptureFailure } from '../scripts/capture-runtime-playground.mjs'; + +test('settles every capture cleanup action without masking the primary failure', async () => { + const outputRoot = await mkdtemp(join(tmpdir(), 'agent-bundle-runtime-capture-cleanup-')); + const temporary = join(outputRoot, '.desktop.png.temporary'); + const primary = new Error('primary capture failed'); + const order: string[] = []; + let fixtureCloseCount = 0; + try { + await expect(atomically(temporary, async (path: string) => { + await writeFile(path, 'partial output', 'utf8'); + throw primary; + })).rejects.toBe(primary); + + const cleanup = await cleanupCaptureResources({ + browser: { + close: async () => { + order.push('browser.close'); + throw new Error('browser close rejected'); + }, + }, + fixture: { + close: async () => { + fixtureCloseCount += 1; + order.push('fixture.close'); + throw new Error('fixture close rejected with fixture-secret'); + }, + }, + restores: [ + async () => { + order.push('restore-one'); + throw new Error('restore one rejected'); + }, + async () => { + order.push('restore-two'); + }, + ], + }); + + expect(order).toEqual(['restore-one', 'restore-two', 'browser.close', 'fixture.close']); + expect(fixtureCloseCount).toBe(1); + expect(cleanup).toEqual({ attemptedRestores: 2, failedSteps: ['restore-1', 'browser.close', 'fixture.close'] }); + await expect(stat(temporary)).rejects.toMatchObject({ code: 'ENOENT' }); + + const failure = captureFailureAfterCleanup(primary, cleanup); + expect(failure).toBeInstanceOf(AggregateError); + expect(failure).toMatchObject({ message: 'primary capture failed' }); + expect((failure as AggregateError).errors[0]).toBe(primary); + expect((failure as AggregateError).errors[1]).toMatchObject({ message: 'Capture cleanup failed: restore-1, browser.close, fixture.close.' }); + const formatted = formatCaptureFailure(failure); + expect(formatted).toBe('primary capture failed\nCapture cleanup failed: restore-1, browser.close, fixture.close.'); + expect(formatted).not.toContain('restore one rejected'); + expect(formatted).not.toContain('browser close rejected'); + expect(formatted).not.toContain('fixture-secret'); + } finally { + await rm(outputRoot, { force: true, recursive: true }); + } +}); + +test('bounds a wedged cleanup step instead of holding the capture process open', async () => { + const cleanup = await cleanupCaptureResources({ + browser: { close: async () => new Promise(() => {}) }, + fixture: { close: async () => {} }, + restores: [async () => new Promise(() => {})], + stepTimeout: 50, + }); + expect(cleanup).toEqual({ attemptedRestores: 1, failedSteps: ['restore-1', 'browser.close'] }); +}); diff --git a/packages/workbench/tests/runtime-playground-capture.test.ts b/packages/workbench/tests/runtime-playground-capture.test.ts index 9bbfc8b4b..449f1788b 100644 --- a/packages/workbench/tests/runtime-playground-capture.test.ts +++ b/packages/workbench/tests/runtime-playground-capture.test.ts @@ -1,14 +1,11 @@ import { execFile as executeFile } from 'node:child_process'; -import { mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'; +import { mkdtemp, readFile, readdir, rm, stat } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { promisify } from 'node:util'; import { expect, test } from '@rstest/core'; -// @ts-expect-error The executable capture script is intentionally imported as the cleanup-boundary test seam. -import { atomically, cleanupCaptureResources, captureFailureAfterCleanup, formatCaptureFailure } from '../scripts/capture-runtime-playground.mjs'; - const execFile = promisify(executeFile); const workspaceRoot = process.cwd(); const captureScript = join(workspaceRoot, 'packages', 'workbench', 'scripts', 'capture-runtime-playground.mjs'); @@ -64,73 +61,6 @@ const expectPng = async (path: string, width: number, height: number): Promise { - const outputRoot = await mkdtemp(join(tmpdir(), 'agent-bundle-runtime-capture-cleanup-')); - const temporary = join(outputRoot, '.desktop.png.temporary'); - const primary = new Error('primary capture failed'); - const order: string[] = []; - let fixtureCloseCount = 0; - try { - await expect(atomically(temporary, async (path: string) => { - await writeFile(path, 'partial output', 'utf8'); - throw primary; - })).rejects.toBe(primary); - - const cleanup = await cleanupCaptureResources({ - browser: { - close: async () => { - order.push('browser.close'); - throw new Error('browser close rejected'); - }, - }, - fixture: { - close: async () => { - fixtureCloseCount += 1; - order.push('fixture.close'); - throw new Error('fixture close rejected with fixture-secret'); - }, - }, - restores: [ - async () => { - order.push('restore-one'); - throw new Error('restore one rejected'); - }, - async () => { - order.push('restore-two'); - }, - ], - }); - - expect(order).toEqual(['restore-one', 'restore-two', 'browser.close', 'fixture.close']); - expect(fixtureCloseCount).toBe(1); - expect(cleanup).toEqual({ attemptedRestores: 2, failedSteps: ['restore-1', 'browser.close', 'fixture.close'] }); - await expect(stat(temporary)).rejects.toMatchObject({ code: 'ENOENT' }); - - const failure = captureFailureAfterCleanup(primary, cleanup); - expect(failure).toBeInstanceOf(AggregateError); - expect(failure).toMatchObject({ message: 'primary capture failed' }); - expect((failure as AggregateError).errors[0]).toBe(primary); - expect((failure as AggregateError).errors[1]).toMatchObject({ message: 'Capture cleanup failed: restore-1, browser.close, fixture.close.' }); - const formatted = formatCaptureFailure(failure); - expect(formatted).toBe('primary capture failed\nCapture cleanup failed: restore-1, browser.close, fixture.close.'); - expect(formatted).not.toContain('restore one rejected'); - expect(formatted).not.toContain('browser close rejected'); - expect(formatted).not.toContain('fixture-secret'); - } finally { - await rm(outputRoot, { force: true, recursive: true }); - } -}); - -test('bounds a wedged cleanup step instead of holding the capture process open', async () => { - const cleanup = await cleanupCaptureResources({ - browser: { close: async () => new Promise(() => {}) }, - fixture: { close: async () => {} }, - restores: [async () => new Promise(() => {})], - stepTimeout: 50, - }); - expect(cleanup).toEqual({ attemptedRestores: 1, failedSteps: ['restore-1', 'browser.close'] }); -}); - test('captures identity-backed HMR, last-good, recovery, and desktop browser evidence', { timeout: 600_000 }, async () => { const outputRoot = await mkdtemp(join(tmpdir(), 'agent-bundle-runtime-capture-')); const outputs = Object.freeze({ diff --git a/rstest.integration-tests.ts b/rstest.integration-tests.ts index 4a7592001..03207cdde 100644 --- a/rstest.integration-tests.ts +++ b/rstest.integration-tests.ts @@ -27,6 +27,7 @@ export const integrationTestFiles: readonly string[] = [ 'packages/agent-bundle/tests/eval-harness.test.ts', 'packages/agent-bundle/tests/eval-service.test.ts', 'packages/agent-bundle/tests/eval-workbench.test.ts', + 'packages/agent-bundle/tests/examples-check-script.test.ts', 'packages/agent-bundle/tests/examples-contract.test.ts', 'packages/agent-bundle/tests/generated-route-server.test.ts', 'packages/agent-bundle/tests/hook-playground-service.test.ts', @@ -62,6 +63,7 @@ export const integrationTestFiles: readonly string[] = [ 'packages/workbench/tests/rsbuild-workbench.test.ts', 'packages/workbench/tests/runtime-inspector.test.ts', 'packages/workbench/tests/runtime-consent-dialog.test.ts', + 'packages/workbench/tests/runtime-playground-capture-cleanup.test.ts', 'packages/workbench/tests/runtime-playground.e2e.test.ts', 'packages/workbench/tests/runtime-playground-hmr.e2e.test.ts', 'packages/workbench/tests/workbench-dev-command.test.ts', @@ -73,9 +75,10 @@ export const integrationTestFiles: readonly string[] = [ * behavioral proof. The behavioral contracts they exercise (HMR activation, * last-good retention, recovery) are already covered per PR by * runtime-playground.e2e.test.ts and runtime-playground-hmr.e2e.test.ts in - * the integration pool, so these run through the root `test:evidence` - * script in CI's nightly schedule instead — evidence regenerates when the - * flow changes, not on every PR (#128). + * the integration pool. Fast capture cleanup contracts stay per PR in + * runtime-playground-capture-cleanup.test.ts. The evidence journey runs + * through the root `test:evidence` script in CI's nightly schedule instead — + * evidence regenerates when the flow changes, not on every PR (#128). */ export const nightlyEvidenceTestFiles: readonly string[] = [ 'packages/workbench/tests/runtime-playground-capture.test.ts', diff --git a/scripts/run-examples-check.mjs b/scripts/run-examples-check.mjs new file mode 100644 index 000000000..43428178d --- /dev/null +++ b/scripts/run-examples-check.mjs @@ -0,0 +1,30 @@ +import { spawn } from 'node:child_process'; + +const requestedTimeScale = Number(process.env.AGENT_BUNDLE_TEST_TIME_SCALE ?? ''); +const timeScale = Number.isSafeInteger(requestedTimeScale) && requestedTimeScale >= 1 + ? Math.max(requestedTimeScale, 2) + : 2; +const pnpmEntrypoint = process.env.npm_execpath; + +if (pnpmEntrypoint === undefined || pnpmEntrypoint.length === 0) { + throw new Error('run-examples-check.mjs must be launched through a pnpm package script.'); +} + +const child = spawn(process.execPath, [ + pnpmEntrypoint, + '--filter', + './examples/*', + '--workspace-concurrency=3', + 'check', +], { + env: { + ...process.env, + AGENT_BUNDLE_TEST_TIME_SCALE: String(timeScale), + }, + stdio: 'inherit', +}); + +process.exitCode = await new Promise((resolve, reject) => { + child.once('error', reject); + child.once('close', (code) => resolve(code ?? 1)); +});