Adopt Lipgloss tree rendering for MCP hierarchy and status dependencies#33276
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Introduces Lipgloss tree rendering to make nested CLI output easier to read, adding (1) an MCP inspection hierarchy summary and (2) a verbose workflow dependency tree in status, plus accompanying unit tests.
Changes:
- Add a tree-rendered MCP “server hierarchy” summary to
mcp inspect. - Extend
statusto derive workflow dependencies (frontmatter + inline directives) and render them as a dependency tree in verbose mode. - Add focused unit tests for MCP tree content and dependency extraction/tree rendering; fix a Datadog env var reference in a locked workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/status_command.go |
Adds dependency extraction/normalization and verbose dependency-tree rendering; extends WorkflowStatus with derived dependencies. |
pkg/cli/status_dependency_tree_test.go |
Adds unit tests for dependency extraction and dependency tree rendering behavior. |
pkg/cli/mcp_inspect.go |
Adds Lipgloss-based hierarchy tree rendering for MCP inspection output and engine resolution helper. |
pkg/cli/mcp_inspect_tree_test.go |
Adds unit tests for MCP hierarchy tree output and engine ID resolution. |
.github/workflows/smoke-otel-backends.lock.yml |
Updates Datadog header env var interpolation to use DD_APPLICATION_KEY. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| if frontmatter != nil { | ||
| switch imports := frontmatter["imports"].(type) { | ||
| case string: | ||
| addDependency(imports) | ||
| case []any: | ||
| for _, item := range imports { | ||
| switch importItem := item.(type) { | ||
| case string: | ||
| addDependency(importItem) | ||
| case map[string]any: | ||
| if uses, ok := importItem["uses"].(string); ok { | ||
| addDependency(uses) | ||
| } | ||
| } | ||
| } | ||
| } |
| serversTree := tree.Root("MCP Servers") | ||
| for _, config := range mcpConfigs { | ||
| serversTree.Child(fmt.Sprintf("%s (%s)", config.Name, config.Type)) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out based on this being a refactor + new feature (tree rendering + dependency extraction).
Key Themes
Test Coverage Gaps — The PR includes tests but misses important edge cases:
extractWorkflowDependencieshandles complex union types (string | array | map) but tests only cover the happy pathresolveWorkflowEngineIDhas an untested fallback edge case (non-nil EngineConfig with empty ID)- Missing tests for error scenarios in dependency extraction
Silent Error Handling — findIncludesInContent errors are swallowed without logging, creating an observability gap. Users won't know why their dependency tree is incomplete if inline directive parsing fails.
Implicit Design Decisions — The tree hierarchy rendering is conditionally suppressed when filters are applied, but this behavior isn't documented in code. Future maintainers may be confused.
Architectural Question — Fragment identifiers (#section) are stripped during normalization. This may be fine now, but if section-level dependency analysis becomes valuable later, the data will be lost.
Positive Highlights
- ✅ Clean tree composition — Excellent use of Lipgloss tree builder API with method chaining
- ✅ Consistent styling — Reuses existing
styles.TreeEnumeratorandstyles.TreeNodefor visual consistency - ✅ Proper separation — New functions are focused and single-purpose (
resolveWorkflowEngineID,normalizeWorkflowDependency) - ✅ Thoughtful UX — Only showing hierarchy in unfiltered mode avoids visual clutter
- ✅ Good test structure — Table-driven tests with descriptive names in
mcp_inspect_tree_test.go
Verdict
Comments only — no blocking issues. The changes are solid; suggestions focus on hardening edge cases and improving observability for future maintainers.
Learn more about these skills:
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.4M
| return isCompiledUpToDateWithCache(workflowPath, lockFilePath, parser.NewImportCache("")) | ||
| } | ||
|
|
||
| func extractWorkflowDependencies(content string, frontmatter map[string]any) []string { |
There was a problem hiding this comment.
[/tdd] The extractWorkflowDependencies function has complex branching logic (string vs array vs map imports, content parsing) but isn't directly tested with unit tests. The test in status_dependency_tree_test.go covers the happy path but doesn't validate edge cases like:
- Malformed frontmatter imports
- Empty strings in import arrays
findIncludesInContentreturning an error- Duplicate dependencies from different sources
Consider adding a dedicated test file extract_workflow_dependencies_test.go with table-driven tests:
func TestExtractWorkflowDependencies_EdgeCases(t *testing.T) {
tests := []struct {
name string
content string
frontmatter map[string]any
want []string
}{
{
name: "handles empty import strings",
frontmatter: map[string]any{"imports": []any{"", " ", "valid.md"}},
want: []string{"valid.md"},
},
{
name: "deduplicates across sources",
frontmatter: map[string]any{"imports": "base.md#section"},
content: "`@include` base.md\n",
want: []string{"base.md"},
},
}
// ...
}This would catch future regressions when the import parsing logic evolves.
| } | ||
|
|
||
| includes, err := findIncludesInContent(content) | ||
| if err == nil { |
There was a problem hiding this comment.
[/zoom-out] Silent error swallowing here: findIncludesInContent errors are ignored with if err == nil. This pattern hides potential parsing failures from users.
From the broader architecture perspective, this creates an inconsistency: frontmatter import parsing failures would likely surface elsewhere, but inline directive failures are silently dropped. Users might see incomplete dependency trees without understanding why.
Consider at minimum logging the error:
includes, err := findIncludesInContent(content)
if err != nil {
statusLog.Printf("failed to extract includes from content: %v", err)
} else {
for _, include := range includes {
addDependency(include)
}
}This maintains the graceful degradation while providing observability.
| String() | ||
| } | ||
|
|
||
| func resolveWorkflowEngineID(workflowData *workflow.WorkflowData) string { |
There was a problem hiding this comment.
[/tdd] The resolveWorkflowEngineID function has a clear fallback chain but the test only validates one path through the logic per test case. The function returns "unknown" in two scenarios:
workflowData == nilworkflowData.EngineConfig == nil && workflowData.AI == ""
But there's an untested edge case: what if workflowData.EngineConfig is non-nil but ID is empty string? Add a test case:
{
name: "empty engine config id falls back to ai",
workflowData: &workflow.WorkflowData{
EngineConfig: &workflow.EngineConfig{ID: ""},
AI: "claude",
},
want: "claude",
},This documents the expected behavior and prevents future refactors from accidentally breaking the fallback logic.
| return dependencies | ||
| } | ||
|
|
||
| func normalizeWorkflowDependency(dependency string) string { |
There was a problem hiding this comment.
[/zoom-out] The normalizeWorkflowDependency function uses strings.Cut to strip fragment identifiers (#section), but there's an architectural question: should fragment information be preserved somewhere?
In a larger codebase, workflow dependencies on specific sections might be valuable for impact analysis ("which workflows depend on section X of base.md?"). The current implementation discards that granularity.
If section-level dependencies aren't needed, this is fine. But if they matter for future features (e.g., gh aw dependencies --section base.md#helpers), consider preserving the raw dependency alongside the normalized one:
type WorkflowDependency struct {
Path string // normalized: "base.md"
Fragment string // optional: "helpers"
}No immediate action needed, but document this design decision for future maintainers.
| } else { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Found %d MCP server(s) to inspect", len(mcpConfigs)))) | ||
| } | ||
| if toolFilter == "" { |
There was a problem hiding this comment.
[/zoom-out] Conditional rendering based on toolFilter == "" creates an implicit coupling: the tree hierarchy is only shown when no tool filter is applied.
This makes sense (filtered views shouldn't show full hierarchy), but the logic is implicit. A future maintainer might wonder why the hierarchy disappeared when they added a tool filter.
Consider adding a comment explaining the design decision:
// Only render hierarchy in unfiltered mode - filtered views focus on specific tools
if toolFilter == "" {
if hierarchy := renderMCPInspectionTree(...Or extract to a named boolean:
showHierarchy := toolFilter == "" && serverFilter == ""
if showHierarchy {
// ...
}
Generated by the Design Decision Gate workflow to record the architectural decision to adopt charm.land/lipgloss/v2/tree for hierarchical CLI output in mcp inspect and status commands. This is a DRAFT — PR author must review and finalize before merge.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (277 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
This repository's ADRs are stored in 🔎 Why this PR triggered the gate
References:
|
|
@copilot review all comments |
🧪 Test Quality Sentinel ReportTest Quality Score: 82/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires Review🚨 Missing Build Tags (Coding Guideline Violation)Both new test files are missing the mandatory
Required fix: Add Why this matters: The build tag determines which test suite (unit vs integration) the tests belong to. Without it, the tests cannot be properly categorized and may be skipped by CI. 🚨 Missing Assertion Messages (Coding Guideline Violation)All 5 test functions use bare assertions without descriptive message arguments. The project guideline requires every testify assertion to include a descriptive message. Examples from the code: // ❌ INCORRECT - no message
assert.Contains(t, result, part)
assert.Equal(t, tt.want, resolveWorkflowEngineID(tt.workflowData))
assert.Len(t, got, len(want))
// ✅ CORRECT - includes descriptive context
assert.Contains(t, result, part, "tree output should include expected hierarchy node")
assert.Equal(t, tt.want, resolveWorkflowEngineID(tt.workflowData), "engine ID resolution should match expected fallback behavior")
assert.Len(t, got, len(want), "dependency count should match expected unique set")Required fix: Add descriptive message arguments to all assertion calls explaining what the assertion validates in business terms.
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: Coding guideline violations detected. Both new test files are missing the mandatory //go:build !integration build tag on line 1, and all 5 tests use bare assertions without descriptive message arguments. Please fix these violations before merging — the build tag issue will cause CI failures.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e order Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the review items in commit c1db3d0.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile fix conflicts |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…tree-for-hierarchy Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
Current CLI output mixed manual hierarchy formatting with table-only views, making nested relationships harder to read. This change introduces
lipglosstree rendering for MCP inspection hierarchy and workflow dependency visibility in status output.MCP inspect: structured hierarchy via Lipgloss Tree
mcp inspectshowing:TreeEnumerator,TreeNode) for consistent terminal rendering.Status command: dependency tree in verbose mode
imports(string/list/object forms)@include,@import)#sectionstripping), deduplicates, and sorts dependencies before rendering.Supporting helpers and coverage