From 3adddf5dedb95455853f9218dde081b65653ad38 Mon Sep 17 00:00:00 2001 From: Neil Fulwiler Date: Thu, 27 Aug 2026 13:37:38 -0400 Subject: [PATCH] fix: apply transitive dependent hooks against the render root, not the immediate parent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per #21: a target's dependent hook must run with ctx.consumer set to the render root, never the intermediate resource that actually declared the edge. The FS a dependent hook can mutate is always the render root's bundle, so keeping ctx.consumer as the root keeps the two consistent, and it means a target kind's `dependents` list is keyed by the render-root kinds that can reach it rather than needing an entry for every pass-through kind in between — dynamo-table's existing `[service, subscriber]` entries already cover a service reaching it through any number of intermediate packages. Updates SPEC.md and the render.go doc comments accordingly, and fixes the three dependency tests that assumed ctx.consumer was the immediate parent: - multi-hop test: dynamo-table now accepts "service" (the root), not "package". - cycle test: alpha's dependents entry now accepts its own kind (the root), not "beta". - diamond test: since ctx.consumer is now identical (the root) for both edges into the shared leaf, the two firings are distinguished by their own params instead of consumer identity. --- SPEC.md | 38 ++++++++++------- pkg/render/render.go | 36 ++++++++++------ pkg/render/render_dependencies_test.go | 57 ++++++++++++++++---------- 3 files changed, 82 insertions(+), 49 deletions(-) diff --git a/SPEC.md b/SPEC.md index 88f4c1f..3bdd3d5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -245,7 +245,8 @@ For a single resource being rendered, the runner executes hooks in this order: 1. The kind's `hooks.render` (in declaration order). 2. For every dependency reachable by walking the full transitive dependency graph rooted at this resource (breadth-first, cycle-safe, visit-once — see [Dependencies](#dependencies)), the - target kind's `hooks.dependents` matching the immediate consumer at that hop. + target kind's `hooks.dependents` matching *this* resource's own kind — never the + intermediate resource that actually declared the edge. 3. The resource's own `metadata.hooks.render` (see [Resource](#resource) — paths relative to the resource file). Resource hooks see the bundle after every kind-level write. 4. The kind's `hooks.post_render`. Mutating; runs after resource hooks so the kind owns the final @@ -620,14 +621,18 @@ the `hooks.dependents` block — `dependents` is just another lifecycle alongsid Each entry has: -- **`kind`** — the consumer kind that may depend on this resource. +- **`kind`** — the kind of render root that may reach this resource as a dependency, directly or + transitively through any number of pass-through resources. See + [Render-time execution](#render-time-execution): the hook's `consumer` is always the render + root, never the resource that actually declared the edge, so this entry is keyed on the root's + kind regardless of how many hops separate it from this target. - **`paths`** — at least one hook file path that runs when a resource of `kind` depends on this one. Hooks run in declaration order. An empty `paths` array is a build error. - **`params_path`** — JSON Schema describing the `params` object the consumer must supply. Paths are resolved relative to the `kind.json` file. Required. -Because hooks are registered per consumer kind, each hook receives a concretely typed `consumer` and -`params` — no union narrowing inside the hook body. +Because hooks are registered per render-root kind, each hook receives a concretely typed `consumer` +and `params` — no union narrowing inside the hook body. ## `dependent` hooks @@ -702,25 +707,27 @@ resolves once and the walk still terminates. Every kind's dependencies resolve t global capability of the render pipeline, not something a kind opts into. Starting from the root resource's own `dependencies`, and then from each newly-reached target's -`dependencies` in turn, every edge (consumer, target) is processed: +`dependencies` in turn, every edge (declarer, target) is processed: 1. Resolve `(kind, name)` to a target resource in the catalog (built from `resource_discovery.paths`). Missing targets are a hard error. 2. Apply the target's own overlays + spec defaults so `ctx.self` matches what the target would see at its own render. No schema validation: targets are inspected, not re-rendered. A target reached via more than one edge is resolved only once and reused. -3. Find the target kind's `hooks.dependents` entry matching the *immediate* consumer's kind — the - resource that declared this particular edge, which may be several hops away from the render - root. A target that doesn't list that consumer's kind as allowed is a hard error. +3. Find the target kind's `hooks.dependents` entry matching the *render root's* kind — never the + declarer's kind, even when the declarer (the resource whose own `dependencies` list named this + target) is several hops away from the root. A target that doesn't list the root's kind as + allowed is a hard error. 4. Run each registered dependent hook against the render root's FS, in declaration order. Concretely: if a service depends on `package/api-rate-limits`, which itself depends on `dynamo-table/rate-limit-exceeded`, the dynamo-table's dependent hooks for consumer kind -`package` run — with `ctx.self` set to the resolved dynamo-table resource and `ctx.consumer` set -to the resolved `api-rate-limits` package, exactly as if the package were being rendered -directly — but mutating the *service's* bundle, since that's the one FS threaded through the -entire walk. `ctx.consumer` is always the immediate parent in the dependency chain, never -necessarily the render root. +`service` run — with `ctx.self` set to the resolved dynamo-table resource and `ctx.consumer` set +to the resolved *service*, not the package that actually declared the edge. `ctx.consumer` is +always the render root, and the FS the hook mutates is always the render root's bundle, so the two +stay consistent — a target's `dependents` entry never needs to name every pass-through kind that +might sit between it and the root; dynamo-table's existing `[service, subscriber]` entries already +cover it being reached through any number of intermediate packages. Render hooks cannot observe state injected by dependent hooks — the lifecycles are strictly ordered (overrides → render → dependents → re-stamp `skip_hooks` overrides → write). @@ -947,8 +954,9 @@ catalog and walks outward from. 8. Apply `hooks.render` in order (calling each `RenderHook.render`), threading the FS through the pipeline 9. Walk the full transitive dependency graph rooted at this resource (breadth-first, cycle-safe, visit-once) — for every edge, look up the target via the catalog, find the target kind's - matching `hooks.dependents` entry for that edge's immediate consumer kind, and run those hooks - against the render root's FS. See [Dependencies](#dependencies). + matching `hooks.dependents` entry for *this resource's own* kind (the render root, never the + declarer at that hop), and run those hooks against the render root's FS. See + [Dependencies](#dependencies). 10. Re-stamp every `skip_hooks: true` override's bytes onto the bundle, discarding any in-flight hook mutations to those files. 11. Write the final files to disk diff --git a/pkg/render/render.go b/pkg/render/render.go index c667834..80c870e 100644 --- a/pkg/render/render.go +++ b/pkg/render/render.go @@ -385,13 +385,20 @@ func depNodeID(kind, name string) string { return kind + "/" + name } // applies every qualifying dependent hook along the way, not just the // root's own directly-declared dependencies. // -// At each edge, the "consumer" the target's dependent hook sees is the -// immediate parent in the walk, not the root: a service depending on a -// package that itself depends on a dynamo-table runs the dynamo-table's -// "package" dependent hooks with the package as ctx.consumer, exactly as -// if the package were being rendered directly. That parent's bundle is -// always the one shared, root-owned bundle threaded through the whole -// walk, so every hop's mutations land in the same output. +// At every edge, the "consumer" a target's dependent hook sees is the +// render root — never the intermediate resource that actually declared +// the edge. A service depending on a package that itself depends on a +// dynamo-table runs the dynamo-table's "service" dependent hooks with +// the *service* as ctx.consumer, not the package: the package is a +// pass-through, and the only FS a dependent hook can ever mutate is +// the render root's bundle, so ctx.consumer names the resource that +// bundle actually belongs to. This also means a target kind's +// `dependents` list is keyed by the possible render-root kinds that +// can reach it, regardless of how many pass-through kinds sit in +// between — dynamo-table's existing `dependents: [service, subscriber]` +// already covers a service reaching it through any number of +// intermediate packages, with no new entry required per pass-through +// kind. func applyDependencies(parent *slog.Logger, bundle hook.Bundle, rootKind, rootName, rootPath string, rootResource *veilv1.Resource, rootMap map[string]any, root string, opts *Options) (hook.Bundle, error) { if opts.Catalog == nil { return nil, fmt.Errorf("no catalog configured") @@ -429,7 +436,7 @@ func applyDependencies(parent *slog.Logger, bundle hook.Bundle, rootKind, rootNa queue = append(queue, node) } - newBundle, err := applyDependentHooks(logger, bundle, dep, node, cur, root, opts) + newBundle, err := applyDependentHooks(logger, bundle, dep, node, rootNode, root, opts) if err != nil { return nil, fmt.Errorf("dependency %s/%s: %w", targetKind, targetName, err) } @@ -444,10 +451,15 @@ func applyDependencies(parent *slog.Logger, bundle hook.Bundle, rootKind, rootNa } // applyDependentHooks runs every dependent hook target's kind registers -// for consumer's kind against the shared bundle. target is already -// resolved (overlays + schema defaults applied) by the caller — reused -// across every consumer that reaches it during the walk so a -// many-times-depended-on resource is only resolved once. +// for the render root's kind against the shared bundle. consumer is +// always the render root (see applyDependencies), never the resource +// that actually declared this edge — a target's `dependents` list is +// matched against the root's kind regardless of which hop the edge +// came from. target is already resolved (overlays + schema defaults +// applied) by the caller — reused across every edge that reaches it +// during the walk so a many-times-depended-on resource is only +// resolved once; params are read fresh per edge below, so two edges +// into the same target with different params still apply independently. func applyDependentHooks(parent *slog.Logger, bundle hook.Bundle, dep *veilv1.Dependency, target, consumer *depNode, root string, opts *Options) (hook.Bundle, error) { loadedKind, err := opts.Registry.LoadKind(target.kind) if err != nil { diff --git a/pkg/render/render_dependencies_test.go b/pkg/render/render_dependencies_test.go index 59362a3..166b01d 100644 --- a/pkg/render/render_dependencies_test.go +++ b/pkg/render/render_dependencies_test.go @@ -15,15 +15,18 @@ func dependentHookIIFE(markerFile string) string { return `var __veilMod=(()=>{var h={render:function(ctx,fs){fs.add("` + markerFile + `","target="+ctx.self.metadata.name+" consumer="+ctx.consumer.metadata.name);return fs;}};return{default:h};})();` } -// dependentHookIIFEKeyedByConsumer returns a pre-bundled dependent hook -// that stamps a marker file per invocation, named after the immediate -// consumer rather than a fixed path. A target reached through more +// dependentHookIIFEKeyedByParam returns a pre-bundled dependent hook +// that stamps a marker file per invocation, named after one of the +// edge's own params rather than a fixed path. ctx.consumer is always +// the render root (see applyDependencies in render.go) — identical +// for every edge into a shared target — so params are the only +// signal that still varies per edge; a target reached through more // than one incoming edge (a diamond dependency) writes one such file // per edge instead of one shared file the second firing would // silently overwrite — proof that each edge's hook ran independently // rather than the target's node-level dedup suppressing repeat edges. -func dependentHookIIFEKeyedByConsumer(prefix string) string { - return `var __veilMod=(()=>{var h={render:function(ctx,fs){fs.add("` + prefix + `-via-"+ctx.consumer.metadata.name+".txt","target="+ctx.self.metadata.name+" consumer="+ctx.consumer.metadata.name);return fs;}};return{default:h};})();` +func dependentHookIIFEKeyedByParam(prefix, paramKey string) string { + return `var __veilMod=(()=>{var h={render:function(ctx,fs){fs.add("` + prefix + `-via-"+ctx.params.` + paramKey + `+".txt","target="+ctx.self.metadata.name+" consumer="+ctx.consumer.metadata.name+" param="+ctx.params.` + paramKey + `);return fs;}};return{default:h};})();` } // noopDependentHookIIFE is a dependent hook that satisfies a required @@ -139,7 +142,7 @@ func (s *RenderSuite) renderKind(kind, name, dir, outDir string) (*RenderedResou func (s *RenderSuite) TestMultiHopDependencyAppliesTransitiveDependentHooks() { s.writeSimpleKind("service") s.writeDependentKind("package", "service", dependentHookIIFE("from-package.txt")) - s.writeDependentKind("dynamo-table", "package", dependentHookIIFE("from-dynamo.txt")) + s.writeDependentKind("dynamo-table", "service", dependentHookIIFE("from-dynamo.txt")) s.reloadRegistryWithKinds("service", "package", "dynamo-table") dir := filepath.Join(s.root, "svc") @@ -177,16 +180,19 @@ func (s *RenderSuite) TestMultiHopDependencyAppliesTransitiveDependentHooks() { // dynamo-table — the regression this test guards against. fromDynamo, err := os.ReadFile(filepath.Join(out, "my-service", "from-dynamo.txt")) s.Require().NoError(err) - s.Equal("target=rate-limit-exceeded consumer=api-rate-limits", string(fromDynamo)) + s.Equal("target=rate-limit-exceeded consumer=my-service", string(fromDynamo)) } // TestDependencyCycleAppliesEachEdgeOnceWithoutInfiniteLoop covers the // cycle-safety half of the BFS walk: alpha depends on beta and beta -// depends back on alpha, and both kinds accept the other as a -// consumer. The walk must apply each real edge's hooks exactly once -// and terminate instead of looping forever re-visiting the same pair. +// depends back on alpha. ctx.consumer is always the render root +// (alpha/a1), so alpha's own dependents list must accept its own kind +// to allow the back-edge — beta's back-edge into alpha is checked +// against the root's kind, not beta's. The walk must apply each real +// edge's hooks exactly once and terminate instead of looping forever +// re-visiting the same pair. func (s *RenderSuite) TestDependencyCycleAppliesEachEdgeOnceWithoutInfiniteLoop() { - s.writeDependentKind("alpha", "beta", dependentHookIIFE("from-alpha.txt")) + s.writeDependentKind("alpha", "alpha", dependentHookIIFE("from-alpha.txt")) s.writeDependentKind("beta", "alpha", dependentHookIIFE("from-beta.txt")) s.reloadRegistryWithKinds("alpha", "beta") @@ -214,7 +220,7 @@ func (s *RenderSuite) TestDependencyCycleAppliesEachEdgeOnceWithoutInfiniteLoop( fromAlpha, err := os.ReadFile(filepath.Join(out, "a1", "from-alpha.txt")) s.Require().NoError(err) - s.Equal("target=a1 consumer=b1", string(fromAlpha)) + s.Equal("target=a1 consumer=a1", string(fromAlpha)) } // TestDiamondDependencyFiresTargetHookOncePerIncomingEdge covers the @@ -225,11 +231,14 @@ func (s *RenderSuite) TestDependencyCycleAppliesEachEdgeOnceWithoutInfiniteLoop( // expansion), but its dependent hook must still fire once per // incoming edge — the visited-set dedup is about not re-expanding a // node's own dependencies, not about suppressing repeat edges into -// it. +// it. ctx.consumer is always the render root (diamond-root/r1) for +// both firings, identical either way, so the two edges are +// distinguished by their own params instead — the one thing that +// still varies per edge. func (s *RenderSuite) TestDiamondDependencyFiresTargetHookOncePerIncomingEdge() { s.writeSimpleKind("diamond-root") s.writeDependentKind("diamond-branch", "diamond-root", noopDependentHookIIFE) - s.writeDependentKind("diamond-leaf", "diamond-branch", dependentHookIIFEKeyedByConsumer("from-leaf")) + s.writeDependentKind("diamond-leaf", "diamond-root", dependentHookIIFEKeyedByParam("from-leaf", "tag")) s.reloadRegistryWithKinds("diamond-root", "diamond-branch", "diamond-leaf") dir := filepath.Join(s.root, "dmd") @@ -243,14 +252,18 @@ func (s *RenderSuite) TestDiamondDependencyFiresTargetHookOncePerIncomingEdge() }, }) s.writeJSON(filepath.Join(dir, "b1.json"), map[string]any{ - "metadata": map[string]any{"kind": "diamond-branch", "name": "b1"}, - "spec": map[string]any{}, - "dependencies": []map[string]any{{"kind": "diamond-leaf", "name": "d1", "params": map[string]any{}}}, + "metadata": map[string]any{"kind": "diamond-branch", "name": "b1"}, + "spec": map[string]any{}, + "dependencies": []map[string]any{ + {"kind": "diamond-leaf", "name": "d1", "params": map[string]any{"tag": "b1"}}, + }, }) s.writeJSON(filepath.Join(dir, "c1.json"), map[string]any{ - "metadata": map[string]any{"kind": "diamond-branch", "name": "c1"}, - "spec": map[string]any{}, - "dependencies": []map[string]any{{"kind": "diamond-leaf", "name": "d1", "params": map[string]any{}}}, + "metadata": map[string]any{"kind": "diamond-branch", "name": "c1"}, + "spec": map[string]any{}, + "dependencies": []map[string]any{ + {"kind": "diamond-leaf", "name": "d1", "params": map[string]any{"tag": "c1"}}, + }, }) s.writeJSON(filepath.Join(dir, "d1.json"), map[string]any{ "metadata": map[string]any{"kind": "diamond-leaf", "name": "d1"}, @@ -266,9 +279,9 @@ func (s *RenderSuite) TestDiamondDependencyFiresTargetHookOncePerIncomingEdge() // edge, not one shared file the second firing silently overwrote. viaB, err := os.ReadFile(filepath.Join(out, "r1", "from-leaf-via-b1.txt")) s.Require().NoError(err) - s.Equal("target=d1 consumer=b1", string(viaB)) + s.Equal("target=d1 consumer=r1 param=b1", string(viaB)) viaC, err := os.ReadFile(filepath.Join(out, "r1", "from-leaf-via-c1.txt")) s.Require().NoError(err) - s.Equal("target=d1 consumer=c1", string(viaC)) + s.Equal("target=d1 consumer=r1 param=c1", string(viaC)) }