Skip to content

feat(config): extend cycle detection to depends_on + cache.key edges (3rq.3)#2

Merged
brandon-fryslie merged 3 commits into
mainfrom
bf/3rq.3-cycle-depends-on-key
May 13, 2026
Merged

feat(config): extend cycle detection to depends_on + cache.key edges (3rq.3)#2
brandon-fryslie merged 3 commits into
mainfrom
bf/3rq.3-cycle-depends-on-key

Conversation

@brandon-fryslie

Copy link
Copy Markdown
Contributor

Summary

  • buildTemplateGraph now covers all three value-dependency edge kinds in one unified graph: template string refs, cache.key template refs, and cache.depends_on targets
  • Node set expanded from template-kind-only to all declared variables — enables detection of mixed cycles spanning non-template vars
  • Error message updated from "Cycle in template variables:" to "Dependency cycle:" (the detector now spans all variable kinds)
  • 6 new tests: depends_on self-cycle and 2-node cycle, cache.key self-cycle and 2-node cycle, mixed template/depends_on cycle, DAG non-regression

Depends 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

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.
Copilot AI review requested due to automatic review settings May 13, 2026 18:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and cache.depends_on edges 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 does findKeyLine(..., ["variables", ...]), but buildTemplateGraph() includes non-variables.* nodes (e.g. segment-local vars). If a cycle involves a segment var (or any non-variables node), the error path/line will be incorrect. Consider tracking each graph node’s declaration path (e.g. variables.<name> vs segments.<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 allVarNames in 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 allVarNames set. That allows segments.A.template to reference .local declared only in segments.B.vars.local and 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.

Comment thread src/config/dsl-types.ts Outdated
Comment thread src/config/dsl-loader.ts
… 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment thread src/config/dsl-loader.ts
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);
@brandon-fryslie brandon-fryslie merged commit f1b1ef2 into main May 13, 2026
7 of 9 checks passed
@brandon-fryslie brandon-fryslie deleted the bf/3rq.3-cycle-depends-on-key branch May 13, 2026 18:58
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>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants