From bb8b1bfa5c04420cc0babc32b111ea22335a7a51 Mon Sep 17 00:00:00 2001 From: Benjamin Diedrichsen Date: Tue, 1 Sep 2026 12:48:06 +0200 Subject: [PATCH] [fix] nopy: detect a dependency cycle instead of overflowing the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes DOCS-AUDIT §6.5, and the substantive half of §1.5. `docs/API.md` has documented a circular-dependency error since it was written; nothing raised it. Two mutually dependent cubes recursed until V8 gave up, and a `RangeError` names no cube — it reads as a nopy crash rather than as a manifest that says something impossible. `BuildContext` now carries a resolution stack: `resolveCube` pushes its (cube, host) pair, delegates the body to `visitCube`, and pops in a `finally`. A pair re-entered while it is still on the stack raises a `NopyUsageError` naming the whole path — `Circular dependency on host1: a → b → c → a`. The whole path, not just the repeated cube, because dependencies are declared dynamically and a hook may `exec` anything at all, so the edge that closed the loop is rarely the one you would guess from the two ends. It has to be a structure of its own. `resolvedCubes` is written by `buildDeployCall`, which runs *after* the descent, so a cycle never reaches it; and it cannot be widened into a "seen" set, because re-entering a *finished* cube with different `param` overrides is exactly what a dependency or a hook is for. That distinction is what the diamond test pins: `shared` is entered twice under `left` and `right` and must still resolve, while `a → b → a` must not. There is still no topological sort and there does not need to be — emission is post-order, so the order already is a topological one. Cycle detection was the one thing a sort would have given that the recursion did not. Six tests: self-dependency, a three-cube loop, the loop reported as usage rather than as a stack overflow, a loop closed by a hook's `exec` rather than a `dependencies()` entry, the diamond, and the same cube on two hosts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DCzYTAm9QUhvLNr2EpdagJ --- packages/nopy/src/cubes/dependencies.ts | 47 +++++++++ .../tests/cubes.dependencies.edge.test.ts | 99 +++++++++++++++++++ 2 files changed, 146 insertions(+) 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(