Skip to content

Repair build, fix execution engine, and add headless CLI + tests - #1

Merged
MKlolbullen merged 1 commit into
mainfrom
claude/project-expansion-enhancement-7u8hpv
Aug 21, 2026
Merged

Repair build, fix execution engine, and add headless CLI + tests#1
MKlolbullen merged 1 commit into
mainfrom
claude/project-expansion-enhancement-7u8hpv

Conversation

@MKlolbullen

@MKlolbullen MKlolbullen commented Aug 21, 2026

Copy link
Copy Markdown
Owner

The project no longer compiled and would panic at startup. This restores a
working build and adds substantial functionality on top.

Foundation / correctness

  • Rewrite the tool-catalog loader to parse the current tools.yaml map schema
    (cat/in/out/def/params) instead of the stale flat-list format that made
    init() panic. Embed tools.yaml via go:embed so the binary is
    self-contained and runs from any directory.
  • Fix internal/tui/builder.go: add the missing selNode field, migrate to the
    bubbletea v1.3.5 mouse API, replace the non-existent list.Separator and
    lipgloss Unset() usages, and resolve pipe-type checks through the DAG.
  • Add DAG helpers MaxLayer/RemoveFromLayer/InsertAtLayer used by the builder.
  • Fix GetParallelNodes so all parallel nodes in a layer share one concurrent
    group instead of being split into sequential steps.

Execution engine

  • Unify placeholder handling: both {{input}}/{{domain}}/{{output}} and
    $(target_file)/$(target)/$(output) are now supported (substituteArgs).
  • Capture tool stdout into the output file when no output placeholder is
    present, so stdout-streaming tools (e.g. "-o -") actually produce results.
  • Merge and de-duplicate every tool's output in a layer before feeding the
    next layer, instead of forwarding only the first tool's file.

Headless CLI

  • cmd/termaid now supports run/preview/tools/validate/version/help
    subcommands and still launches the TUI when invoked with no arguments,
    making termaid scriptable and CI-friendly.

Quality

  • Add unit tests for graph, catalog, and pipeline.
  • Add a GitHub Actions CI workflow (gofmt/vet/test -race/build), a Makefile,
    and a .gitignore.
  • gofmt the whole tree; remove the accidental empty project.zip.
  • Update README to document the CLI, placeholder styles, and stdout capture.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01MQHqzzjxxDUvDePyPerast

Summary by Sourcery

Restore a reliable build and execution engine while making workflows scriptable through a self-contained headless CLI.

New Features:

  • Add headless run, preview, tools, validate, version, and help commands while preserving the interactive TUI by default.
  • Embed and expose the tool catalog for self-contained execution from any working directory.

Bug Fixes:

  • Restore catalog loading for the current YAML schema and prevent startup panics.
  • Correct workflow execution so placeholder variants, stdout-only tools, parallel layers, and outputs from all branches are handled reliably.
  • Repair TUI builder compatibility and graph-based pipe validation.

Enhancements:

  • Improve DAG layer manipulation and execution ordering for parallel workflows.

Build:

  • Add a Makefile, project ignore rules, formatting cleanup, and remove the accidental project archive.

CI:

  • Add GitHub Actions checks for formatting, vetting, race-enabled tests, and builds.

Documentation:

  • Document headless CLI usage, embedded catalogs, supported placeholders, and stdout capture behavior.

Tests:

  • Add unit coverage for graph operations, catalog parsing and validation, placeholder substitution, output merging, and tool validation.

Summary by CodeRabbit

  • New Features

    • Added CLI commands for running, previewing, validating, and listing tools, plus version and help output.
    • Added headless workflow execution with cancellation, status reporting, and concurrency controls.
    • Embedded and expanded the tool catalog with metadata and compatibility checks.
    • Improved workflow argument substitution and merged, deduplicated outputs across processing stages.
    • Enhanced interactive workflow editing, node selection, and type validation.
  • Documentation

    • Expanded setup, CLI, Makefile, catalog, and placeholder usage guidance.
  • Chores

    • Added automated formatting, vetting, testing, race detection, and build checks.

The project no longer compiled and would panic at startup. This restores a
working build and adds substantial functionality on top.

Foundation / correctness
- Rewrite the tool-catalog loader to parse the current tools.yaml map schema
  (cat/in/out/def/params) instead of the stale flat-list format that made
  init() panic. Embed tools.yaml via go:embed so the binary is
  self-contained and runs from any directory.
- Fix internal/tui/builder.go: add the missing selNode field, migrate to the
  bubbletea v1.3.5 mouse API, replace the non-existent list.Separator and
  lipgloss Unset() usages, and resolve pipe-type checks through the DAG.
- Add DAG helpers MaxLayer/RemoveFromLayer/InsertAtLayer used by the builder.
- Fix GetParallelNodes so all parallel nodes in a layer share one concurrent
  group instead of being split into sequential steps.

Execution engine
- Unify placeholder handling: both {{input}}/{{domain}}/{{output}} and
  $(target_file)/$(target)/$(output) are now supported (substituteArgs).
- Capture tool stdout into the output file when no output placeholder is
  present, so stdout-streaming tools (e.g. "-o -") actually produce results.
- Merge and de-duplicate every tool's output in a layer before feeding the
  next layer, instead of forwarding only the first tool's file.

Headless CLI
- cmd/termaid now supports run/preview/tools/validate/version/help
  subcommands and still launches the TUI when invoked with no arguments,
  making termaid scriptable and CI-friendly.

Quality
- Add unit tests for graph, catalog, and pipeline.
- Add a GitHub Actions CI workflow (gofmt/vet/test -race/build), a Makefile,
  and a .gitignore.
- gofmt the whole tree; remove the accidental empty project.zip.
- Update README to document the CLI, placeholder styles, and stdout capture.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQHqzzjxxDUvDePyPerast
@sourcery-ai

sourcery-ai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Reviewer's Guide

Restores a working build, modernizes the TUI builder and DAG execution semantics, and adds a headless CLI plus tests/CI by fixing catalog loading, dataflow, and placeholder handling while wiring the graph/pipeline layers together.

Sequence diagram for headless CLI run execution

sequenceDiagram
  actor User
  participant termaid_main
  participant tui_RunHeadless as tui_RunHeadless
  participant tui_LoadWorkflow as tui_LoadWorkflow
  participant graph_DAG as graph_DAG
  participant pipeline_Run as pipeline_Run

  User->>termaid_main: termaid run -d domain -w workflow.json
  termaid_main->>tui_RunHeadless: RunHeadless(ctx, path, domain, workdir, conc, writer)
  tui_RunHeadless->>tui_LoadWorkflow: LoadWorkflow(path)
  tui_LoadWorkflow->>graph_DAG: construct DAG from workflow
  tui_LoadWorkflow-->>tui_RunHeadless: *graph.DAG
  tui_RunHeadless->>pipeline_Run: Run(ctx, domain, workdir, cats, conc, statusCh)
  pipeline_Run-->>tui_RunHeadless: Status events
  tui_RunHeadless-->>termaid_main: error or nil
  termaid_main-->>User: exit code and logs
Loading

File-Level Changes

Change Details Files
Rewrite tool catalog loading to use embedded tools.yaml map schema and expose normalized catalog data to TUI and headless consumers.
  • Introduce assets package that go:embeds assets/tools.yaml into the binary
  • Replace old flat-list YAML parsing with a map-based rawTool schema that captures cat/in/out/def/params
  • Derive catalogEntry structures (including DefArr and type info) and build catalog/catalo gMap indices
  • Normalize catalog placeholders from $(...) form to {{...}} for builder defaults and add tests for catalog parsing
assets/embed.go
internal/tui/catalog.go
internal/tui/catalog_test.go
Fix and extend DAG graph utilities and ensure execution ordering/parallelism semantics match the TUI builder and pipeline.
  • Add MaxLayer, RemoveFromLayer, InsertAtLayer helpers and use them from builder logic
  • Rework GetParallelNodes to group all parallel nodes in a layer into a single concurrent group and keep sequential nodes as their own groups
  • Tighten matrix validation, rendering, and JSON export logic and add unit tests covering add/remove, execution order, and insertion semantics
internal/graph/dag.go
internal/graph/render.go
internal/graph/dag_test.go
Adjust the TUI builder to the updated graph and bubbletea APIs while adding type-aware piping and better mouse handling.
  • Add selNode tracking to BuilderModel and use DAG helpers for node selection and placement
  • Replace deprecated mouse constants with v1.3.5 MouseMsg fields and adjust canvas hit-testing/scrolling
  • Remove use of list.Separator in favor of a custom sepItem type and make toolDelegate robust to mixed item types
  • Implement canPipe on BuilderModel using catalogMap in/out types, treating root as domain and handling wildcard/raw types
  • Fix ANSI stripping using a regex instead of lipgloss.Unset() which no longer exists
internal/tui/builder.go
Unify placeholder handling and layer output merging in the execution pipeline so tools using either placeholder dialect and stdout-only output behave correctly.
  • Introduce substituteArgs to resolve both {{input}}/{{domain}}/{{output}} and $(target_file)/$(target)/$(output) placeholders across tool args
  • Add writesOwnFile to detect explicit output placeholders and gate stdout redirection
  • Capture stdout into the tool output file when the tool does not write its own file, enabling tools that stream to stdout
  • Change per-layer dataflow to merge all tool outputs in a category directory into a single deduplicated file feeding the next layer, and add tests for substitution, merging, and tool validation
internal/pipeline/pipeline.go
internal/pipeline/dataflow.go
internal/pipeline/pipeline_test.go
Add a headless CLI around the TUI/pipeline stack to support run/preview/tools/validate/version/help commands and make the tool CI/script friendly.
  • Refactor cmd/termaid/main.go into a subcommand dispatcher that launches the TUI when no args are provided and otherwise routes to cmdRun/cmdPreview/cmdTools/cmdValidate
  • Implement RunHeadless in internal/tui/headless.go to wire workflow loading, DAG-to-category conversion, pipeline.Run, and human-readable status streaming
  • Expose catalog summaries, Mermaid rendering, and workflow validation helpers for headless use
  • Update README with headless usage examples, placeholder documentation, stdout capture semantics, and Go version bump
cmd/termaid/main.go
internal/tui/headless.go
internal/tui/menu.go
README.md
Introduce CI, Makefile, Go version bump, and general formatting/cleanup to stabilize the project.
  • Add GitHub Actions workflow that enforces gofmt, go vet, go test -race, and go build on pushes/PRs
  • Add a Makefile with standard targets (build/install/test/race/vet/fmt/fmt-check/tidy/run/clean/help)
  • Update .gitignore, remove the stray project.zip, standardize gofmt across the tree, and bump README Go requirement to 1.24+
.github/workflows/ci.yml
.gitignore
Makefile
README.md
project.zip

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 81138085-7628-4e6f-8bbe-a5efbdaa5924

📥 Commits

Reviewing files that changed from the base of the PR and between 3b5db90 and bbd61ce.

⛔ Files ignored due to path filters (1)
  • project.zip is excluded by !**/*.zip
📒 Files selected for processing (22)
  • .github/workflows/ci.yml
  • .gitignore
  • Makefile
  • README.md
  • assets/embed.go
  • cmd/demo/main.go
  • cmd/responsive-demo/main.go
  • cmd/termaid/main.go
  • cmd/tui-test/main.go
  • internal/graph/dag.go
  • internal/graph/dag_test.go
  • internal/graph/render.go
  • internal/pipeline/dataflow.go
  • internal/pipeline/pipeline.go
  • internal/pipeline/pipeline_test.go
  • internal/tui/builder.go
  • internal/tui/catalog.go
  • internal/tui/catalog_test.go
  • internal/tui/headless.go
  • internal/tui/menu.go
  • internal/tui/model.go
  • internal/tui/responsive.go

Walkthrough

The PR adds a command-line interface, headless workflow execution, embedded tool-catalog loading, pipeline output merging, DAG updates, TUI changes, build automation, CI checks, documentation, and formatting cleanup.

Changes

Termaid functional stack

Layer / File(s) Summary
Embedded catalog and catalog resolution
assets/embed.go, internal/tui/catalog.go, internal/tui/catalog_test.go, README.md
The binary embeds tools.yaml. Catalog parsing now resolves tool metadata, descriptions, defaults, types, and placeholders. Tests cover parsing and type compatibility.
DAG mutation and builder integration
internal/graph/dag.go, internal/graph/dag_test.go, internal/tui/builder.go
The DAG adds layer mutation helpers and revised parallel grouping. The builder adds DAG-aware type checks, node selection state, safe category separators, updated mouse handling, and revised canvas coordinates.
Pipeline output flow and headless execution
internal/pipeline/*.go, internal/tui/headless.go, internal/tui/catalog_test.go
Pipeline outputs are merged and deduplicated across category tools. Placeholder substitution supports both dialects. Headless APIs validate workflows, execute them with cancellation support, stream statuses, and report completion.
Command-line entry points and usage
cmd/termaid/main.go, README.md
The CLI supports interactive mode plus run, preview, tools, validate, version, and help commands. The README documents command usage and Go 1.24+ requirements.
Build automation and formatting support
.github/workflows/ci.yml, Makefile, .gitignore, cmd/*, internal/graph/render.go, internal/tui/{menu,model,responsive}.go
Make targets and GitHub Actions run formatting, vetting, tests, race detection, and builds. Ignore rules cover binaries and runtime files. Formatting-only changes affect demos, rendering, menus, models, responsive layout, and dataflow code.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant RunHeadless
  participant Pipeline
  participant StatusWriter
  CLI->>RunHeadless: provide workflow, domain, workdir, and concurrency
  RunHeadless->>Pipeline: load, validate, and execute workflow
  Pipeline-->>StatusWriter: emit start, finish, and error statuses
  RunHeadless-->>CLI: return execution result and completion summary
Loading
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/project-expansion-enhancement-7u8hpv

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@MKlolbullen
MKlolbullen merged commit df2420e into main Aug 21, 2026
3 of 4 checks passed

@sourcery-ai sourcery-ai Bot 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.

Hey - I've found 2 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="internal/graph/dag_test.go" line_range="127-130" />
<code_context>
+	}
+}
+
+func TestToMermaidAndJSON(t *testing.T) {
+	g := NewDAG()
+	_ = g.AddNode("input", "subfinder-1", "subfinder", "-d {{domain}}", 1)
+
+	mmd := g.ToMermaid()
+	if !strings.HasPrefix(mmd, "graph LR") {
+		t.Fatalf("mermaid missing header: %q", mmd)
+	}
+
+	js := g.ToJSON()
+	if !strings.Contains(js, `"subfinder-1"`) || !strings.Contains(js, `"version": "2.0"`) {
+		t.Fatalf("json missing expected content: %s", js)
+	}
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add negative tests for ValidateMatrix to cover matrix inconsistency cases

Current tests only exercise happy-path behavior like ToMermaid/ToJSON and structural helpers. Please add tests that assert ValidateMatrix fails in specific cases, such as:

- Two nodes sharing the same Coordinate when at least one has Parallel == false, verifying the conflict error.
- A node present in g.Nodes but missing from g.Matrix at its (Layer, Position), verifying the "node not found in matrix" path.

These will ensure future DAG mutations keep the matrix and nodes in sync.

```suggestion
	if !strings.Contains(js, `"subfinder-1"`) || !strings.Contains(js, `"version": "2.0"`) {
		t.Fatalf("json missing expected content: %s", js)
	}
}

func TestValidateMatrixRejectsNonParallelCoordinateConflicts(t *testing.T) {
	g := NewDAG()

	if err := g.AddNode("input", "a-1", "a", "", 1); err != nil {
		t.Fatalf("unexpected error adding node a-1: %v", err)
	}
	if err := g.AddNode("a-1", "b-1", "b", "", 1); err != nil {
		t.Fatalf("unexpected error adding node b-1: %v", err)
	}

	// Force both nodes onto the same coordinate, but make at least one non-parallel.
	a := g.Nodes["a-1"]
	b := g.Nodes["b-1"]

	a.Layer, a.Position = 0, 0
	b.Layer, b.Position = 0, 0

	// Explicitly set parallel flags to exercise the conflict path.
	a.Parallel = false
	b.Parallel = true

	g.Nodes["a-1"] = a
	g.Nodes["b-1"] = b

	// Manually construct a conflicting matrix cell.
	if g.Matrix == nil {
		g.Matrix = make(map[int]map[int][]string)
	}
	if g.Matrix[0] == nil {
		g.Matrix[0] = make(map[int][]string)
	}
	g.Matrix[0][0] = []string{"a-1", "b-1"}

	if err := g.ValidateMatrix(); err == nil {
		t.Fatal("expected ValidateMatrix to fail for non-parallel nodes sharing a coordinate")
	}
}

func TestValidateMatrixRejectsMissingNodeInMatrix(t *testing.T) {
	g := NewDAG()

	if err := g.AddNode("input", "a-1", "a", "", 1); err != nil {
		t.Fatalf("unexpected error adding node a-1: %v", err)
	}

	// Sanity check: the initial DAG should be valid.
	if err := g.ValidateMatrix(); err != nil {
		t.Fatalf("expected initial matrix to be valid, got error: %v", err)
	}

	// Remove the node from the matrix at its (Layer, Position) but keep it in g.Nodes.
	n := g.Nodes["a-1"]
	layer, pos := n.Layer, n.Position

	cell := g.Matrix[layer][pos]
	var filtered []string
	for _, id := range cell {
		if id != "a-1" {
			filtered = append(filtered, id)
		}
	}
	g.Matrix[layer][pos] = filtered

	if err := g.ValidateMatrix(); err == nil {
		t.Fatal("expected ValidateMatrix to fail when node is missing from matrix at its coordinate")
	}
}
```
</issue_to_address>

### Comment 2
<location path="internal/graph/dag_test.go" line_range="102-26" />
<code_context>
+	}
+}
+
+func TestInsertAtLayerRepositions(t *testing.T) {
+	g := NewDAG()
+	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)
+
+	g.RemoveFromLayer("a-1")
+	g.InsertAtLayer("a-1", 3, 2)
+
+	if n := g.Nodes["a-1"]; n.Layer != 3 || n.Position != 2 {
+		t.Fatalf("node position = (%d,%d), want (3,2)", n.Layer, n.Position)
+	}
+	if g.MaxLayer() != 3 {
+		t.Fatalf("MaxLayer = %d, want 3", g.MaxLayer())
+	}
+}
+
+func TestToMermaidAndJSON(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Exercise MoveNode/CompactLayer and RemoveFromLayer on non-existent IDs to better cover DAG helpers

Please also add tests for the remaining DAG helpers:

- MoveNode and CompactLayer: move nodes across layers/positions, then compact a layer, and assert matrix membership, MaxX/MaxY, and ordering stay consistent.
- RemoveFromLayer with an unknown ID: verify it’s a no-op and does not panic.

This will better validate that the new layout-editing helpers keep the internal matrix consistent.

Suggested implementation:

```golang
func TestInsertAtLayerRepositions(t *testing.T) {
	g := NewDAG()
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)

	g.RemoveFromLayer("a-1")
	g.InsertAtLayer("a-1", 3, 2)

	if n := g.Nodes["a-1"]; n.Layer != 3 || n.Position != 2 {
		t.Fatalf("node position = (%d,%d), want (3,2)", n.Layer, n.Position)
	}
	if g.MaxLayer() != 3 {
		t.Fatalf("MaxLayer = %d, want 3", g.MaxLayer())
	}
}

func TestMoveNodeAndCompactLayer(t *testing.T) {
	g := NewDAG()

	// layer 1: a-1 at pos 0, a-2 at pos 1
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)
	_ = g.AddNodeAtPosition("input", "a-2", "a", "", 1, 1, "", false)

	// layer 2: b-1 at pos 0
	_ = g.AddNodeAtPosition("input", "b-1", "b", "", 2, 0, "", false)

	// sanity: initial max dimensions
	initialMaxX := g.MaxX()
	initialMaxY := g.MaxY()
	if initialMaxY != 2 {
		t.Fatalf("initial MaxY = %d, want 2", initialMaxY)
	}

	// move a-2 from (1,1) -> (2,1)
	g.MoveNode("a-2", 2, 1)

	if n := g.Nodes["a-2"]; n.Layer != 2 || n.Position != 1 {
		t.Fatalf("a-2 position = (%d,%d), want (2,1)", n.Layer, n.Position)
	}

	// compact layer 1 (should only contain a-1 at position 0 after compaction)
	g.CompactLayer(1)

	if n := g.Nodes["a-1"]; n.Layer != 1 || n.Position != 0 {
		t.Fatalf("a-1 position after compact = (%d,%d), want (1,0)", n.Layer, n.Position)
	}

	// verify MaxX/MaxY stay consistent and non-decreasing in terms of capacity
	if maxX, maxY := g.MaxX(), g.MaxY(); maxX < 0 || maxY < 1 {
		t.Fatalf("unexpected MaxX/MaxY after moves/compaction: (%d,%d)", maxX, maxY)
	}

	// ensure moved node is still present in the layout matrix at its new coordinates
	// (implementation relies on the internal matrix representation being indexed by [layer][position])
	if nodeAtNewPos := g.matrix[g.Nodes["a-2"].Layer][g.Nodes["a-2"].Position]; nodeAtNewPos == nil || nodeAtNewPos.ID != "a-2" {
		t.Fatalf("a-2 not found in layout matrix at its new position")
	}
}

func TestRemoveFromLayerUnknownIDIsNoOp(t *testing.T) {
	g := NewDAG()
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)
	_ = g.AddNodeAtPosition("input", "b-1", "b", "", 2, 0, "", false)

	beforeNodes := len(g.Nodes)
	beforeMaxLayer := g.MaxLayer()
	beforeMaxX := g.MaxX()
	beforeMaxY := g.MaxY()

	// removing an unknown ID should be a no-op and must not panic
	g.RemoveFromLayer("non-existent-id")

	if len(g.Nodes) != beforeNodes {
		t.Fatalf("RemoveFromLayer(non-existent) changed node count: got %d, want %d", len(g.Nodes), beforeNodes)
	}
	if g.MaxLayer() != beforeMaxLayer {
		t.Fatalf("RemoveFromLayer(non-existent) changed MaxLayer: got %d, want %d", g.MaxLayer(), beforeMaxLayer)
	}
	if g.MaxX() != beforeMaxX || g.MaxY() != beforeMaxY {
		t.Fatalf("RemoveFromLayer(non-existent) changed MaxX/MaxY: got (%d,%d), want (%d,%d)",
			g.MaxX(), g.MaxY(), beforeMaxX, beforeMaxY)
	}
}

func TestToMermaidAndJSON(t *testing.T) {

```

The new tests assume:
1. `MoveNode(id string, layer, position int)` and `CompactLayer(layer int)` exist and do not return errors.
2. `MaxX()` and `MaxY()` are defined DAG helpers returning the maximum X (position) and Y (layer) extents.
3. The internal layout matrix is accessible as `g.matrix[layer][position]` and stores nodes with an `ID` field; if your actual matrix representation differs (e.g., different field name or structure), update the access in `TestMoveNodeAndCompactLayer` accordingly.
4. If `MoveNode`, `CompactLayer`, `MaxX`, or `MaxY` can return errors or have different signatures, adjust the test calls to handle those return values while preserving the assertions about node positions, matrix membership, and dimension consistency.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +127 to +130
if !strings.Contains(js, `"subfinder-1"`) || !strings.Contains(js, `"version": "2.0"`) {
t.Fatalf("json missing expected content: %s", js)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Add negative tests for ValidateMatrix to cover matrix inconsistency cases

Current tests only exercise happy-path behavior like ToMermaid/ToJSON and structural helpers. Please add tests that assert ValidateMatrix fails in specific cases, such as:

  • Two nodes sharing the same Coordinate when at least one has Parallel == false, verifying the conflict error.
  • A node present in g.Nodes but missing from g.Matrix at its (Layer, Position), verifying the "node not found in matrix" path.

These will ensure future DAG mutations keep the matrix and nodes in sync.

Suggested change
if !strings.Contains(js, `"subfinder-1"`) || !strings.Contains(js, `"version": "2.0"`) {
t.Fatalf("json missing expected content: %s", js)
}
}
if !strings.Contains(js, `"subfinder-1"`) || !strings.Contains(js, `"version": "2.0"`) {
t.Fatalf("json missing expected content: %s", js)
}
}
func TestValidateMatrixRejectsNonParallelCoordinateConflicts(t *testing.T) {
g := NewDAG()
if err := g.AddNode("input", "a-1", "a", "", 1); err != nil {
t.Fatalf("unexpected error adding node a-1: %v", err)
}
if err := g.AddNode("a-1", "b-1", "b", "", 1); err != nil {
t.Fatalf("unexpected error adding node b-1: %v", err)
}
// Force both nodes onto the same coordinate, but make at least one non-parallel.
a := g.Nodes["a-1"]
b := g.Nodes["b-1"]
a.Layer, a.Position = 0, 0
b.Layer, b.Position = 0, 0
// Explicitly set parallel flags to exercise the conflict path.
a.Parallel = false
b.Parallel = true
g.Nodes["a-1"] = a
g.Nodes["b-1"] = b
// Manually construct a conflicting matrix cell.
if g.Matrix == nil {
g.Matrix = make(map[int]map[int][]string)
}
if g.Matrix[0] == nil {
g.Matrix[0] = make(map[int][]string)
}
g.Matrix[0][0] = []string{"a-1", "b-1"}
if err := g.ValidateMatrix(); err == nil {
t.Fatal("expected ValidateMatrix to fail for non-parallel nodes sharing a coordinate")
}
}
func TestValidateMatrixRejectsMissingNodeInMatrix(t *testing.T) {
g := NewDAG()
if err := g.AddNode("input", "a-1", "a", "", 1); err != nil {
t.Fatalf("unexpected error adding node a-1: %v", err)
}
// Sanity check: the initial DAG should be valid.
if err := g.ValidateMatrix(); err != nil {
t.Fatalf("expected initial matrix to be valid, got error: %v", err)
}
// Remove the node from the matrix at its (Layer, Position) but keep it in g.Nodes.
n := g.Nodes["a-1"]
layer, pos := n.Layer, n.Position
cell := g.Matrix[layer][pos]
var filtered []string
for _, id := range cell {
if id != "a-1" {
filtered = append(filtered, id)
}
}
g.Matrix[layer][pos] = filtered
if err := g.ValidateMatrix(); err == nil {
t.Fatal("expected ValidateMatrix to fail when node is missing from matrix at its coordinate")
}
}

if kids := g.Nodes["input"].Children; len(kids) != 1 || kids[0] != "subfinder-1" {
t.Fatalf("input children = %v, want [subfinder-1]", kids)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Exercise MoveNode/CompactLayer and RemoveFromLayer on non-existent IDs to better cover DAG helpers

Please also add tests for the remaining DAG helpers:

  • MoveNode and CompactLayer: move nodes across layers/positions, then compact a layer, and assert matrix membership, MaxX/MaxY, and ordering stay consistent.
  • RemoveFromLayer with an unknown ID: verify it’s a no-op and does not panic.

This will better validate that the new layout-editing helpers keep the internal matrix consistent.

Suggested implementation:

func TestInsertAtLayerRepositions(t *testing.T) {
	g := NewDAG()
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)

	g.RemoveFromLayer("a-1")
	g.InsertAtLayer("a-1", 3, 2)

	if n := g.Nodes["a-1"]; n.Layer != 3 || n.Position != 2 {
		t.Fatalf("node position = (%d,%d), want (3,2)", n.Layer, n.Position)
	}
	if g.MaxLayer() != 3 {
		t.Fatalf("MaxLayer = %d, want 3", g.MaxLayer())
	}
}

func TestMoveNodeAndCompactLayer(t *testing.T) {
	g := NewDAG()

	// layer 1: a-1 at pos 0, a-2 at pos 1
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)
	_ = g.AddNodeAtPosition("input", "a-2", "a", "", 1, 1, "", false)

	// layer 2: b-1 at pos 0
	_ = g.AddNodeAtPosition("input", "b-1", "b", "", 2, 0, "", false)

	// sanity: initial max dimensions
	initialMaxX := g.MaxX()
	initialMaxY := g.MaxY()
	if initialMaxY != 2 {
		t.Fatalf("initial MaxY = %d, want 2", initialMaxY)
	}

	// move a-2 from (1,1) -> (2,1)
	g.MoveNode("a-2", 2, 1)

	if n := g.Nodes["a-2"]; n.Layer != 2 || n.Position != 1 {
		t.Fatalf("a-2 position = (%d,%d), want (2,1)", n.Layer, n.Position)
	}

	// compact layer 1 (should only contain a-1 at position 0 after compaction)
	g.CompactLayer(1)

	if n := g.Nodes["a-1"]; n.Layer != 1 || n.Position != 0 {
		t.Fatalf("a-1 position after compact = (%d,%d), want (1,0)", n.Layer, n.Position)
	}

	// verify MaxX/MaxY stay consistent and non-decreasing in terms of capacity
	if maxX, maxY := g.MaxX(), g.MaxY(); maxX < 0 || maxY < 1 {
		t.Fatalf("unexpected MaxX/MaxY after moves/compaction: (%d,%d)", maxX, maxY)
	}

	// ensure moved node is still present in the layout matrix at its new coordinates
	// (implementation relies on the internal matrix representation being indexed by [layer][position])
	if nodeAtNewPos := g.matrix[g.Nodes["a-2"].Layer][g.Nodes["a-2"].Position]; nodeAtNewPos == nil || nodeAtNewPos.ID != "a-2" {
		t.Fatalf("a-2 not found in layout matrix at its new position")
	}
}

func TestRemoveFromLayerUnknownIDIsNoOp(t *testing.T) {
	g := NewDAG()
	_ = g.AddNodeAtPosition("input", "a-1", "a", "", 1, 0, "", false)
	_ = g.AddNodeAtPosition("input", "b-1", "b", "", 2, 0, "", false)

	beforeNodes := len(g.Nodes)
	beforeMaxLayer := g.MaxLayer()
	beforeMaxX := g.MaxX()
	beforeMaxY := g.MaxY()

	// removing an unknown ID should be a no-op and must not panic
	g.RemoveFromLayer("non-existent-id")

	if len(g.Nodes) != beforeNodes {
		t.Fatalf("RemoveFromLayer(non-existent) changed node count: got %d, want %d", len(g.Nodes), beforeNodes)
	}
	if g.MaxLayer() != beforeMaxLayer {
		t.Fatalf("RemoveFromLayer(non-existent) changed MaxLayer: got %d, want %d", g.MaxLayer(), beforeMaxLayer)
	}
	if g.MaxX() != beforeMaxX || g.MaxY() != beforeMaxY {
		t.Fatalf("RemoveFromLayer(non-existent) changed MaxX/MaxY: got (%d,%d), want (%d,%d)",
			g.MaxX(), g.MaxY(), beforeMaxX, beforeMaxY)
	}
}

func TestToMermaidAndJSON(t *testing.T) {

The new tests assume:

  1. MoveNode(id string, layer, position int) and CompactLayer(layer int) exist and do not return errors.
  2. MaxX() and MaxY() are defined DAG helpers returning the maximum X (position) and Y (layer) extents.
  3. The internal layout matrix is accessible as g.matrix[layer][position] and stores nodes with an ID field; if your actual matrix representation differs (e.g., different field name or structure), update the access in TestMoveNodeAndCompactLayer accordingly.
  4. If MoveNode, CompactLayer, MaxX, or MaxY can return errors or have different signatures, adjust the test calls to handle those return values while preserving the assertions about node positions, matrix membership, and dimension consistency.

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