feat(config): extend cycle detection to depends_on + cache.key edges (3rq.3)#2
Merged
Merged
Conversation
Single entry point loadDslConfig(filePath) → DslConfig that either returns a fully-validated config (every legal state representable, every illegal state unrepresentable) or throws ConfigError aggregating every issue compiler-style. Shape validation: globals + 8 source kinds (literal/input/env/file/shell/ template/time/git) with per-kind required fields; cache policies validate "exactly one of ttl/watch_file/depends_on/key/never" present; segment fields narrowed to closed enums; layout entries must resolve to declared segments. Cross-ref pass: extracts dotted var refs from every template (template/bg/fg/ when + per-variable templates and cache.key), validates each against the declared variable namespace following the scope-proxy's leaf-vs-namespace dispatch rules in src/template-engine/scope.ts. Cycle pass: DFS over the template→template-variable graph; reports the cycle path. Self-cycles, two-node, and N-node cycles all caught. Lives at src/config/dsl-loader.ts alongside the existing loader.ts — chunk 7 (brandon-segment-dsl-migration-bzh) wires this through the daemon and deletes the legacy loader.
…(3rq.3) buildTemplateGraph now covers all three value-dependency edge kinds in one unified DFS — template string refs, cache.key template refs, and cache.depends_on targets. The node set expands from template-kind-only to all declared variables so mixed cycles (template var → shell var via depends_on → back to template var) are caught automatically. Error message updated from "Cycle in template variables:" to the more general "Dependency cycle:" since the detector now spans all variable kinds. 6 new tests: depends_on 2-node cycle, self-depends_on, DAG non-regression, cache.key 2-node cycle, self-cache.key, and mixed template/depends_on cycle.
There was a problem hiding this comment.
Pull request overview
This PR updates the new JSON5-based DSL config loader so dependency-cycle detection spans all variable dependency edge types (template refs, cache.key template refs, and cache.depends_on edges), and adjusts error messaging/tests accordingly.
Changes:
- Unifies dependency-cycle detection across template refs,
cache.key, andcache.depends_onedges and expands the node set beyond template-only variables. - Updates cycle error message wording to “Dependency cycle:” to match the expanded detector.
- Adds new/updated DSL loader tests covering
depends_on,cache.key, mixed-edge cycles, and DAG non-regression.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/config/dsl-loader.ts |
Builds the unified dependency graph and performs cross-ref + cycle validation. |
src/config/dsl-types.ts |
Defines DSL config types, including cache policies and segment-local vars shape. |
test/dsl-loader.test.ts |
Adds coverage for the expanded cycle detection and related validation behavior. |
package.json |
Adds json5 dependency needed by the DSL loader. |
pnpm-lock.yaml |
Locks json5 dependency version. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
src/config/dsl-loader.ts:907
- Cycle detection always reports the issue under
variables.<name>and doesfindKeyLine(..., ["variables", ...]), butbuildTemplateGraph()includes non-variables.*nodes (e.g. segment-local vars). If a cycle involves a segment var (or any non-variablesnode), the error path/line will be incorrect. Consider tracking each graph node’s declaration path (e.g.variables.<name>vssegments.<seg>.vars.<var>) and using that when emitting the cycle issue.
ctx.issues.push({
path: `variables.${node}`,
message: `Dependency cycle: ${cycle.join(" → ")}`,
line: findKeyLine(ctx.source, ["variables", cycle[0]!]),
});
src/config/dsl-loader.ts:975
- Segment vars are added to
allVarNamesin both bare form (<var>) and namespaced form (<seg>.<var>), but edges are only built from the bare name here (addVarEdges(vName, vDecl)). This can miss cycles for namespaced refs and can also collide when multiple segments use the same local var name. Use a single canonical node name for segment vars (preferably the namespaced form) and build edges from that same name.
if (!seg.vars) continue;
for (const [vName, vDecl] of Object.entries(seg.vars)) {
addVarEdges(vName, vDecl);
}
void segName;
src/config/dsl-loader.ts:738
- Cross-reference validation merges all segment-local var names (including bare names) into a single
allVarNamesset. That allowssegments.A.templateto reference.localdeclared only insegments.B.vars.localand still pass validation. If bare refs are meant to be segment-scoped, build the template-validation scope per segment (global vars + that segment’s locals) instead of using a union across all segments.
for (const [segName, seg] of Object.entries(cfg.segments)) {
if (seg.vars) {
for (const v of Object.keys(seg.vars)) allVarNames.add(v);
for (const v of Object.keys(seg.vars)) allVarNames.add(`${segName}.${v}`);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… scope Three bugs from Copilot review on PR #2: 1. Cycle path reporting: validateNoCycles hardcoded `variables.${node}` for all nodes. Segment-local vars (canonical: segName.varName) now carry NodeInfo through buildTemplateGraph so errors report the correct path `segments.S.vars.V` and findKeyLine targets the right source block. 2. Segment-var graph naming: segment vars had both a bare node and a namespaced node in the dependency graph, but edges were only added from the bare name. This caused missed cycles and silent collisions when two segments both declared a var with the same local name (e.g. "local"). Now segment vars use only the namespaced canonical form (segName.varName) as their graph node; addVarEdges passes segCtx so bare template refs like `.local` resolve to `segName.local` within that segment's context. 3. Cross-ref scope: validateCrossReferences built one global allVarNames set that included all segments' bare local vars, so a segment template could reference another segment's bare local and pass validation — runtime would reject it. Each segment's template and local-var refs now validate against a per-segment scope: global vars + own locals (bare + namespaced) + other segments' vars (namespaced only). depends_on still uses the global set since it takes fully-qualified names. 4 new tests: namespaced ref from same segment passes; bare ref to another segment's local is rejected; namespaced ref to another segment's local passes; correct segment-local scope for own-segment template. Followup ticket for findKeyLine + dotted var names: brandon-segment-dsl-config-nrf.
Comment on lines
+733
to
+738
| // Full set for depends_on validation (all names, bare + namespaced). depends_on | ||
| // takes explicit fully-qualified names, so cross-segment visibility is intentional. | ||
| const allVarNames = new Set<string>(Object.keys(cfg.variables)); | ||
| for (const [segName, seg] of Object.entries(cfg.segments)) { | ||
| if (seg.vars) { | ||
| for (const v of Object.keys(seg.vars)) allVarNames.add(v); |
3 tasks
This was referenced May 29, 2026
Merged
brandon-fryslie
added a commit
that referenced
this pull request
Jun 4, 2026
* feat(layout): collapse LayoutNode to container | segment (2de.11) [LAW:one-type-per-behavior] LayoutNode is now exactly `container | segment`. The `inline` and `stepper` node kinds are deleted: interaction, state-driven display, and multi-region clickability all live in a segment's TEMPLATE (via widgets today; the `actions:` model in .12), so they were variability the node type should never have carried. A `segment` is THE unit of rendering — one ref into the named segments map, rendered to one strip item (the 2de.10 structural join). `cells` and `layout` rows survive as authoring SUGAR, lowered at the loader to `container(horizontal, [segment…])` so nothing downstream sees more than two kinds [LAW:one-source-of-truth]. Deleted (their surface is gone, not relocated yet): - InlineNode/StepperNode/InlineCell/ClickWrite types + nodeStateUse projection - node-registry inline/stepper arms, the composite "HUE PIN", setStateUrl/glyphs - validateInlineNode/InlineCell/ClickWrite/StepperNode loader validators - deriveNodeValidators/nodeContributions/nodeDeriveSpecs; deriveValidators is now widget-only (the actions surface merges in at .12) - the node-set-write session.id trigger + node backing-var cross-ref loop Hue rotation is retained but purely DECORATIVE — each segment advances the cursor by one, a container by none; it carries no structural meaning now that cohesion is one-segment-one-item. Tests: dsl-inline-cell + dsl-stepper-node deleted (they pinned the removed surfaces); segment-render-unit #1/#2 re-expressed through the segment surface (a link-bearing template; an empty template) [LAW:behavior-not-structure]; node builders in the suite construct the canonical container|segment tree. dsl-spine snapshot byte-identical; full suite green (1057); live daemon renders the real config (widgets, hue, OSC-8) unchanged. * refactor(layout): drop stale loader comment; delegate deriveValidators [LAW:comments-explain-why-only] Remove a stale WHAT-comment above layoutRowsToNode — it still described the lowering as "a vertical container of cells leaves" and sat directly above the correct description, two divergent copies back-to-back. [LAW:one-source-of-truth] deriveValidators delegates to deriveWidgetValidators rather than duplicating its body. The widget surface IS the whole writable-key surface today, so one body means no drift; when the actions table lands (.12) this single site becomes the widget+action merge. Addresses PR #70 review (both nits). lint + typecheck clean; validator/loader suites green. --------- Co-authored-by: elton-prawn <264370887+elton-prawn@users.noreply.github.com>
This was referenced Jun 9, 2026
brandon-fryslie
added a commit
that referenced
this pull request
Jun 10, 2026
… absence policy (2l6) (#96) Sheriff findings #2+#8: git/cache providers swallowed ~10 failure paths (stash failure rendered as 0, repoName failure as basename, failed status as a clean fallback branch) and projectGitInfo coerced absence to ''/0 while MetricsPayload preserved it — two opposite absence rules in one file, with render-payload's "providers log their own rejections" comment false for exactly these two providers. [LAW:types-are-the-program] New Outcome<T> = ok | absent | failed(reason) (src/utils/outcome.ts). execGitAsync returns the LaunchResult instead of flattening it to a throw; one classifier maps it per command — a non-zero exit that is the domain answering "there is none" (no upstream, no tags, no remote) is absent; transport failures are failed. GitInfo's on-demand fields are Outcome-typed; getGitInfo returns Outcome<GitInfo>; the basename repoName survives only as the explicit no-remote policy, never as an error fallback. readTail/cacheExpiresAt distinguish ENOENT (absent) from real read errors (failed). [LAW:effects-at-boundaries][LAW:single-enforcer] Failure flows as a value to each consumption surface's edge and is logged exactly once there: buildRenderPayload (pull path, via deps.log=dlog) and GitDataProvider's subscribe delivery fold. projectGitInfo is now a pure fold returning {git?, failures[]}; absent and failed both project as MISSING payload fields into applyInput's default + last_error chain. [LAW:one-type-per-behavior] GitPayload adopts MetricsPayload's preserve-absence policy (all fields optional); the coerce-to-''/0 shim is gone, so "no stashes" and "stash count unknown" are no longer the same rendered 0. Verified live against an isolated daemon: healthy repo renders with zero warnings (absent logs nothing), a corrupt .git produces exactly one warn line with the real git error while the bar still renders. Ticket: brandon-git-segment-2l6 (epic brandon-law-remediation-58c) Co-authored-by: elton-prawn <264370887+elton-prawn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
buildTemplateGraphnow covers all three value-dependency edge kinds in one unified graph: template string refs,cache.keytemplate refs, andcache.depends_ontargetsdepends_onself-cycle and 2-node cycle,cache.keyself-cycle and 2-node cycle, mixed template/depends_on cycle, DAG non-regressionDepends on PR #1 (bf/3rq.1-jsonschema-loader).
Test plan
pnpm test -- test/dsl-loader.test.ts→ 79 tests pass (6 new)pnpm test→ 650 tests pass (no regressions)pnpm typecheck→ clean