diff --git a/packages/nopy/src/cubes/dependencies.ts b/packages/nopy/src/cubes/dependencies.ts index 57852f8..eb7f41e 100644 --- a/packages/nopy/src/cubes/dependencies.ts +++ b/packages/nopy/src/cubes/dependencies.ts @@ -23,6 +23,18 @@ export class BuildContext { public readonly cubeSessions: CubeSession[] = []; private readonly resolvedCubes = new Set(); + /** + * The (cube, host) pairs currently being resolved, innermost last. + * + * `resolvedCubes` cannot serve here: it is written by `buildDeployCall`, which + * runs *after* the recursive descent, so a cycle never reaches it — the two + * cubes just recurse until the stack overflows. It cannot be widened into a + * "seen" set either, because re-entering a cube with different `param` + * overrides is a legitimate thing for a dependency or a hook to do. What is + * never legitimate is re-entering one that has not finished. + */ + private readonly resolving: { cubeId: string; host: string }[] = []; + constructor( public readonly allCubes: Record, public readonly variables: Variables, @@ -130,6 +142,25 @@ export class BuildContext { this.assertVariablesComplete(cube); } + /** + * Fails a run whose dependency graph loops back on itself. + * + * Names the whole path rather than just the repeated cube: with dependencies + * declared dynamically — and hooks free to `exec` anything at all — the edge + * that closed the loop is rarely the one you would guess from the two ends. + */ + private assertNoCycle(cubeId: string, host: string): void { + const at = this.resolving.findIndex((f) => f.cubeId === cubeId && f.host === host); + if (at === -1) return; + + const path = [...this.resolving.slice(at).map((f) => f.cubeId), cubeId].join(' → '); + throw new NopyUsageError( + `Circular dependency on ${host}: ${path}. ` + + 'A cube cannot depend on itself, directly or through a chain — check the ' + + '`dependencies()` of each cube named, and any `before`/`after` hook that calls `exec`.' + ); + } + /** * Resolves a cube, its dependencies, and hooks recursively */ @@ -145,6 +176,22 @@ export class BuildContext { log.debug('Resolving cube', { cubeId, host }); + this.assertNoCycle(cubeId, host); + this.resolving.push({ cubeId, host }); + try { + await this.visitCube(cube, host, overrides); + } finally { + this.resolving.pop(); + } + } + + /** + * The body of {@link resolveCube}, once the cube is known and the cycle guard + * has admitted it. + */ + private async visitCube(cube: Cube, host: string, overrides: CubeVariables): Promise { + const cubeId = cube.id; + // 1. Declare secrets and schema, then assign overrides and defaults. Both // declarations have to come first: the cube's first assignment is what // seeds the config `env` onto it, and by then it must already be known diff --git a/packages/nopy/tests/cubes.dependencies.edge.test.ts b/packages/nopy/tests/cubes.dependencies.edge.test.ts index dff1bad..932185b 100644 --- a/packages/nopy/tests/cubes.dependencies.edge.test.ts +++ b/packages/nopy/tests/cubes.dependencies.edge.test.ts @@ -8,6 +8,7 @@ import { z } from 'zod'; import { BuildContext } from '../src/cubes/dependencies.js'; import { Variables } from '../src/nopy.common.js'; import type { NopyConfig } from '../src/nopy.config.js'; +import { NopyUsageError } from '../src/nopy.errors.js'; import type { NopySession } from '../src/nopy.session.js'; vi.mock('../src/nopy.prompts.js', async () => { @@ -61,6 +62,104 @@ describe('BuildContext error handling', () => { }); }); +describe('BuildContext cycle detection', () => { + const depCube = (id: string, deps: string[]) => + new Cube( + Manifest.create({ id, name: id, schema: z.object({}), dependencies: () => deps }), + `/test/${id}`, + 'deploy.py' + ); + + const build = (cubes: Cube[]) => + new BuildContext( + Object.fromEntries(cubes.map((c) => [c.id, c])), + new Variables(), + session(), + config, + { method: 'ssh' }, + { useDefaults: true } + ); + + it('rejects a cube that depends on itself', async () => { + const context = build([depCube('cube-a', ['cube-a'])]); + + await expect(context.resolveCube('cube-a', 'host1')).rejects.toThrow( + 'Circular dependency on host1: cube-a → cube-a' + ); + }); + + it('names the whole path of a longer loop', async () => { + const context = build([ + depCube('cube-a', ['cube-b']), + depCube('cube-b', ['cube-c']), + depCube('cube-c', ['cube-a']), + ]); + + await expect(context.resolveCube('cube-a', 'host1')).rejects.toThrow( + 'Circular dependency on host1: cube-a → cube-b → cube-c → cube-a' + ); + }); + + it('reports the loop as usage rather than overflowing the stack', async () => { + const context = build([depCube('cube-a', ['cube-b']), depCube('cube-b', ['cube-a'])]); + + // The point of the finding: before the guard this recursed until V8 gave up, + // and a RangeError names no cube and reads as a nopy crash rather than a + // manifest that says something impossible. + await expect(context.resolveCube('cube-a', 'host1')).rejects.toBeInstanceOf(NopyUsageError); + expect(context.deployCalls).toHaveLength(0); + }); + + it('catches a loop a hook closes rather than a dependency', async () => { + const a = new Cube( + Manifest.create({ + id: 'cube-a', + name: 'A', + schema: z.object({}), + dependencies: () => ['cube-b'], + }), + '/test/cube-a', + 'deploy.py' + ); + const b = new Cube( + Manifest.create({ + id: 'cube-b', + name: 'B', + schema: z.object({}), + before: [async (ctx) => ctx.exec('cube-a', {})], + }), + '/test/cube-b', + 'deploy.py' + ); + + await expect(build([a, b]).resolveCube('cube-a', 'host1')).rejects.toThrow( + 'Circular dependency on host1: cube-a → cube-b → cube-a' + ); + }); + + it('allows a diamond, where the shared cube is entered twice but never nested', async () => { + const context = build([ + depCube('top', ['left', 'right']), + depCube('left', ['shared']), + depCube('right', ['shared']), + depCube('shared', []), + ]); + + await context.resolveCube('top', 'host1'); + + expect(context.deployCalls.map((c) => c.cube)).toEqual(['shared', 'left', 'right', 'top']); + }); + + it('allows the same cube on a different host', async () => { + const context = build([depCube('cube-a', [])]); + + await context.resolveCube('cube-a', 'host1'); + await context.resolveCube('cube-a', 'host2'); + + expect(context.deployCalls.map((c) => c.host)).toEqual(['host1', 'host2']); + }); +}); + describe('BuildContext log configuration', () => { const build = (log: NopyConfig['log']) => new BuildContext(