diff --git a/AGENTS.md b/AGENTS.md index 89c3cf908..9eef4bfdb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,7 +46,7 @@ Before a PR run `bun run check`, `bun run format`, `bun run test:unit`, and `bun - Across package/app boundaries import the owning package's public exports, never workspace internals or forwarding-only shims. `@open-pencil/scene-graph` owns graph types and primitives; `@open-pencil/kiwi` owns low-level Kiwi/FIG helpers; `@open-pencil/core` provides the compatibility barrel plus the subpaths listed in `packages/core/package.json`. - `bun run check:arch` enforces: public workspace exports, framework-neutral Core, no app services in views or shared UI, property-panel internals scoped to that panel. - Package aliases are `#core/*`, `#fig/*`, `#vue/*`, `#cli/*`, `#mcp/*`, `#dom-css/*`, `#design-jsx/*`, `#emit/*`; a package's own tests use `#core-tests/*` and `#fig-tests/*`; the app uses `@/`. Prefer clear relative imports nearby. Never escape an alias root with `../` (for example `#tests/../vite`); fix module ownership instead. -- Never drill with `../../`: one `../` to a sibling folder is fine, two or more means an alias. This holds in tests as much as in source — a test reaches its package's source through `#/*` and its own helpers through `#-tests/*`. `open-pencil/no-deep-parent-relative-imports` enforces it; a new test directory must be added to `lint:structure` so the rule reaches it. +- Never drill with `../../`: one `../` to a sibling folder is fine, two or more means an alias. This holds in tests as much as in source — a test reaches its package's source through `#/*` and its own helpers through `#-tests/*`. `open-pencil/no-deep-parent-relative-imports` enforces it for imports and `open-pencil/no-deep-parent-relative-paths` for paths built from `import.meta`; a new test directory must be added to `lint:structure` so the rules reach it. - Reuse named types from `@open-pencil/scene-graph`; do not respell `Color`, `Vector`, `SceneNode`, `Effect`, `Fill`, or `Stroke`. ## Code conventions diff --git a/oxlint.json b/oxlint.json index a45e5fd19..9cfa6caac 100644 --- a/oxlint.json +++ b/oxlint.json @@ -180,6 +180,7 @@ "open-pencil/no-core-framework-imports": "error", "open-pencil/no-direct-storage-access": "error", "open-pencil/no-deep-parent-relative-imports": "error", + "open-pencil/no-deep-parent-relative-paths": "error", "open-pencil/no-broad-double-cast": "error", "open-pencil/no-unknown-record-double-cast": "error", "open-pencil/no-broad-unknown-type-assertions": "error", diff --git a/packages/cli/tests/helpers/paths.ts b/packages/cli/tests/helpers/paths.ts index b01f3bb6f..59ba6458d 100644 --- a/packages/cli/tests/helpers/paths.ts +++ b/packages/cli/tests/helpers/paths.ts @@ -1,7 +1,17 @@ -import { resolve } from 'node:path' +import { existsSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' /** The CLI entry point, for tests that run it as a subprocess. */ export const CLI_ENTRY = Bun.resolveSync('#cli/index.ts', import.meta.dir) +/** The workspace root, found by its lockfile rather than by counting directories. */ +export function workspaceRoot(from = dirname(fileURLToPath(import.meta.url))): string { + for (let dir = from; ; dir = dirname(dir)) { + if (existsSync(join(dir, 'bun.lock'))) return dir + if (dirname(dir) === dir) throw new Error(`No workspace root above ${from}`) + } +} + /** Fixtures shared across the repository; a module import would escape the package root. */ -export const FIXTURES = resolve(import.meta.dir, '../../../../tests/fixtures') +export const FIXTURES = join(workspaceRoot(), 'tests/fixtures') diff --git a/packages/core/tests/helpers/fig/fixtures.ts b/packages/core/tests/helpers/fig/fixtures.ts index 76159eb21..be2cedcc3 100644 --- a/packages/core/tests/helpers/fig/fixtures.ts +++ b/packages/core/tests/helpers/fig/fixtures.ts @@ -10,9 +10,11 @@ import { import type { NodeChange } from '@open-pencil/kiwi/fig/codec' import * as v from 'valibot' +import { FIXTURES } from '../paths' + import { collectAllNodes } from './traversal' -export const FIXTURES = resolve(import.meta.dir, '../../../../../tests/fixtures') +export { FIXTURES } export const VALID_NODE_TYPES = new Set([ 'CANVAS', diff --git a/packages/core/tests/helpers/paths.ts b/packages/core/tests/helpers/paths.ts new file mode 100644 index 000000000..d4c7c3231 --- /dev/null +++ b/packages/core/tests/helpers/paths.ts @@ -0,0 +1,19 @@ +import { existsSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' + +/** The workspace root, found by its lockfile rather than by counting directories. */ +export function workspaceRoot(from = dirname(fileURLToPath(import.meta.url))): string { + for (let dir = from; ; dir = dirname(dir)) { + if (existsSync(join(dir, 'bun.lock'))) return dir + if (dirname(dir) === dir) throw new Error(`No workspace root above ${from}`) + } +} + +/** Fixtures shared across the repository; a module import would escape the package root. */ +export const FIXTURES = join(workspaceRoot(), 'tests/fixtures') + +/** A file inside this package, for assets that are not modules. */ +export function corePackagePath(...segments: string[]): string { + return join(dirname(Bun.resolveSync('@open-pencil/core/package.json', import.meta.dir)), ...segments) +} diff --git a/packages/core/tests/io/formats/fig/roundtrip/glyph-blob.test.ts b/packages/core/tests/io/formats/fig/roundtrip/glyph-blob.test.ts index 069a67bdb..1c7d87f4b 100644 --- a/packages/core/tests/io/formats/fig/roundtrip/glyph-blob.test.ts +++ b/packages/core/tests/io/formats/fig/roundtrip/glyph-blob.test.ts @@ -10,8 +10,9 @@ import { expectDefined } from '#core-tests/helpers/assert' import { HEAVY_TEST_TIMEOUT_MS } from '#core-tests/helpers/test-utils' import { FIXTURES } from '#core-tests/helpers/fig/fixtures' +import { corePackagePath } from '#core-tests/helpers/paths' -const INTER_ASSETS = resolve(import.meta.dir, '../../../../../assets') +const INTER_ASSETS = corePackagePath('assets') function countGlyphBlobs(bytes: Uint8Array) { const parsed = parseFigBuffer(new Uint8Array(bytes).buffer) diff --git a/packages/core/tests/io/formats/fig/session/worker-reader.test.ts b/packages/core/tests/io/formats/fig/session/worker-reader.test.ts index 14581d5d1..bb1ba40fe 100644 --- a/packages/core/tests/io/formats/fig/session/worker-reader.test.ts +++ b/packages/core/tests/io/formats/fig/session/worker-reader.test.ts @@ -15,7 +15,7 @@ test.each(['first-page', 'none'] as const)( graph.createNode('TEXT', graph.addPage('Second').id, { text: 'Second' }) const bytes = await exportFigFile(graph) const worker = new Worker( - new URL('../../../../../src/kiwi/fig/session/worker.ts', import.meta.url), + import.meta.resolve('#core/kiwi/fig/session/worker'), { type: 'module' } ) const channel = new MessageChannel() diff --git a/packages/fig/tests/helpers/fig-fixtures.ts b/packages/fig/tests/helpers/fig-fixtures.ts index 217973085..75a351f9e 100644 --- a/packages/fig/tests/helpers/fig-fixtures.ts +++ b/packages/fig/tests/helpers/fig-fixtures.ts @@ -1,8 +1,9 @@ import { readFileSync } from 'node:fs' import { resolve } from 'node:path' -/** Shared archives live outside the package; this is the only path that reaches them. */ -export const FIXTURES = resolve(import.meta.dir, '../../../../tests/fixtures') +import { FIXTURES } from './paths' + +export { FIXTURES } export function readFixtureBytes(name: string): Uint8Array { return readFileSync(resolve(FIXTURES, name)) diff --git a/packages/fig/tests/helpers/paths.ts b/packages/fig/tests/helpers/paths.ts new file mode 100644 index 000000000..d0b248dbe --- /dev/null +++ b/packages/fig/tests/helpers/paths.ts @@ -0,0 +1,14 @@ +import { existsSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' + +/** The workspace root, found by its lockfile rather than by counting directories. */ +export function workspaceRoot(from = dirname(fileURLToPath(import.meta.url))): string { + for (let dir = from; ; dir = dirname(dir)) { + if (existsSync(join(dir, 'bun.lock'))) return dir + if (dirname(dir) === dir) throw new Error(`No workspace root above ${from}`) + } +} + +/** Shared archives live outside the package; this is the only path that reaches them. */ +export const FIXTURES = join(workspaceRoot(), 'tests/fixtures') diff --git a/tests/AGENTS.md b/tests/AGENTS.md index c26f27dc4..a2f49daec 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -7,6 +7,7 @@ - Package-local tests mirror source domains under `packages//tests/`; central app tests mirror `src/app/**` under `tests/app/`; `tests/integration/` requires a genuinely cross-owner contract; E2E under `tests/e2e/` follows user workflows; native and Figma acceptance are explicit exceptions. - Existing `tests/engine/**` domains migrate together with runner discovery: `tools/dev/unit-tests/src/shards.ts` lists each owner's canonical home and its current `tests/engine` directories, so a move is a `git mv` plus imports. `bun run check:test-homes` rejects any test added under `tests/engine` and any stale entry in `tools/checks/test-homes/engine-baseline.txt`; new tests go to the canonical home, and a moved file is removed from the baseline (`--write` regenerates it). Scene Graph has migrated to `packages/scene-graph/tests` and the editor domain to `packages/core/tests/editor`; put new Core tests there, mirroring `packages/core/src`. - Address a package's source by `#/*` and its own helpers by `#-tests/*` (`#core-tests/*`, `#fig-tests/*`); never drill with `../../`. Registering a new alias means `imports` in the package manifest and `PACKAGE_ALIASES`/`PACKAGE_ALIAS_OWNERS` in `tools/checks/architecture/src/steiger-rules/support.ts`. +- Reach repository files, such as `tests/fixtures`, through `repoPath`/`testPath` in `tests/helpers/paths.ts`, or a package helper's `workspaceRoot()`, which finds the root by its lockfile; never climb from `import.meta` (`open-pencil/no-deep-parent-relative-paths`). - Owner-local helpers and fixtures stay local; only genuinely shared support goes under central `tests/helpers//` and `tests/fixtures/`. MCP transport tests: `tests/engine/mcp/{server,stdio,transport}` with `tests/helpers/mcp`. - Never commit temporary, diagnostic, or profile specs; keep them in ignored `scratch/`. diff --git a/tests/e2e/app/closed-documents.spec.ts b/tests/e2e/app/closed-documents.spec.ts index 7648348e6..d0b2fb291 100644 --- a/tests/e2e/app/closed-documents.spec.ts +++ b/tests/e2e/app/closed-documents.spec.ts @@ -1,10 +1,9 @@ -import { fileURLToPath } from 'node:url' - import { expect, test } from '@playwright/test' import type * as AppTabs from '@/app/tabs' import { CanvasHelper } from '#tests/helpers/canvas' +import { testPath } from '#tests/helpers/paths' const FIXTURE = 'gold-preview.fig' @@ -14,7 +13,7 @@ const FIXTURE = 'gold-preview.fig' test('documents closed in tabs are released', async ({ page }) => { test.setTimeout(90_000) await page.route(`**/__fixtures/${FIXTURE}`, (route) => - route.fulfill({ path: fileURLToPath(new URL(`../../fixtures/${FIXTURE}`, import.meta.url)) }) + route.fulfill({ path: testPath('fixtures', FIXTURE) }) ) await page.goto('/?test') await new CanvasHelper(page).waitForInit() diff --git a/tests/e2e/canvas/live-editing.spec.ts b/tests/e2e/canvas/live-editing.spec.ts index 12a4df1f8..ed1f0df7c 100644 --- a/tests/e2e/canvas/live-editing.spec.ts +++ b/tests/e2e/canvas/live-editing.spec.ts @@ -1,10 +1,9 @@ -import { fileURLToPath } from 'node:url' - import type { Page } from '@playwright/test' import { expect, test } from '#tests/e2e/fixtures' import { expectDefined } from '#tests/helpers/assert' import { CanvasHelper } from '#tests/helpers/canvas' +import { testPath } from '#tests/helpers/paths' // This size exceeds the 3x overscan budget and previously produced fractional // raster offsets, even though the live scene itself was pixel-aligned. @@ -80,7 +79,7 @@ for (const imported of [false, true]) { test.setTimeout(45_000) await page.route('**/__fixtures/gold-preview.fig', (route) => route.fulfill({ - path: fileURLToPath(new URL('../../fixtures/gold-preview.fig', import.meta.url)), + path: testPath('fixtures/gold-preview.fig'), contentType: 'application/octet-stream' }) ) diff --git a/tests/engine/io/fig/export/worker.test.ts b/tests/engine/io/fig/export/worker.test.ts index 0a8176bbe..a5de6ff5b 100644 --- a/tests/engine/io/fig/export/worker.test.ts +++ b/tests/engine/io/fig/export/worker.test.ts @@ -1,6 +1,7 @@ import { describe, test, expect, beforeAll, setDefaultTimeout } from 'bun:test' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' +import { pathToFileURL } from 'node:url' import { unzipSync } from 'fflate' @@ -12,11 +13,12 @@ import { SceneGraph } from '@open-pencil/core' +import { coreSourcePath, testPath } from '#tests/helpers/paths' import { heavy } from '#tests/helpers/test-utils' setDefaultTimeout(30_000) -const FIXTURES = resolve(import.meta.dir, '../../../../fixtures') +const FIXTURES = testPath('fixtures') const CUSTOM_FIG_KIWI_VERSION = 77 function canvasFigVersion(figData: Uint8Array): number { @@ -38,10 +40,9 @@ function compressInWorker(message: { figKiwiVersion?: number }): Promise { return new Promise((resolve, reject) => { - const worker = new Worker( - new URL('../../../../../packages/core/src/io/formats/fig/export-worker.ts', import.meta.url), - { type: 'module' } - ) + const worker = new Worker(pathToFileURL(coreSourcePath('io/formats/fig/export-worker.ts')), { + type: 'module' + }) worker.onmessage = (event: MessageEvent) => { worker.terminate() resolve(event.data) diff --git a/tests/engine/io/fig/page-manifest.test.ts b/tests/engine/io/fig/page-manifest.test.ts index 790cfbc31..a82d3e4d7 100644 --- a/tests/engine/io/fig/page-manifest.test.ts +++ b/tests/engine/io/fig/page-manifest.test.ts @@ -1,11 +1,12 @@ import { expect, test } from 'bun:test' import { readFileSync } from 'node:fs' -import { resolve } from 'node:path' import { parseFigBuffer } from '@open-pencil/fig' import type { FigPageManifestEntry } from '@open-pencil/kiwi/fig' -const fixturePath = resolve(import.meta.dir, '../../../fixtures/gold-preview.fig') +import { testPath } from '#tests/helpers/paths' + +const fixturePath = testPath('fixtures/gold-preview.fig') test('reports FIG pages before materializing NodeChange objects', () => { const bytes = readFileSync(fixturePath) diff --git a/tests/helpers/fig/fixtures.ts b/tests/helpers/fig/fixtures.ts index cfe052e4c..a728cea60 100644 --- a/tests/helpers/fig/fixtures.ts +++ b/tests/helpers/fig/fixtures.ts @@ -10,9 +10,10 @@ import { type SceneNode } from '@open-pencil/core' +import { testPath } from '../paths' import { collectAllNodes } from './traversal' -export const FIXTURES = resolve(import.meta.dir, '../../fixtures') +export const FIXTURES = testPath('fixtures') export const VALID_NODE_TYPES = new Set([ 'CANVAS', diff --git a/tests/helpers/paths.ts b/tests/helpers/paths.ts index 3c6d868b7..e360bd8b9 100644 --- a/tests/helpers/paths.ts +++ b/tests/helpers/paths.ts @@ -1,7 +1,16 @@ import { existsSync } from 'node:fs' -import { join } from 'node:path' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' -const repoRoot = join(import.meta.dir, '..', '..') +/** The workspace root, found by its lockfile rather than by counting directories. */ +export function workspaceRoot(from = dirname(fileURLToPath(import.meta.url))): string { + for (let dir = from; ; dir = dirname(dir)) { + if (existsSync(join(dir, 'bun.lock'))) return dir + if (dirname(dir) === dir) throw new Error(`No workspace root above ${from}`) + } +} + +const repoRoot = workspaceRoot() export function repoPath(...segments: string[]): string { return join(repoRoot, ...segments) diff --git a/tools/checks/lint/src/plugin.ts b/tools/checks/lint/src/plugin.ts index 4db9cd867..3b6d15af7 100644 --- a/tools/checks/lint/src/plugin.ts +++ b/tools/checks/lint/src/plugin.ts @@ -374,6 +374,7 @@ import { noVueSelfPackageImports, noCrossPackageSourceImports, noDeepParentRelativeImports, + noDeepParentRelativePaths, noCoreParentRelativeImports, noMcpParentRelativeImports, noVueParentRelativeImports, @@ -452,6 +453,7 @@ const plugin = { 'no-vue-self-package-imports': noVueSelfPackageImports, 'no-cross-package-source-imports': noCrossPackageSourceImports, 'no-deep-parent-relative-imports': noDeepParentRelativeImports, + 'no-deep-parent-relative-paths': noDeepParentRelativePaths, 'no-core-parent-relative-imports': noCoreParentRelativeImports, 'no-mcp-parent-relative-imports': noMcpParentRelativeImports, 'no-vue-parent-relative-imports': noVueParentRelativeImports, diff --git a/tools/checks/lint/src/rules/imports.ts b/tools/checks/lint/src/rules/imports.ts index 992bb432b..f41fdcc97 100644 --- a/tools/checks/lint/src/rules/imports.ts +++ b/tools/checks/lint/src/rules/imports.ts @@ -88,6 +88,71 @@ const noDeepParentRelativeImports = createParentRelativeImportRule({ minDepth: 2 }) +/** + * A path that climbs two or more directories, written as `../..`, `..\\..`, or `'..', '..'`. + * An unknown part, such as a template expression, ends the run of parent segments. + */ +function climbsTwoLevels(values: (string | null)[]): boolean { + const joined = values + .map((value) => value ?? '\u0000') + .join('/') + .replaceAll('\\', '/') + return /(^|\/)\.\.\/+\.\.(\/|$)/.test(joined) +} + +function staticString(node: TSESTree.Node): string | null { + if (node.type === 'Literal' && typeof node.value === 'string') return node.value + if (node.type === 'TemplateLiteral') { + const head = node.quasis[0]?.value.cooked ?? null + return head !== null && node.expressions.length ? `${head}\u0000` : head + } + return null +} + +/** `import.meta.url`, `.dir`, or `.dirname`, directly or wrapped, as in `dirname(fileURLToPath(import.meta.url))`. */ +function isImportMetaLocation(node: TSESTree.Node): boolean { + if (node.type === 'MemberExpression') { + if (node.object.type === 'MetaProperty') { + return ( + node.property.type === 'Identifier' && + ['url', 'dir', 'dirname'].includes(node.property.name) + ) + } + return isImportMetaLocation(node.object) + } + if (node.type === 'CallExpression') return node.arguments.some(isImportMetaLocation) + return false +} + +const noDeepParentRelativePaths: RuleDefinition = { + meta: { + docs: { + description: + 'Disallow climbing two or more directories from import.meta — use a path helper or alias' + } + }, + create(context: RuleContext) { + const message = + 'Resolve the file through a path helper such as repoPath() or an import alias instead of climbing with ../..' + function check(node: TSESTree.Node, relative: TSESTree.CallExpressionArgument[]) { + if (relative.length && climbsTwoLevels(relative.map(staticString))) { + context.report({ node, message }) + } + } + return { + NewExpression(node) { + if (node.callee.type !== 'Identifier' || node.callee.name !== 'URL') return + if (!node.arguments[1] || !isImportMetaLocation(node.arguments[1])) return + check(node, node.arguments.slice(0, 1)) + }, + CallExpression(node) { + const start = node.arguments.findIndex(isImportMetaLocation) + if (start !== -1) check(node, node.arguments.slice(start + 1)) + } + } + } +} + const noCoreParentRelativeImports = createParentRelativeImportRule({ description: 'Disallow parent-relative imports in core internals — use #core/* aliases', applies: (file) => @@ -214,6 +279,7 @@ export { noVueSelfPackageImports, noCrossPackageSourceImports, noDeepParentRelativeImports, + noDeepParentRelativePaths, noCoreParentRelativeImports, noMcpParentRelativeImports, noVueParentRelativeImports, diff --git a/tools/checks/lint/tests/deep-paths.test.ts b/tools/checks/lint/tests/deep-paths.test.ts new file mode 100644 index 000000000..a9b13599e --- /dev/null +++ b/tools/checks/lint/tests/deep-paths.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, test } from 'bun:test' + +import { lint, ruleDiagnostics } from './helpers/lint.ts' + +const rule = 'no-deep-parent-relative-paths' +const rules = { [`open-pencil/${rule}`]: 'error' } +// A template that climbs one level and then names a folder, assembled so it stays source text. +const oneLevelTemplate = ['join(import.meta.dir, `../', '$', "{folder}`, '..', 'assets')"].join('') + +describe('no-deep-parent-relative-paths', () => { + test.each([ + "new URL('../../fixtures/a.fig', import.meta.url)", + 'new URL(`../../fixtures/a.fig`, import.meta.url)', + "resolve(import.meta.dir, '../../../tests/fixtures')", + "join(import.meta.dirname, '..', '..', 'assets')", + "path.resolve(import.meta.dir, '../..')", + "join(import.meta.dir, '..\\\\..\\\\assets')", + "join(dirname(fileURLToPath(import.meta.url)), '../../fixtures')" + ])('rejects %s', async (source) => { + expect(ruleDiagnostics(await lint(source, rules), rule)).toHaveLength(1) + }) + + test.each([ + "new URL('../fixtures/a.fig', import.meta.url)", + "new URL('./worker.ts', import.meta.url)", + "join(import.meta.dir, '..', 'fixtures')", + "credentialRef('../../other-app', 'api-key')", + "resolve(root, '../../somewhere')", + "join(dirname(fileURLToPath(import.meta.url)), '..', 'fixtures')", + oneLevelTemplate + ])('accepts %s', async (source) => { + expect(ruleDiagnostics(await lint(source, rules), rule)).toHaveLength(0) + }) +})