From 48f5c786096890edcbaaee879cf41caace1c59ce Mon Sep 17 00:00:00 2001 From: David Arce Date: Sun, 10 May 2026 17:58:50 +0200 Subject: [PATCH 1/2] feat(tui): add Upgrade Tools menu with parallel upgrade engine - Add ToolRef schema (name + command) to UserManifest with validation - Embed built-in tool catalog (tools/engram.yaml, tools/crit.yaml) - Add internal/tui/tool_catalog.go loader for embedded catalog - Persist manifest.Tools after wizard confirm, preserving user overrides - Add internal/tui/steps/tool_upgrade.go with parallel upgrade engine using injected executor for deterministic tests - Render '(no upgradable)' badge for tools with no effective command - Show empty-state and confirm gate before executing upgrades - Wire 'Upgrade Tools' option into main TUI menu --- internal/cli/advisors_catalog_test.go | 2 +- internal/cli/advisors_sync_test.go | 16 +- internal/cli/init.go | 2 +- internal/cli/menu.go | 45 ++ internal/materialize/linker_test.go | 4 +- internal/materialize/materializer.go | 2 +- internal/materialize/renderers/codex_test.go | 28 +- internal/materialize/renderers/copilot.go | 1 - internal/model/content.go | 8 +- internal/model/manifest.go | 39 ++ internal/model/manifest_test.go | 73 +++ internal/model/opencode_models_test.go | 4 +- internal/model/workflow.go | 2 +- internal/parse/frontmatter_test.go | 20 +- internal/parse/lockfile_test.go | 4 +- internal/parse/manifest_test.go | 117 +++++ internal/recommend/advisor_filter_test.go | 14 +- internal/recommend/cache.go | 2 +- internal/recommend/prompt.go | 12 +- internal/resolve/resolver.go | 10 +- internal/resolve/workflow_expander_test.go | 1 - internal/tui/app.go | 49 ++ internal/tui/app_test.go | 85 ++++ internal/tui/scanner.go | 14 +- internal/tui/steps/confirm.go | 1 - internal/tui/steps/install_spinner_test.go | 1 - internal/tui/steps/recommend.go | 4 +- internal/tui/steps/sdd_info_test.go | 6 +- internal/tui/steps/tool_upgrade.go | 476 +++++++++++++++++++ internal/tui/steps/tool_upgrade_test.go | 311 ++++++++++++ internal/tui/steps/workflow_models.go | 1 - internal/tui/styles.go | 2 +- internal/tui/tool_catalog.go | 71 +++ internal/tui/tool_catalog_test.go | 53 +++ testdata/manifests/valid-full.yaml | 6 + tooldefs.go | 14 + tools/crit.yaml | 4 + tools/engram.yaml | 4 + 38 files changed, 1425 insertions(+), 83 deletions(-) create mode 100644 internal/tui/steps/tool_upgrade.go create mode 100644 internal/tui/steps/tool_upgrade_test.go create mode 100644 internal/tui/tool_catalog.go create mode 100644 internal/tui/tool_catalog_test.go create mode 100644 tooldefs.go create mode 100644 tools/crit.yaml create mode 100644 tools/engram.yaml diff --git a/internal/cli/advisors_catalog_test.go b/internal/cli/advisors_catalog_test.go index a6594bd..fb68ad7 100644 --- a/internal/cli/advisors_catalog_test.go +++ b/internal/cli/advisors_catalog_test.go @@ -486,7 +486,7 @@ func TestImportFromLocalCatalog_RendererSpyCalled(t *testing.T) { seedCatalogAdvisor(t, catalogRoot, "spy-beta-advisor", "Beta", false) src := AnAdvisorSource(). - WithSource("local:" + catalogRoot). + WithSource("local:"+catalogRoot). WithSelect("spy-alpha-advisor", "spy-beta-advisor"). Build() manifest := AUserManifest().WithAdvisorSource(src).Build() diff --git a/internal/cli/advisors_sync_test.go b/internal/cli/advisors_sync_test.go index a06278d..8f87a43 100644 --- a/internal/cli/advisors_sync_test.go +++ b/internal/cli/advisors_sync_test.go @@ -46,10 +46,10 @@ func (f *fakeAdvisorRenderer) RegenerateAdvisorFiles( } // AgentRenderer stubs — minimum needed to satisfy the interface. -func (f *fakeAdvisorRenderer) Name() string { return "fake" } -func (f *fakeAdvisorRenderer) AgentType() string { return "fake" } -func (f *fakeAdvisorRenderer) NeedsCopyMode() bool { return false } -func (f *fakeAdvisorRenderer) Definition() model.AgentDefinition { return model.AgentDefinition{} } +func (f *fakeAdvisorRenderer) Name() string { return "fake" } +func (f *fakeAdvisorRenderer) AgentType() string { return "fake" } +func (f *fakeAdvisorRenderer) NeedsCopyMode() bool { return false } +func (f *fakeAdvisorRenderer) Definition() model.AgentDefinition { return model.AgentDefinition{} } func (f *fakeAdvisorRenderer) WorkspacePaths() materialize.AgentPaths { return materialize.AgentPaths{} } @@ -72,10 +72,10 @@ func (f *fakeAdvisorRenderer) Finalize(_ string) error { return nil } // materialize.AdvisorRenderer — used to verify the silent-skip path. type nonAdvisorRenderer struct{} -func (n *nonAdvisorRenderer) Name() string { return "non-advisor" } -func (n *nonAdvisorRenderer) AgentType() string { return "non-advisor" } -func (n *nonAdvisorRenderer) NeedsCopyMode() bool { return false } -func (n *nonAdvisorRenderer) Definition() model.AgentDefinition { return model.AgentDefinition{} } +func (n *nonAdvisorRenderer) Name() string { return "non-advisor" } +func (n *nonAdvisorRenderer) AgentType() string { return "non-advisor" } +func (n *nonAdvisorRenderer) NeedsCopyMode() bool { return false } +func (n *nonAdvisorRenderer) Definition() model.AgentDefinition { return model.AgentDefinition{} } func (n *nonAdvisorRenderer) WorkspacePaths() materialize.AgentPaths { return materialize.AgentPaths{} } diff --git a/internal/cli/init.go b/internal/cli/init.go index 6a6377e..7a9fe2c 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -116,6 +116,7 @@ func runInit(cmd *cobra.Command, args []string) error { Agents: existingAgents, Sources: existingSources, WorkflowModels: mergeWorkflowModels(existingManifest.Workflows), + Tools: existingManifest.Tools, } } } @@ -274,7 +275,6 @@ func runInit(cmd *cobra.Command, args []string) error { return nil } - // printDone writes a styled "completed" step line with a green checkmark. func printDone(out io.Writer, msg string) { _, _ = fmt.Fprintln(out, tuistyles.StyleSuccess.Foreground(tuistyles.ColorSuccess).Render(" ✓ ")+tuistyles.StyleSummaryValue.Render(msg)) diff --git a/internal/cli/menu.go b/internal/cli/menu.go index a600e99..b473ba3 100644 --- a/internal/cli/menu.go +++ b/internal/cli/menu.go @@ -32,6 +32,7 @@ const ( menuActionConfigureModels menuAction = "configure-models" menuActionManageAdvisors menuAction = "manage-advisors" menuActionUpgrade menuAction = "upgrade" + menuActionUpgradeTools menuAction = "upgrade-tools" menuActionUninstall menuAction = "uninstall" menuActionQuit menuAction = "quit" ) @@ -69,6 +70,7 @@ func buildMenuOptions(hasRouting bool) []huh.Option[menuAction] { opts = append(opts, huh.NewOption("Manage SDD advisors", menuActionManageAdvisors), huh.NewOption("Status", menuActionStatus), + huh.NewOption("Upgrade Tools", menuActionUpgradeTools), huh.NewOption("Upgrade DevRune", menuActionUpgrade), huh.NewOption("Uninstall", menuActionUninstall), huh.NewOption("Quit", menuActionQuit), @@ -148,6 +150,12 @@ func RunMenu(cmd *cobra.Command) error { } // Loop back to menu. + case menuActionUpgradeTools: + if err := runUpgradeToolsFromMenu(cmd); err != nil { + _ = showMenuMessage(cmd, "Upgrade Tools Failed", err.Error()) + } + // Loop back to menu. + case menuActionUpgrade: // Upgrade: if user confirms, binary is replaced and we exit. // If user cancels, loop back to menu. @@ -360,6 +368,7 @@ func runInitFromMenu(cmd *cobra.Command) error { Agents: existingAgents, Sources: existingSources, WorkflowModels: mergeWorkflowModels(existingManifest.Workflows), + Tools: existingManifest.Tools, } catalogSources = existingManifest.Catalogs } @@ -441,3 +450,39 @@ func runInitFromMenu(cmd *cobra.Command) error { return nil } + +// runUpgradeToolsFromMenu reads devrune.yaml, loads the embedded tool catalog, +// and delegates to steps.RunToolUpgradeStep to run the TUI upgrade flow. +func runUpgradeToolsFromMenu(cmd *cobra.Command) error { + wd := workingDir(cmd) + manifestPath := filepath.Join(wd, "devrune.yaml") + + // Verify manifest exists. + if _, err := os.Stat(manifestPath); err != nil { + return showMenuMessage(cmd, "Upgrade Tools", "devrune.yaml not found — run New setup first") + } + + // Read and parse manifest. + manifestData, err := os.ReadFile(manifestPath) + if err != nil { + return fmt.Errorf("read manifest: %w", err) + } + manifest, err := parse.ParseManifest(manifestData) + if err != nil { + return fmt.Errorf("parse manifest: %w", err) + } + + // Load embedded tool catalog and build lookup map. + builtinTools, err := tui.LoadBuiltinTools() + if err != nil { + return fmt.Errorf("load tool catalog: %w", err) + } + catalogMap := tui.BuiltinToolMap(builtinTools) + + // Run the upgrade step (preview → confirm → execute → summary). + if _, err := steps.RunToolUpgradeStep(manifest.Tools, catalogMap); err != nil { + return err + } + + return nil +} diff --git a/internal/materialize/linker_test.go b/internal/materialize/linker_test.go index b5aff09..fc1fb87 100644 --- a/internal/materialize/linker_test.go +++ b/internal/materialize/linker_test.go @@ -22,8 +22,8 @@ func TestNewLinker(t *testing.T) { {"hardlink", "hardlink", false}, {"", "symlink", false}, // empty defaults to symlink {"invalid", "", true}, - {"SYMLINK", "", true}, // case-sensitive - {"Copy", "", true}, // case-sensitive + {"SYMLINK", "", true}, // case-sensitive + {"Copy", "", true}, // case-sensitive } for _, tt := range tests { diff --git a/internal/materialize/materializer.go b/internal/materialize/materializer.go index 7b3ca37..77f75b7 100644 --- a/internal/materialize/materializer.go +++ b/internal/materialize/materializer.go @@ -314,7 +314,7 @@ func (m *Materializer) Install( allWorkflows = append(allWorkflows, wfManifest) } - } + } // Step 8: RenderCatalog removed — root catalog is generated after the per-agent loop (T021). diff --git a/internal/materialize/renderers/codex_test.go b/internal/materialize/renderers/codex_test.go index 1d0329e..71b2084 100644 --- a/internal/materialize/renderers/codex_test.go +++ b/internal/materialize/renderers/codex_test.go @@ -19,12 +19,12 @@ import ( // Matches the values in agents/codex.yaml. func codexAgentDef() model.AgentDefinition { return model.AgentDefinition{ - Name: "codex", - Type: "codex", - Workspace: ".codex", - SkillDir: "../.agents/skills", - RulesDir: "rules", - CatalogFile: "AGENTS.md", + Name: "codex", + Type: "codex", + Workspace: ".codex", + SkillDir: "../.agents/skills", + RulesDir: "rules", + CatalogFile: "AGENTS.md", DefaultRules: "individual", MCP: &model.MCPConfig{ FilePath: "config.toml", @@ -175,15 +175,15 @@ func TestCodexRenderer_TransformFrontmatter(t *testing.T) { r := renderers.NewCodexRenderer(codexAgentDef()) input := map[string]interface{}{ - "name": "my-skill", - "description": "A skill", - "allowed-tools": []string{"Bash"}, - "argument-hint": "[topic]", + "name": "my-skill", + "description": "A skill", + "allowed-tools": []string{"Bash"}, + "argument-hint": "[topic]", "disable-model-invocation": false, - "tools-mode": "auto", - "mode": "subagent", - "model": "sonnet", - "temperature": 0.7, + "tools-mode": "auto", + "mode": "subagent", + "model": "sonnet", + "temperature": 0.7, } got := renderers.CodexTransformFrontmatter(r, input) diff --git a/internal/materialize/renderers/copilot.go b/internal/materialize/renderers/copilot.go index 6cd1192..360611f 100644 --- a/internal/materialize/renderers/copilot.go +++ b/internal/materialize/renderers/copilot.go @@ -1183,4 +1183,3 @@ func transformCopilotToolNames(body string) string { } return body } - diff --git a/internal/model/content.go b/internal/model/content.go index b90466c..e835d6b 100644 --- a/internal/model/content.go +++ b/internal/model/content.go @@ -17,11 +17,11 @@ const ( // RuleMeta holds metadata parsed from a rule's frontmatter. // It is populated only for ContentItems with KindRule; nil for skills/prompts/memory. type RuleMeta struct { - Scope string `yaml:"scope"` // e.g. "architecture", "testing", "tech", "api" - Technology string `yaml:"technology"` // e.g. "java", "any" - AppliesTo string `yaml:"applies_to"` // comma-separated skill names + Scope string `yaml:"scope"` // e.g. "architecture", "testing", "tech", "api" + Technology string `yaml:"technology"` // e.g. "java", "any" + AppliesTo string `yaml:"applies_to"` // comma-separated skill names Description string `yaml:"description"` // human-readable description - DisplayName string `yaml:"display_name"` // optional display name from frontmatter; empty string falls back to ContentItem.Name + DisplayName string `yaml:"display_name"` // optional display name from frontmatter; empty string falls back to ContentItem.Name } // ContentItem describes a single discoverable item within a resolved package. diff --git a/internal/model/manifest.go b/internal/model/manifest.go index d71e593..d74de9c 100644 --- a/internal/model/manifest.go +++ b/internal/model/manifest.go @@ -269,6 +269,29 @@ func ReservedAdvisorNames() []string { return out } +// ToolRef is one persisted entry under devrune.yaml tools:. +// Name identifies the tool (required, no leading/trailing whitespace). +// Command is the shell command to upgrade the tool; may be empty or whitespace, +// which means the tool is considered "no upgradable" unless a catalog default exists. +type ToolRef struct { + Name string `yaml:"name"` + Command string `yaml:"command,omitempty"` +} + +// Validate checks that the ToolRef is internally consistent. +// Rules: +// - Name must be non-empty and must not have leading or trailing whitespace. +// - Command may be empty or whitespace (no error; treated as no-command downstream). +func (t ToolRef) Validate() error { + if t.Name == "" { + return fmt.Errorf("manifest: tool name must not be empty") + } + if strings.TrimSpace(t.Name) != t.Name { + return fmt.Errorf("manifest: tool name %q must not have leading or trailing whitespace", t.Name) + } + return nil +} + // UserManifest represents the user's devrune.yaml file. // It declares packages, MCP servers, agents, and optional workflows to install. type UserManifest struct { @@ -291,6 +314,10 @@ type UserManifest struct { // references the primary DevRune package catalog (where DevRune // packages come from). Advisors holds advisor-only sources. Advisors []AdvisorSource `yaml:"advisors,omitempty"` + // Tools lists the tools declared in devrune.yaml. Each entry has a name + // and an optional upgrade command. Tools without an effective command are + // displayed as "(no upgradable)" in the Upgrade Tools TUI flow. + Tools []ToolRef `yaml:"tools,omitempty"` } // PackageRef is a reference to a package in the user manifest. @@ -367,5 +394,17 @@ func (m UserManifest) Validate() error { } } + // Validate Tools: no duplicates, each entry has valid Name. + seenTools := make(map[string]bool, len(m.Tools)) + for _, tool := range m.Tools { + if err := tool.Validate(); err != nil { + return err + } + if seenTools[tool.Name] { + return fmt.Errorf("manifest: duplicate tool %q", tool.Name) + } + seenTools[tool.Name] = true + } + return nil } diff --git a/internal/model/manifest_test.go b/internal/model/manifest_test.go index 181c30f..d390df4 100644 --- a/internal/model/manifest_test.go +++ b/internal/model/manifest_test.go @@ -177,6 +177,79 @@ func TestUserManifest_Validate(t *testing.T) { wantErr: true, errMsg: "source must not be empty", }, + // ── Tools validation ────────────────────────────────────────────────── + { + name: "valid manifest with tools (engram and crit, both with command)", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: "engram", Command: "brew install gentleman-programming/tap/engram"}, + {Name: "crit", Command: "brew install crit"}, + }, + }, + wantErr: false, + }, + { + name: "empty command is allowed (tool is no upgradable, not an error)", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: "custom-local", Command: ""}, + }, + }, + wantErr: false, + }, + { + name: "empty tool name fails", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: "", Command: "brew install something"}, + }, + }, + wantErr: true, + errMsg: "manifest: tool name must not be empty", + }, + { + name: "tool name with leading whitespace fails", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: " engram", Command: "brew install gentleman-programming/tap/engram"}, + }, + }, + wantErr: true, + errMsg: "whitespace", + }, + { + name: "tool name with trailing whitespace fails", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: "engram ", Command: "brew install gentleman-programming/tap/engram"}, + }, + }, + wantErr: true, + errMsg: "whitespace", + }, + { + name: "duplicate tool name fails", + manifest: UserManifest{ + SchemaVersion: "devrune/v1", + Agents: []AgentRef{{Name: "claude"}}, + Tools: []ToolRef{ + {Name: "engram", Command: "brew install gentleman-programming/tap/engram"}, + {Name: "engram", Command: "go install engram"}, + }, + }, + wantErr: true, + errMsg: `manifest: duplicate tool "engram"`, + }, } for _, tt := range tests { diff --git a/internal/model/opencode_models_test.go b/internal/model/opencode_models_test.go index 982fd66..080b4dc 100644 --- a/internal/model/opencode_models_test.go +++ b/internal/model/opencode_models_test.go @@ -61,9 +61,9 @@ func TestLoadOpenCodeModels_ValidSource(t *testing.T) { // Build source with 3 models, 2 tool_call=true, 1 false. srcData := buildOpenCodeSourceJSON(t, openCodeProvider, map[string]bool{ - "gpt-4o": true, + "gpt-4o": true, "gpt-4o-mini": true, - "ada": false, + "ada": false, }) writeFile(t, srcPath, srcData) diff --git a/internal/model/workflow.go b/internal/model/workflow.go index 173b1b1..c68867b 100644 --- a/internal/model/workflow.go +++ b/internal/model/workflow.go @@ -44,7 +44,7 @@ type WorkflowMetadata struct { Name string `yaml:"name"` // slug identifier, e.g. "sdd" DisplayName string `yaml:"displayName,omitempty"` // human-readable label for catalogs, e.g. "SDD (Spec-Driven Development)" Version string `yaml:"version"` // semver, e.g. "1.0.0" - WorkingDir string `yaml:"workingDir,omitempty"` // directory name for workflow files (orchestrator, _shared/); defaults to Name + WorkingDir string `yaml:"workingDir,omitempty"` // directory name for workflow files (orchestrator, _shared/); defaults to Name } // EffectiveDisplayName returns DisplayName if set, otherwise falls back to Name. diff --git a/internal/parse/frontmatter_test.go b/internal/parse/frontmatter_test.go index a217606..31e03cb 100644 --- a/internal/parse/frontmatter_test.go +++ b/internal/parse/frontmatter_test.go @@ -11,12 +11,12 @@ import ( func TestParseFrontmatter(t *testing.T) { tests := []struct { - name string - input string - wantFMKeys []string + name string + input string + wantFMKeys []string wantBodyContains string - wantErr bool - errContains string + wantErr bool + errContains string }{ { name: "standard skill file with frontmatter", @@ -31,7 +31,7 @@ allowed-tools: Some markdown body. `, - wantFMKeys: []string{"name", "description", "allowed-tools"}, + wantFMKeys: []string{"name", "description", "allowed-tools"}, wantBodyContains: "# Body content here", }, { @@ -40,7 +40,7 @@ Some markdown body. No frontmatter here. `, - wantFMKeys: []string{}, + wantFMKeys: []string{}, wantBodyContains: "# Just a markdown file", }, { @@ -58,7 +58,7 @@ argument-hint: "[topic] [extra]" --- # Skill body `, - wantFMKeys: []string{"name", "description", "argument-hint"}, + wantFMKeys: []string{"name", "description", "argument-hint"}, wantBodyContains: "# Skill body", }, { @@ -78,7 +78,7 @@ name: trick --- Body here `, - wantFMKeys: []string{}, + wantFMKeys: []string{}, wantBodyContains: "Some text before", }, { @@ -92,7 +92,7 @@ name: my-skill More content. `, - wantFMKeys: []string{"name"}, + wantFMKeys: []string{"name"}, wantBodyContains: "--- separator in body", }, } diff --git a/internal/parse/lockfile_test.go b/internal/parse/lockfile_test.go index 52c7992..195707d 100644 --- a/internal/parse/lockfile_test.go +++ b/internal/parse/lockfile_test.go @@ -201,8 +201,8 @@ func TestSerializeLockfile_IsDeterministic(t *testing.T) { }, }, { - Source: model.SourceRef{Scheme: model.SchemeGitHub, Owner: "a-owner", Repo: "a-repo", Ref: "v1.0.0"}, - Hash: "sha256:aaa", + Source: model.SourceRef{Scheme: model.SchemeGitHub, Owner: "a-owner", Repo: "a-repo", Ref: "v1.0.0"}, + Hash: "sha256:aaa", Contents: []model.ContentItem{}, }, }, diff --git a/internal/parse/manifest_test.go b/internal/parse/manifest_test.go index 3b5eb26..5ac4379 100644 --- a/internal/parse/manifest_test.go +++ b/internal/parse/manifest_test.go @@ -422,6 +422,123 @@ advisors: }) } +// TestParseManifest_Tools verifica que un YAML con sección tools: se parsea +// correctamente y que los campos Name/Command se leen con exactitud. +func TestParseManifest_Tools(t *testing.T) { + rawYAML := []byte(`schemaVersion: devrune/v1 +agents: + - name: claude +packages: + - source: github:owner/repo@v1.0.0 +tools: + - name: engram + command: "brew install gentleman-programming/tap/engram" + - name: crit + command: "brew install crit" +`) + m, err := parse.ParseManifest(rawYAML) + if err != nil { + t.Fatalf("ParseManifest: unexpected error: %v", err) + } + + if len(m.Tools) != 2 { + t.Fatalf("Tools length = %d, want 2", len(m.Tools)) + } + + wantTools := []struct { + Name string + Command string + }{ + {"engram", "brew install gentleman-programming/tap/engram"}, + {"crit", "brew install crit"}, + } + + for i, want := range wantTools { + got := m.Tools[i] + if got.Name != want.Name { + t.Errorf("Tools[%d].Name = %q, want %q", i, got.Name, want.Name) + } + if got.Command != want.Command { + t.Errorf("Tools[%d].Command = %q, want %q", i, got.Command, want.Command) + } + } +} + +// TestSerializeManifest_Tools_RoundTrip verifica el comportamiento de +// serialización de la sección tools: en dos escenarios: +// - Un manifest con tools: sobrevive parse → marshal → parse con entradas intactas. +// - Un manifest sin tools: no serializa la clave tools: (gracias a omitempty). +func TestSerializeManifest_Tools_RoundTrip(t *testing.T) { + t.Run("manifest con tools — round-trip conserva entries intactas", func(t *testing.T) { + original, err := parse.ParseManifest([]byte(`schemaVersion: devrune/v1 +agents: + - name: claude +packages: + - source: github:owner/repo@v1.0.0 +tools: + - name: engram + command: "brew install gentleman-programming/tap/engram" + - name: crit + command: "brew install crit" +`)) + if err != nil { + t.Fatalf("ParseManifest: %v", err) + } + + serialized, err := parse.SerializeManifest(original) + if err != nil { + t.Fatalf("SerializeManifest: %v", err) + } + + reparsed, err := parse.ParseManifest(serialized) + if err != nil { + t.Fatalf("ParseManifest (reparsed): %v", err) + } + + if len(reparsed.Tools) != len(original.Tools) { + t.Fatalf("Tools length after round-trip = %d, want %d", len(reparsed.Tools), len(original.Tools)) + } + for i, want := range original.Tools { + got := reparsed.Tools[i] + if got.Name != want.Name { + t.Errorf("Tools[%d].Name = %q, want %q", i, got.Name, want.Name) + } + if got.Command != want.Command { + t.Errorf("Tools[%d].Command = %q, want %q", i, got.Command, want.Command) + } + } + }) + + t.Run("manifest sin tools — clave tools ausente tras serialización (omitempty)", func(t *testing.T) { + original, err := parse.ParseManifest([]byte(`schemaVersion: devrune/v1 +agents: + - name: claude +packages: + - source: github:owner/repo@v1.0.0 +`)) + if err != nil { + t.Fatalf("ParseManifest: %v", err) + } + + serialized, err := parse.SerializeManifest(original) + if err != nil { + t.Fatalf("SerializeManifest: %v", err) + } + + // omitempty debe omitir la clave cuando el slice es nil/vacío. + if strings.Contains(string(serialized), "tools:") { + t.Errorf("serialized output no debe contener 'tools:' cuando el slice está vacío (omitempty), got:\n%s", serialized) + } + + reparsed, err := parse.ParseManifest(serialized) + if err != nil { + t.Fatalf("ParseManifest (reparsed): %v", err) + } + if len(reparsed.Tools) != 0 { + t.Errorf("Tools after round-trip = %d, want 0", len(reparsed.Tools)) + } + }) +} // mustReadFixture reads a test fixture file from testdata//. // It fails the test immediately if the file cannot be read. diff --git a/internal/recommend/advisor_filter_test.go b/internal/recommend/advisor_filter_test.go index 17a2fba..6d9d43d 100644 --- a/internal/recommend/advisor_filter_test.go +++ b/internal/recommend/advisor_filter_test.go @@ -205,12 +205,12 @@ func TestDetectProjectScope(t *testing.T) { want: []string{model.AdvisorScopeFrontend}, }, { - name: "nil profile — unknown project, scope is nil (not empty slice)", + name: "nil profile — unknown project, scope is nil (not empty slice)", profile: nil, want: nil, }, { - name: "empty profile no frameworks no languages — scope is nil", + name: "empty profile no frameworks no languages — scope is nil", profile: &detect.ProjectProfile{}, want: nil, }, @@ -300,11 +300,11 @@ func TestFilterAdvisersByProfile_Matrix(t *testing.T) { } tests := []struct { - name string - advisorScope []string // scope of the single advisor under test - profile *detect.ProjectProfile - wantIncluded bool - note string + name string + advisorScope []string // scope of the single advisor under test + profile *detect.ProjectProfile + wantIncluded bool + note string }{ // Nil project scope (unknown) — return input unchanged regardless of advisor scope. { diff --git a/internal/recommend/cache.go b/internal/recommend/cache.go index 6c8a537..3594e42 100644 --- a/internal/recommend/cache.go +++ b/internal/recommend/cache.go @@ -37,7 +37,7 @@ func cacheDir() string { // cacheEntry is the on-disk format for a cached recommendation result. type cacheEntry struct { - Timestamp time.Time `json:"timestamp"` + Timestamp time.Time `json:"timestamp"` Recommendations []Recommendation `json:"recommendations"` } diff --git a/internal/recommend/prompt.go b/internal/recommend/prompt.go index 308c405..5d06f29 100644 --- a/internal/recommend/prompt.go +++ b/internal/recommend/prompt.go @@ -24,12 +24,12 @@ type promptPayload struct { // profileSummary is a JSON-friendly summary of the detected project profile. type profileSummary struct { - Languages []string `json:"languages"` - Frameworks []string `json:"frameworks"` - Dependencies []depSummary `json:"dependencies"` - ConfigFiles []string `json:"config_files"` - TotalFiles int `json:"total_files"` - TotalLines int `json:"total_lines"` + Languages []string `json:"languages"` + Frameworks []string `json:"frameworks"` + Dependencies []depSummary `json:"dependencies"` + ConfigFiles []string `json:"config_files"` + TotalFiles int `json:"total_files"` + TotalLines int `json:"total_lines"` } type depSummary struct { diff --git a/internal/resolve/resolver.go b/internal/resolve/resolver.go index d9f033b..1dc99ab 100644 --- a/internal/resolve/resolver.go +++ b/internal/resolve/resolver.go @@ -102,15 +102,15 @@ func (r *Resolver) SetPriorLockfile(lf model.Lockfile) { // 2. No prior entry for this CacheKey → always re-fetch (first resolve). // 3. Fetcher implements RevisionResolver: // a. Prior revision recorded → cheap GET, compare SHAs. Match reuses -// the cache; mismatch/error/missing-cache-dir all fall through to -// re-fetch. +// the cache; mismatch/error/missing-cache-dir all fall through to +// re-fetch. // b. No prior revision (old lockfile schema) → re-fetch so the next -// lockfile captures the SHA. +// lockfile captures the SHA. // 4. Fetcher does NOT implement RevisionResolver (legacy backend; today // only the local scheme but kept defensively for future backends): // a. Mutable ref (HEAD / empty) → re-fetch. This preserves the safety -// guarantee of the HEAD-bypass fix (6bed877) — without an SHA -// check, trusting the cache would silently hide upstream moves. +// guarantee of the HEAD-bypass fix (6bed877) — without an SHA +// check, trusting the cache would silently hide upstream moves. // b. Immutable-looking ref (everything else) → reuse cache by hash. func (r *Resolver) cachedDir(ctx context.Context, sourceRef model.SourceRef) (dir, hash, revision string, ok bool) { if r.priorIndex == nil { diff --git a/internal/resolve/workflow_expander_test.go b/internal/resolve/workflow_expander_test.go index 72480a0..68365a6 100644 --- a/internal/resolve/workflow_expander_test.go +++ b/internal/resolve/workflow_expander_test.go @@ -85,7 +85,6 @@ func TestExpandWorkflows_NoWorkflows(t *testing.T) { } } - // TestExpandWorkflows_MultipleWorkflows verifies that multiple valid workflow sources all pass. func TestExpandWorkflows_MultipleWorkflows(t *testing.T) { manifest := model.UserManifest{ diff --git a/internal/tui/app.go b/internal/tui/app.go index 652be41..823305c 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -36,11 +36,14 @@ type RunResult struct { // WorkflowModels is the merged per-agent role model map extracted from all workflow entries. // Recommendations holds AI recommendations from a previous devrune.recommended.yaml run, // used to pre-select items with AI badges in the selection step. +// Tools holds tool overrides from the existing manifest; used to preserve user-defined +// commands when the wizard rebuilds manifest.Tools after ConfirmSummary. type ExistingConfig struct { Agents []string Sources []string WorkflowModels map[string]map[string]string Recommendations []model.RecommendedItem // AI recommendations for pre-selection + Tools []model.ToolRef // persisted tool overrides from existing manifest } // Run executes the interactive TUI wizard and returns the resulting @@ -72,9 +75,11 @@ func Run(projectDir string, catalogSources []string, existing *ExistingConfig) ( // Determine preselected agents and sources from existing config. var preselectedAgents []string var preselectedSources []string + var existingTools []model.ToolRef if existing != nil { preselectedAgents = existing.Agents preselectedSources = existing.Sources + existingTools = existing.Tools } // Step 1 — agents (alt screen, step indicator inside form) @@ -392,6 +397,9 @@ func Run(projectDir string, catalogSources []string, existing *ExistingConfig) ( return RunResult{}, mapErr(err) } + // Persist tools: merge active tools from catalog with existing user overrides. + manifest.Tools = buildToolRefsForManifest(activeTools, existingTools) + return RunResult{Manifest: manifest, InstalledTools: installedTools}, nil } @@ -567,3 +575,44 @@ func filterToolsBySelection(tools []model.ToolDef, selection steps.SelectionResu } return result } + +// buildToolRefsForManifest constructs the []model.ToolRef slice to persist in +// manifest.Tools after the wizard's ConfirmSummary step. +// +// Rules (applied per activeTools entry, in order): +// 1. If existing contains a ToolRef with the same Name and a non-empty Command, +// that override wins — the user's custom command is preserved. +// 2. Otherwise, the ToolDef.Command from the catalog scan is used (may be empty). +// +// The result is deduplicated by Name and follows the order of activeTools. +// Tools that were not selected (not in activeTools) are dropped regardless of +// whether they appear in existing. +func buildToolRefsForManifest(activeTools []model.ToolDef, existing []model.ToolRef) []model.ToolRef { + // Build a lookup map for existing overrides by name. + overrides := make(map[string]string, len(existing)) + for _, ref := range existing { + if strings.TrimSpace(ref.Command) != "" { + overrides[ref.Name] = ref.Command + } + } + + // Iterate activeTools in order, deduplicating by name. + seen := make(map[string]bool, len(activeTools)) + refs := make([]model.ToolRef, 0, len(activeTools)) + for _, td := range activeTools { + if seen[td.Name] { + continue + } + seen[td.Name] = true + + cmd := td.Command + if override, ok := overrides[td.Name]; ok { + cmd = override + } + refs = append(refs, model.ToolRef{ + Name: td.Name, + Command: cmd, + }) + } + return refs +} diff --git a/internal/tui/app_test.go b/internal/tui/app_test.go index c42288b..1317675 100644 --- a/internal/tui/app_test.go +++ b/internal/tui/app_test.go @@ -219,6 +219,91 @@ func selectionWith(selectedTools, selectedMCPs, selectedWorkflows []string) step } } +// TestBuildToolRefsForManifest covers all merge/dedup rules for buildToolRefsForManifest. +func TestBuildToolRefsForManifest(t *testing.T) { + tests := []struct { + name string + activeTools []model.ToolDef + existing []model.ToolRef + want []model.ToolRef + }{ + { + name: "uses_catalog_command_when_no_existing_override", + activeTools: []model.ToolDef{ + {Name: "crit", Command: "brew install crit"}, + }, + existing: nil, + want: []model.ToolRef{ + {Name: "crit", Command: "brew install crit"}, + }, + }, + { + name: "preserves_existing_override_over_catalog", + activeTools: []model.ToolDef{ + {Name: "crit", Command: "brew install crit"}, + }, + existing: []model.ToolRef{ + {Name: "crit", Command: "custom-install-crit"}, + }, + want: []model.ToolRef{ + {Name: "crit", Command: "custom-install-crit"}, + }, + }, + { + name: "deduplicates_by_name_keeps_first", + activeTools: []model.ToolDef{ + {Name: "engram", Command: "brew install engram"}, + {Name: "engram", Command: "brew install engram-duplicate"}, + }, + existing: nil, + want: []model.ToolRef{ + {Name: "engram", Command: "brew install engram"}, + }, + }, + { + name: "does_not_include_tools_not_in_activeTools", + activeTools: []model.ToolDef{ + {Name: "crit", Command: "brew install crit"}, + }, + existing: []model.ToolRef{ + {Name: "crit", Command: "custom-crit"}, + {Name: "engram", Command: "custom-engram"}, // not in activeTools + }, + want: []model.ToolRef{ + {Name: "crit", Command: "custom-crit"}, + }, + }, + { + name: "preserves_empty_command_when_catalog_and_existing_both_empty", + activeTools: []model.ToolDef{ + {Name: "unknown-tool", Command: ""}, + }, + existing: nil, + want: []model.ToolRef{ + {Name: "unknown-tool", Command: ""}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := buildToolRefsForManifest(tc.activeTools, tc.existing) + + if len(got) != len(tc.want) { + t.Fatalf("len(got)=%d, len(want)=%d; got=%+v", len(got), len(tc.want), got) + } + for i, want := range tc.want { + if got[i].Name != want.Name { + t.Errorf("[%d] Name: got %q, want %q", i, got[i].Name, want.Name) + } + if got[i].Command != want.Command { + t.Errorf("[%d] Command: got %q, want %q", i, got[i].Command, want.Command) + } + } + }) + } +} + // TestFilterToolsBySelection covers the filtering logic with table-driven tests. func TestFilterToolsBySelection(t *testing.T) { // Fixture tools used across test cases. diff --git a/internal/tui/scanner.go b/internal/tui/scanner.go index 9ef5621..47e3ee5 100644 --- a/internal/tui/scanner.go +++ b/internal/tui/scanner.go @@ -19,16 +19,16 @@ import ( // ScannedRepo holds the scan results for one repository source ref. type ScannedRepo struct { - Source string // original source ref string - Skills []string // discovered skill names - Rules []string // discovered rule names - MCPs []string // discovered MCP names (files in mcps/ dir) + Source string // original source ref string + Skills []string // discovered skill names + Rules []string // discovered rule names + MCPs []string // discovered MCP names (files in mcps/ dir) Workflows []string // discovered workflow names (dirs with workflow.yaml) WorkflowManifests []model.WorkflowManifest // parsed workflow manifests Tools []model.ToolDef // discovered tool definitions (files in tools/ dir) - Descs map[string]string // item name → description (for skills, workflows, MCPs) - MCPFiles map[string]string // MCP name → filename with extension (e.g. "engram" → "engram.yaml") - Error error // scan error (nil if ok) + Descs map[string]string // item name → description (for skills, workflows, MCPs) + MCPFiles map[string]string // MCP name → filename with extension (e.g. "engram" → "engram.yaml") + Error error // scan error (nil if ok) } // CanonicalSDDSource is the source ref string for the catalog that ships SDD. diff --git a/internal/tui/steps/confirm.go b/internal/tui/steps/confirm.go index 7ee5967..a22fcd5 100644 --- a/internal/tui/steps/confirm.go +++ b/internal/tui/steps/confirm.go @@ -208,7 +208,6 @@ func buildManifestFromSelection(agents []string, selection SelectionResult, work } } - // appendSubpath appends a subpath to a source ref string. // For remote sources (github/gitlab), it uses the "//" separator convention. // For local sources, it appends as a filesystem path. diff --git a/internal/tui/steps/install_spinner_test.go b/internal/tui/steps/install_spinner_test.go index 88e6be4..f6548d9 100644 --- a/internal/tui/steps/install_spinner_test.go +++ b/internal/tui/steps/install_spinner_test.go @@ -45,7 +45,6 @@ func TestInstallModel_ErrorStateRendersMessage(t *testing.T) { } } - // TestInstallModel_ErrorStateNotQuitUntilKeypress verifies that the model // does NOT quit immediately on receiving an error — it stays alive to show // the error, and only quits on a subsequent keypress. diff --git a/internal/tui/steps/recommend.go b/internal/tui/steps/recommend.go index 053faff..472a849 100644 --- a/internal/tui/steps/recommend.go +++ b/internal/tui/steps/recommend.go @@ -449,8 +449,8 @@ func (m recommendFlowModel) View() tea.View { // RecommendFlowResult holds the output of RunRecommendFlow. type RecommendFlowResult struct { Result *recommend.RecommendResult - Accepted bool // user chose "Yes, apply" - Skipped bool // user chose "No, go back" or error + Accepted bool // user chose "Yes, apply" + Skipped bool // user chose "No, go back" or error Err error } diff --git a/internal/tui/steps/sdd_info_test.go b/internal/tui/steps/sdd_info_test.go index f89ca41..de53b25 100644 --- a/internal/tui/steps/sdd_info_test.go +++ b/internal/tui/steps/sdd_info_test.go @@ -42,9 +42,9 @@ func TestSDDInfoContent(t *testing.T) { wantSubstring: "4 phases", }, { - name: "narrow terminal (w=60) short content does not contain numbered phase list", - width: 60, - wantAbsent: "① Explore", + name: "narrow terminal (w=60) short content does not contain numbered phase list", + width: 60, + wantAbsent: "① Explore", }, { name: "at threshold (w=70) returns full content (strictly less-than check)", diff --git a/internal/tui/steps/tool_upgrade.go b/internal/tui/steps/tool_upgrade.go new file mode 100644 index 0000000..37f71e4 --- /dev/null +++ b/internal/tui/steps/tool_upgrade.go @@ -0,0 +1,476 @@ +// SPDX-License-Identifier: MIT + +package steps + +import ( + "fmt" + "os/exec" + "strings" + "sync" + + "charm.land/bubbles/v2/spinner" + tea "charm.land/bubbletea/v2" + "charm.land/huh/v2" + "charm.land/lipgloss/v2" + + "github.com/davidarce/devrune/internal/model" + "github.com/davidarce/devrune/internal/tui/tuistyles" +) + +// --------------------------------------------------------------------------- +// T012 — Types and parallel upgrade engine +// --------------------------------------------------------------------------- + +// ToolUpgradeStatus represents the outcome of upgrading a single tool. +type ToolUpgradeStatus string + +const ( + // ToolUpgradeOK indica que el comando de upgrade completó sin errores. + ToolUpgradeOK ToolUpgradeStatus = "ok" + // ToolUpgradeFail indica que el comando de upgrade retornó un error. + ToolUpgradeFail ToolUpgradeStatus = "fail" +) + +// ToolUpgradeResult holds the outcome for a single tool upgrade. +type ToolUpgradeResult struct { + Name string + Status ToolUpgradeStatus + Error string +} + +// ToolUpgradeSummary aggregates results for all executed tool upgrades. +// Results are in stable input order (non-upgradable tools are excluded). +type ToolUpgradeSummary struct { + Results []ToolUpgradeResult +} + +// ToolCommandExecutor is a function that runs a shell command string and +// returns nil on success or an error on failure. Inject a test double to +// avoid executing real commands in unit tests. +type ToolCommandExecutor func(command string) error + +// upgradeToolItem is the internal representation of a single tool during +// the upgrade flow. Upgradable is false when no effective command exists. +type upgradeToolItem struct { + Name string + Command string + Upgradable bool +} + +// defaultToolCommandExecutor runs the given command string via `sh -c`. +func defaultToolCommandExecutor(command string) error { + cmd := exec.Command("sh", "-c", command) //nolint:gosec + return cmd.Run() +} + +// upgradeToolsParallel runs each upgradable item concurrently and returns a +// ToolUpgradeSummary whose Results slice is ordered to match the input slice +// (non-upgradable items are omitted from Results entirely). +// +// If exec is nil, defaultToolCommandExecutor is used. +func upgradeToolsParallel(items []upgradeToolItem, execFn ToolCommandExecutor) ToolUpgradeSummary { + if execFn == nil { + execFn = defaultToolCommandExecutor + } + + // Collect only upgradable items, preserving relative order. + type indexedItem struct { + idx int + item upgradeToolItem + } + var upgradable []indexedItem + for i, it := range items { + if it.Upgradable { + upgradable = append(upgradable, indexedItem{idx: i, item: it}) + } + } + + results := make([]ToolUpgradeResult, len(upgradable)) + var wg sync.WaitGroup + ch := make(chan struct { + pos int + res ToolUpgradeResult + }, len(upgradable)) + + for pos, ii := range upgradable { + wg.Add(1) + go func(pos int, it upgradeToolItem) { + defer wg.Done() + err := execFn(it.Command) + res := ToolUpgradeResult{Name: it.Name} + if err != nil { + res.Status = ToolUpgradeFail + res.Error = err.Error() + } else { + res.Status = ToolUpgradeOK + } + ch <- struct { + pos int + res ToolUpgradeResult + }{pos: pos, res: res} + }(pos, ii.item) + } + + go func() { + wg.Wait() + close(ch) + }() + + for r := range ch { + results[r.pos] = r.res + } + + return ToolUpgradeSummary{Results: results} +} + +// --------------------------------------------------------------------------- +// T013 — buildUpgradeToolItems helper (pure, exported for T016 tests) +// --------------------------------------------------------------------------- + +// buildUpgradeToolItems resolves the effective upgrade command for each tool +// reference and classifies it as upgradable or not. +// +// Resolution order: +// 1. strings.TrimSpace(ref.Command) non-empty → use it. +// 2. strings.TrimSpace(catalog[ref.Name].Command) non-empty → use it. +// 3. otherwise → Upgradable = false. +func buildUpgradeToolItems(tools []model.ToolRef, catalog map[string]model.ToolDef) []upgradeToolItem { + items := make([]upgradeToolItem, len(tools)) + for i, ref := range tools { + cmd := strings.TrimSpace(ref.Command) + if cmd == "" { + if def, ok := catalog[ref.Name]; ok { + cmd = strings.TrimSpace(def.Command) + } + } + items[i] = upgradeToolItem{ + Name: ref.Name, + Command: cmd, + Upgradable: cmd != "", + } + } + return items +} + +// --------------------------------------------------------------------------- +// T013 — RunToolUpgradeStep (public entry point) +// --------------------------------------------------------------------------- + +// RunToolUpgradeStep presents the upgrade-tools TUI flow: +// 1. Empty state when len(tools)==0. +// 2. Preview list (upgradable vs. "no upgradable"). +// 3. Explicit yes/no confirmation. +// 4. Parallel upgrade with spinner (T014). +// 5. Result summary. +// +// Returns an empty summary and nil error when the user cancels or there is +// nothing to run. +func RunToolUpgradeStep(tools []model.ToolRef, catalog map[string]model.ToolDef) (ToolUpgradeSummary, error) { + return runToolUpgradeStepWithExecutor(tools, catalog, nil) +} + +// runToolUpgradeStepWithExecutor is the testable variant that accepts an +// injected executor. +func runToolUpgradeStepWithExecutor( + tools []model.ToolRef, + catalog map[string]model.ToolDef, + execFn ToolCommandExecutor, +) (ToolUpgradeSummary, error) { + + // ── Empty state ────────────────────────────────────────────────────────── + if len(tools) == 0 { + return showUpgradeEmptyState() + } + + // ── Build items ────────────────────────────────────────────────────────── + items := buildUpgradeToolItems(tools, catalog) + + // ── Preview (huh Note) ─────────────────────────────────────────────────── + var previewLines strings.Builder + upgradableCount := 0 + for _, it := range items { + if it.Upgradable { + fmt.Fprintf(&previewLines, " ✓ %s\n", it.Name) + upgradableCount++ + } else { + fmt.Fprintf(&previewLines, " - %s (no upgradable)\n", it.Name) + } + } + + confirmed, err := showUpgradePreviewAndConfirm(previewLines.String(), items, upgradableCount) + if err != nil { + return ToolUpgradeSummary{}, err + } + if !confirmed { + return ToolUpgradeSummary{}, nil + } + + // ── All non-upgradable ─────────────────────────────────────────────────── + if upgradableCount == 0 { + return showAllNonUpgradable() + } + + // ── Spinner + execute ──────────────────────────────────────────────────── + summary, err := runUpgradeSpinner(items, execFn) + if err != nil { + return ToolUpgradeSummary{}, err + } + + // ── Summary screen ─────────────────────────────────────────────────────── + if err := showUpgradeSummary(summary); err != nil { + return ToolUpgradeSummary{}, err + } + + return summary, nil +} + +// showUpgradeEmptyState renders the "no tools" note and waits for user. +func showUpgradeEmptyState() (ToolUpgradeSummary, error) { + body := "Añade tools desde Setup o edita\ndevrune.yaml con una sección tools:." + + form := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title("Upgrade Tools"). + Description("No hay tools para actualizar.\n\n" + body), + ), + ).WithTheme(tuistyles.DevRuneThemeFunc). + WithViewHook(func(v tea.View) tea.View { + v.AltScreen = true + return v + }) + + if err := form.Run(); err != nil { + return ToolUpgradeSummary{}, err + } + return ToolUpgradeSummary{}, nil +} + +// showAllNonUpgradable renders a clear message when every tool is non-upgradable. +func showAllNonUpgradable() (ToolUpgradeSummary, error) { + form := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title("Upgrade Tools"). + Description("Ninguna tool tiene un comando de upgrade efectivo.\n\nRevisa que tus tools tengan un campo 'command' en devrune.yaml o en el catálogo embebido."), + ), + ).WithTheme(tuistyles.DevRuneThemeFunc). + WithViewHook(func(v tea.View) tea.View { + v.AltScreen = true + return v + }) + + if err := form.Run(); err != nil { + return ToolUpgradeSummary{}, err + } + return ToolUpgradeSummary{}, nil +} + +// showUpgradePreviewAndConfirm renders the preview note then an explicit yes/no +// confirmation. Returns true only when the user selects "Yes". +func showUpgradePreviewAndConfirm(previewLines string, items []upgradeToolItem, upgradableCount int) (bool, error) { + // Preview step. + previewForm := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title("Upgrade Tools"). + Description("Estas tools están declaradas en devrune.yaml.\nSolo se ejecutarán las que tengan command efectivo.\n\n" + previewLines), + ), + ).WithTheme(tuistyles.DevRuneThemeFunc). + WithViewHook(func(v tea.View) tea.View { + v.AltScreen = true + return v + }) + + if err := previewForm.Run(); err != nil { + return false, err + } + + if upgradableCount == 0 { + // No need for the confirm step; caller handles the "all non-upgradable" case. + return true, nil + } + + // Build confirm description with commands. + var confirmLines strings.Builder + fmt.Fprintf(&confirmLines, "DevRune ejecutará comandos de upgrade en\ntu sistema para %d tool(s) upgradable(s).\n\n", upgradableCount) + for _, it := range items { + if it.Upgradable { + fmt.Fprintf(&confirmLines, " %s → %s\n", it.Name, it.Command) + } + } + confirmLines.WriteString("\n¿Ejecutar upgrades ahora?") + + var choice string + confirmForm := huh.NewForm( + huh.NewGroup( + huh.NewSelect[string](). + Title("Confirm Upgrade Tools"). + Description(confirmLines.String()). + Options( + huh.NewOption("Yes, upgrade tools", "yes"), + huh.NewOption("No, back to menu", "no"), + ). + Value(&choice), + ), + ).WithTheme(tuistyles.DevRuneThemeFunc). + WithViewHook(func(v tea.View) tea.View { + v.AltScreen = true + return v + }) + + if err := confirmForm.Run(); err != nil { + return false, err + } + + return choice == "yes", nil +} + +// --------------------------------------------------------------------------- +// T014 — Spinner model and summary rendering +// --------------------------------------------------------------------------- + +// toolUpgradeDoneMsg is sent when all parallel upgrades have completed. +type toolUpgradeDoneMsg struct { + summary ToolUpgradeSummary +} + +// toolUpgradeModel is a Bubbletea model that shows a spinner while tools +// are upgraded in parallel in the background. +type toolUpgradeModel struct { + spinner spinner.Model + items []upgradeToolItem // all items (for display) + summary ToolUpgradeSummary + done bool + execFn ToolCommandExecutor +} + +func newToolUpgradeModel(items []upgradeToolItem, execFn ToolCommandExecutor) toolUpgradeModel { + s := spinner.New() + s.Spinner = spinner.Dot + s.Style = lipgloss.NewStyle().Foreground(tuistyles.ColorSecondary) + return toolUpgradeModel{ + spinner: s, + items: items, + execFn: execFn, + } +} + +func (m toolUpgradeModel) Init() tea.Cmd { + return tea.Batch( + m.spinner.Tick, + m.doUpgrade(), + ) +} + +func (m toolUpgradeModel) doUpgrade() tea.Cmd { + items := m.items + fn := m.execFn + return func() tea.Msg { + summary := upgradeToolsParallel(items, fn) + return toolUpgradeDoneMsg{summary: summary} + } +} + +func (m toolUpgradeModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case spinner.TickMsg: + var cmd tea.Cmd + m.spinner, cmd = m.spinner.Update(msg) + return m, cmd + case toolUpgradeDoneMsg: + m.summary = msg.summary + m.done = true + return m, tea.Quit + case tea.KeyPressMsg: + if msg.String() == "ctrl+c" { + m.done = true + return m, tea.Quit + } + } + return m, nil +} + +func (m toolUpgradeModel) View() tea.View { + if m.done { + v := tea.NewView("") + v.AltScreen = true + return v + } + + var sb strings.Builder + sb.WriteString("\n") + sb.WriteString(" ") + sb.WriteString(tuistyles.StyleTitle.Render("Upgrade Tools")) + sb.WriteString("\n\n") + sb.WriteString(" ") + sb.WriteString(m.spinner.View()) + sb.WriteString(" ") + sb.WriteString(tuistyles.StyleInfo.Render("Upgrading selected tools...")) + sb.WriteString("\n\n") + + for _, it := range m.items { + if it.Upgradable { + fmt.Fprintf(&sb, " %s running\n", it.Name) + } + } + + sb.WriteString("\n") + sb.WriteString(tuistyles.StyleSubtitle.Render(" Los upgrades corren en paralelo; un fallo no cancela las demás tools.")) + sb.WriteString("\n") + + v := tea.NewView(sb.String()) + v.AltScreen = true + return v +} + +// runUpgradeSpinner launches the bubbletea spinner for parallel upgrades and +// returns the aggregated ToolUpgradeSummary. +func runUpgradeSpinner(items []upgradeToolItem, execFn ToolCommandExecutor) (ToolUpgradeSummary, error) { + m := newToolUpgradeModel(items, execFn) + p := tea.NewProgram(m) + + finalModel, err := p.Run() + if err != nil { + return ToolUpgradeSummary{}, fmt.Errorf("tool upgrade model: %w", err) + } + + result, ok := finalModel.(toolUpgradeModel) + if !ok { + return ToolUpgradeSummary{}, fmt.Errorf("tool upgrade model: unexpected model type") + } + + return result.summary, nil +} + +// showUpgradeSummary renders the final per-tool ok/fail summary using a huh +// Note and waits for the user to press Continue / Back to menu. +func showUpgradeSummary(summary ToolUpgradeSummary) error { + var sb strings.Builder + sb.WriteString("Results\n\n") + + for _, r := range summary.Results { + switch r.Status { + case ToolUpgradeOK: + okMark := tuistyles.StyleSuccess.Render("✓") + fmt.Fprintf(&sb, " %s %s ok\n", okMark, r.Name) + case ToolUpgradeFail: + failMark := tuistyles.StyleError.Render("✗") + fmt.Fprintf(&sb, " %s %s fail: %s\n", failMark, r.Name, r.Error) + } + } + + form := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title("Upgrade Tools Complete"). + Description(sb.String()), + ), + ).WithTheme(tuistyles.DevRuneThemeFunc). + WithViewHook(func(v tea.View) tea.View { + v.AltScreen = true + return v + }) + + return form.Run() +} diff --git a/internal/tui/steps/tool_upgrade_test.go b/internal/tui/steps/tool_upgrade_test.go new file mode 100644 index 0000000..904aeb6 --- /dev/null +++ b/internal/tui/steps/tool_upgrade_test.go @@ -0,0 +1,311 @@ +// SPDX-License-Identifier: MIT + +package steps + +import ( + "errors" + "sync" + "testing" + + "github.com/davidarce/devrune/internal/model" +) + +// --------------------------------------------------------------------------- +// T015 — Parallel upgrade engine tests +// --------------------------------------------------------------------------- + +// mockExecutor returns a ToolCommandExecutor that records every command it +// receives (protected by mu) and returns the provided error for matching cmds. +func mockExecutor(mu *sync.Mutex, recorded *[]string, failCmd string, failErr error) ToolCommandExecutor { + return func(command string) error { + mu.Lock() + *recorded = append(*recorded, command) + mu.Unlock() + if failCmd != "" && command == failCmd { + return failErr + } + return nil + } +} + +// TestUpgradeToolsParallel_AllOK verifica que 3 tools upgradables con executor +// sin errores producen Results con status=ok en el orden del input. +func TestUpgradeToolsParallel_AllOK(t *testing.T) { + items := []upgradeToolItem{ + {Name: "engram", Command: "brew install engram", Upgradable: true}, + {Name: "crit", Command: "brew install crit", Upgradable: true}, + {Name: "other", Command: "brew install other", Upgradable: true}, + } + + var mu sync.Mutex + var recorded []string + exec := mockExecutor(&mu, &recorded, "", nil) + + summary := upgradeToolsParallel(items, exec) + + if len(summary.Results) != 3 { + t.Fatalf("expected 3 results, got %d", len(summary.Results)) + } + + for i, res := range summary.Results { + if res.Name != items[i].Name { + t.Errorf("result[%d]: expected name %q, got %q", i, items[i].Name, res.Name) + } + if res.Status != ToolUpgradeOK { + t.Errorf("result[%d] %q: expected status ok, got %q", i, res.Name, res.Status) + } + if res.Error != "" { + t.Errorf("result[%d] %q: expected no error, got %q", i, res.Name, res.Error) + } + } +} + +// TestUpgradeToolsParallel_Mixed verifica que cuando la tool del medio falla, +// solo esa tiene status=fail y las demás status=ok; el orden de input se mantiene. +func TestUpgradeToolsParallel_Mixed(t *testing.T) { + failCmd := "brew install crit" + failErr := errors.New("exit status 1") + + items := []upgradeToolItem{ + {Name: "engram", Command: "brew install engram", Upgradable: true}, + {Name: "crit", Command: failCmd, Upgradable: true}, + {Name: "other", Command: "brew install other", Upgradable: true}, + } + + var mu sync.Mutex + var recorded []string + exec := mockExecutor(&mu, &recorded, failCmd, failErr) + + summary := upgradeToolsParallel(items, exec) + + if len(summary.Results) != 3 { + t.Fatalf("expected 3 results, got %d", len(summary.Results)) + } + + // engram → ok + if summary.Results[0].Status != ToolUpgradeOK { + t.Errorf("engram: expected ok, got %q", summary.Results[0].Status) + } + + // crit → fail + if summary.Results[1].Status != ToolUpgradeFail { + t.Errorf("crit: expected fail, got %q", summary.Results[1].Status) + } + if summary.Results[1].Error != failErr.Error() { + t.Errorf("crit: expected error %q, got %q", failErr.Error(), summary.Results[1].Error) + } + + // other → ok + if summary.Results[2].Status != ToolUpgradeOK { + t.Errorf("other: expected ok, got %q", summary.Results[2].Status) + } + + // orden estable: names deben coincidir con input + names := []string{"engram", "crit", "other"} + for i, r := range summary.Results { + if r.Name != names[i] { + t.Errorf("result[%d]: expected name %q, got %q", i, names[i], r.Name) + } + } +} + +// TestUpgradeToolsParallel_NonUpgradableSkipped verifica que una tool con +// Upgradable=false no se incluye en Results y el executor nunca la recibe. +func TestUpgradeToolsParallel_NonUpgradableSkipped(t *testing.T) { + items := []upgradeToolItem{ + {Name: "engram", Command: "brew install engram", Upgradable: true}, + {Name: "custom-local", Command: "", Upgradable: false}, + {Name: "crit", Command: "brew install crit", Upgradable: true}, + } + + var mu sync.Mutex + var recorded []string + exec := mockExecutor(&mu, &recorded, "", nil) + + summary := upgradeToolsParallel(items, exec) + + // Solo 2 upgradable → 2 results + if len(summary.Results) != 2 { + t.Fatalf("expected 2 results, got %d", len(summary.Results)) + } + + // El executor no debe haber recibido el command de custom-local (vacío) + mu.Lock() + defer mu.Unlock() + for _, cmd := range recorded { + if cmd == "" { + t.Error("executor was called with empty command (non-upgradable tool should be skipped)") + } + } + + // Los nombres de los resultados deben ser solo los upgradables + for _, r := range summary.Results { + if r.Name == "custom-local" { + t.Error("non-upgradable tool 'custom-local' should not appear in results") + } + } +} + +// TestUpgradeToolsParallel_ConcurrencyCapture usa sync.Mutex para capturar +// commands y verifica que solo los Upgradable=true llegaron al executor. +func TestUpgradeToolsParallel_ConcurrencyCapture(t *testing.T) { + items := []upgradeToolItem{ + {Name: "a", Command: "cmd-a", Upgradable: true}, + {Name: "b", Command: "cmd-b", Upgradable: false}, + {Name: "c", Command: "cmd-c", Upgradable: true}, + {Name: "d", Command: "cmd-d", Upgradable: false}, + {Name: "e", Command: "cmd-e", Upgradable: true}, + } + + var mu sync.Mutex + var recorded []string + + exec := func(command string) error { + mu.Lock() + recorded = append(recorded, command) + mu.Unlock() + return nil + } + + upgradeToolsParallel(items, exec) + + mu.Lock() + got := make([]string, len(recorded)) + copy(got, recorded) + mu.Unlock() + + // Exactamente 3 commands upgradables + if len(got) != 3 { + t.Fatalf("expected 3 commands executed, got %d: %v", len(got), got) + } + + // Todos deben ser de tools upgradables + allowed := map[string]bool{"cmd-a": true, "cmd-c": true, "cmd-e": true} + for _, cmd := range got { + if !allowed[cmd] { + t.Errorf("unexpected command executed: %q (non-upgradable tool reached executor)", cmd) + } + } +} + +// --------------------------------------------------------------------------- +// T016 — Effective command classification tests (buildUpgradeToolItems) +// --------------------------------------------------------------------------- + +func catalogWith(name, command string) map[string]model.ToolDef { + return map[string]model.ToolDef{ + name: {Name: name, Command: command}, + } +} + +// TestBuildUpgradeToolItems_YAMLOverrideWins verifica que el command del +// ToolRef (YAML) gana sobre el catálogo cuando ambos están presentes. +func TestBuildUpgradeToolItems_YAMLOverrideWins(t *testing.T) { + tools := []model.ToolRef{ + {Name: "engram", Command: "custom-override-command"}, + } + catalog := catalogWith("engram", "catalog-command") + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Command != "custom-override-command" { + t.Errorf("expected YAML override to win, got %q", items[0].Command) + } + if !items[0].Upgradable { + t.Error("expected Upgradable=true for non-empty command") + } +} + +// TestBuildUpgradeToolItems_FallbackToCatalog verifica que cuando el ToolRef +// tiene Command vacío, se usa el command del catálogo. +func TestBuildUpgradeToolItems_FallbackToCatalog(t *testing.T) { + tools := []model.ToolRef{ + {Name: "crit", Command: ""}, + } + catalog := catalogWith("crit", "brew install crit") + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Command != "brew install crit" { + t.Errorf("expected catalog fallback command, got %q", items[0].Command) + } + if !items[0].Upgradable { + t.Error("expected Upgradable=true when catalog provides command") + } +} + +// TestBuildUpgradeToolItems_BothEmptyNotUpgradable verifica que cuando tanto +// ToolRef.Command como catalog están vacíos/whitespace, Upgradable=false. +func TestBuildUpgradeToolItems_BothEmptyNotUpgradable(t *testing.T) { + tools := []model.ToolRef{ + {Name: "custom-local", Command: " "}, // whitespace + } + catalog := catalogWith("custom-local", " ") // whitespace en catálogo + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Upgradable { + t.Error("expected Upgradable=false when both YAML and catalog are empty/whitespace") + } +} + +// TestBuildUpgradeToolItems_NoCatalogEntry verifica que cuando no hay entrada +// en el catálogo pero el ToolRef tiene command, se usa ese command. +func TestBuildUpgradeToolItems_NoCatalogEntryWithYAML(t *testing.T) { + tools := []model.ToolRef{ + {Name: "my-tool", Command: "brew install my-tool"}, + } + catalog := map[string]model.ToolDef{} // catálogo vacío + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Command != "brew install my-tool" { + t.Errorf("expected ToolRef command when no catalog entry, got %q", items[0].Command) + } + if !items[0].Upgradable { + t.Error("expected Upgradable=true when ToolRef.Command is present") + } +} + +// TestBuildUpgradeToolItems_NoCatalogEntryNoYAML verifica que cuando no hay +// entrada en el catálogo ni command en ToolRef, Upgradable=false. +func TestBuildUpgradeToolItems_NoCatalogEntryNoYAML(t *testing.T) { + tools := []model.ToolRef{ + {Name: "unknown-tool", Command: ""}, + } + catalog := map[string]model.ToolDef{} // catálogo vacío + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Upgradable { + t.Error("expected Upgradable=false when no catalog entry and no YAML command") + } + if items[0].Command != "" { + t.Errorf("expected empty command, got %q", items[0].Command) + } +} + +// TestBuildUpgradeToolItems_EmptyToolsList verifica que una lista vacía de +// tools produce un slice vacío. +func TestBuildUpgradeToolItems_EmptyToolsList(t *testing.T) { + items := buildUpgradeToolItems(nil, map[string]model.ToolDef{}) + + if len(items) != 0 { + t.Errorf("expected empty slice for nil tools, got %d items", len(items)) + } +} diff --git a/internal/tui/steps/workflow_models.go b/internal/tui/steps/workflow_models.go index 0f828d2..f9399b9 100644 --- a/internal/tui/steps/workflow_models.go +++ b/internal/tui/steps/workflow_models.go @@ -483,7 +483,6 @@ func (m modelSelectorModel) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } - // filterCopilotOptions returns options from all with tier <= maxTier. // The sentinel (ModelInheritOption) is always included regardless of maxTier. func filterCopilotOptions(all []model.ModelOption, maxTier float64) []model.ModelOption { diff --git a/internal/tui/styles.go b/internal/tui/styles.go index cc35e92..fc6f512 100644 --- a/internal/tui/styles.go +++ b/internal/tui/styles.go @@ -37,7 +37,7 @@ func Banner() string { } artStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("10")).Bold(true) // ANSI bright green - dimStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) // ANSI gray + dimStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("8")) // ANSI gray var b strings.Builder b.WriteString("\n") diff --git a/internal/tui/tool_catalog.go b/internal/tui/tool_catalog.go new file mode 100644 index 0000000..1685493 --- /dev/null +++ b/internal/tui/tool_catalog.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: MIT + +package tui + +import ( + "fmt" + "io/fs" + "strings" + + devrune "github.com/davidarce/devrune" + "github.com/davidarce/devrune/internal/model" + "gopkg.in/yaml.v3" +) + +// LoadBuiltinTools reads every .yaml file embedded under tools/ and returns +// the parsed []model.ToolDef slice. Unlike the external catalog scanner, this +// loader is strict: a YAML file that fails to parse OR whose Name field is +// empty causes an error that includes the file path, so catalog authors are +// notified of broken entries at development time. +func LoadBuiltinTools() ([]model.ToolDef, error) { + var tools []model.ToolDef + + err := fs.WalkDir(devrune.BuiltinToolsFS, "tools", func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + + // Skip directories (only process files). + if d.IsDir() { + return nil + } + + // Only process YAML files. + if !strings.HasSuffix(d.Name(), ".yaml") { + return nil + } + + data, err := devrune.BuiltinToolsFS.ReadFile(path) + if err != nil { + return fmt.Errorf("tool_catalog: read %s: %w", path, err) + } + + var tool model.ToolDef + if err := yaml.Unmarshal(data, &tool); err != nil { + return fmt.Errorf("tool_catalog: parse %s: %w", path, err) + } + + if strings.TrimSpace(tool.Name) == "" { + return fmt.Errorf("tool_catalog: %s: tool name must not be empty", path) + } + + tools = append(tools, tool) + return nil + }) + if err != nil { + return nil, err + } + + return tools, nil +} + +// BuiltinToolMap indexes a slice of ToolDef by Name for O(1) lookup. +// Callers should load the slice once with LoadBuiltinTools and then pass it +// here to build the lookup map. +func BuiltinToolMap(tools []model.ToolDef) map[string]model.ToolDef { + m := make(map[string]model.ToolDef, len(tools)) + for _, t := range tools { + m[t.Name] = t + } + return m +} diff --git a/internal/tui/tool_catalog_test.go b/internal/tui/tool_catalog_test.go new file mode 100644 index 0000000..1520576 --- /dev/null +++ b/internal/tui/tool_catalog_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT + +package tui_test + +import ( + "testing" + + "github.com/davidarce/devrune/internal/tui" +) + +func TestLoadBuiltinTools_returnsEngranAndCrit(t *testing.T) { + tools, err := tui.LoadBuiltinTools() + if err != nil { + t.Fatalf("LoadBuiltinTools() error: %v", err) + } + + if len(tools) < 2 { + t.Fatalf("expected at least 2 built-in tools, got %d", len(tools)) + } + + m := tui.BuiltinToolMap(tools) + for _, name := range []string{"engram", "crit"} { + tool, ok := m[name] + if !ok { + t.Errorf("expected built-in tool %q not found in catalog", name) + continue + } + if tool.Name != name { + t.Errorf("tool.Name = %q, want %q", tool.Name, name) + } + if tool.Command == "" { + t.Errorf("tool %q has empty command — built-in catalog entries must have a default command", name) + } + } +} + +func TestBuiltinToolMap_keyedByName(t *testing.T) { + tools, err := tui.LoadBuiltinTools() + if err != nil { + t.Fatalf("LoadBuiltinTools() error: %v", err) + } + + m := tui.BuiltinToolMap(tools) + for _, tool := range tools { + got, ok := m[tool.Name] + if !ok { + t.Errorf("BuiltinToolMap missing key %q", tool.Name) + } + if got.Name != tool.Name { + t.Errorf("map[%q].Name = %q", tool.Name, got.Name) + } + } +} diff --git a/testdata/manifests/valid-full.yaml b/testdata/manifests/valid-full.yaml index be8c52c..f50b9ba 100644 --- a/testdata/manifests/valid-full.yaml +++ b/testdata/manifests/valid-full.yaml @@ -27,3 +27,9 @@ install: rulesMode: claude: concat opencode: individual + +tools: + - name: engram + command: "brew install gentleman-programming/tap/engram" + - name: crit + command: "brew install crit" diff --git a/tooldefs.go b/tooldefs.go new file mode 100644 index 0000000..4c3d4a2 --- /dev/null +++ b/tooldefs.go @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT + +// Package devrune (root package) embeds the built-in tool YAML files. +// This file exists at the module root so that //go:embed can access tools/*.yaml +// without parent-directory traversal (which Go's embed directive does not permit). +package devrune + +import "embed" + +// BuiltinToolsFS exposes the embedded tool definition YAML files. +// Import as: import devrune "github.com/davidarce/devrune" +// +//go:embed tools/*.yaml +var BuiltinToolsFS embed.FS diff --git a/tools/crit.yaml b/tools/crit.yaml new file mode 100644 index 0000000..829791d --- /dev/null +++ b/tools/crit.yaml @@ -0,0 +1,4 @@ +name: crit +description: "AI-powered code review tool that analyses your git diff and provides structured feedback." +command: "brew install crit" +binary: crit diff --git a/tools/engram.yaml b/tools/engram.yaml new file mode 100644 index 0000000..838f1f4 --- /dev/null +++ b/tools/engram.yaml @@ -0,0 +1,4 @@ +name: engram +description: Persistent memory layer for AI coding agents — stores observations, decisions, and session context across conversations. +command: brew install gentleman-programming/tap/engram +binary: engram From 70f6902152ae30c8de91dc7f1e0925953339d3f5 Mon Sep 17 00:00:00 2001 From: David Arce Date: Mon, 11 May 2026 08:35:06 +0200 Subject: [PATCH 2/2] fix(tui): polish Upgrade Tools UX and invert command resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-up fixes on top of the initial Upgrade Tools feature based on real-usage feedback: - Catalog wins over manifest. Resolution was 'manifest command first, catalog fallback'. A stale 'command:' override in the user's devrune.yaml (e.g. 'brew install tomasz-tomczyk/tap/crit') would silently shadow the catalog's correct command and fail. Invert the order: catalog always wins for known tools; manifest 'command:' only serves unknown tools as an extensibility hatch. - Drop the redundant preview screen. The flow showed a Note listing upgradable tools (advances on any key, contradicting the 'enter submit' hint) immediately before a Confirm screen that already listed the same tools with their commands. Land directly on the Confirm screen. - Add the standard '◆ DevRune' header to every Upgrade Tools screen (preview/confirm/summary/empty/all-non-upgradable) via BannerNote(), matching the convention used by 'devrune upgrade'. Tests updated: 'manifest wins' assertion inverted; new cases cover 'catalog wins even when manifest differs' and 'manifest used when catalog entry is empty' (fallback path for unknown tools). Refs davidarce/devrune-starter-catalog#71 (corrects the upstream crit command so users picking up the new catalog get the fix automatically). --- internal/tui/steps/tool_upgrade.go | 59 +++++++++---------------- internal/tui/steps/tool_upgrade_test.go | 53 +++++++++++++++++++--- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/internal/tui/steps/tool_upgrade.go b/internal/tui/steps/tool_upgrade.go index 37f71e4..05548ed 100644 --- a/internal/tui/steps/tool_upgrade.go +++ b/internal/tui/steps/tool_upgrade.go @@ -130,18 +130,19 @@ func upgradeToolsParallel(items []upgradeToolItem, execFn ToolCommandExecutor) T // buildUpgradeToolItems resolves the effective upgrade command for each tool // reference and classifies it as upgradable or not. // -// Resolution order: -// 1. strings.TrimSpace(ref.Command) non-empty → use it. -// 2. strings.TrimSpace(catalog[ref.Name].Command) non-empty → use it. +// Resolution order (catalog wins; manifest is fallback for unknown tools): +// 1. strings.TrimSpace(catalog[ref.Name].Command) non-empty → use it. +// 2. strings.TrimSpace(ref.Command) non-empty → use it. // 3. otherwise → Upgradable = false. func buildUpgradeToolItems(tools []model.ToolRef, catalog map[string]model.ToolDef) []upgradeToolItem { items := make([]upgradeToolItem, len(tools)) for i, ref := range tools { - cmd := strings.TrimSpace(ref.Command) + var cmd string + if def, ok := catalog[ref.Name]; ok { + cmd = strings.TrimSpace(def.Command) + } if cmd == "" { - if def, ok := catalog[ref.Name]; ok { - cmd = strings.TrimSpace(def.Command) - } + cmd = strings.TrimSpace(ref.Command) } items[i] = upgradeToolItem{ Name: ref.Name, @@ -158,10 +159,9 @@ func buildUpgradeToolItems(tools []model.ToolRef, catalog map[string]model.ToolD // RunToolUpgradeStep presents the upgrade-tools TUI flow: // 1. Empty state when len(tools)==0. -// 2. Preview list (upgradable vs. "no upgradable"). -// 3. Explicit yes/no confirmation. -// 4. Parallel upgrade with spinner (T014). -// 5. Result summary. +// 2. Explicit yes/no confirmation listing tools and their effective commands. +// 3. Parallel upgrade with spinner (T014). +// 4. Result summary. // // Returns an empty summary and nil error when the user cancels or there is // nothing to run. @@ -185,19 +185,14 @@ func runToolUpgradeStepWithExecutor( // ── Build items ────────────────────────────────────────────────────────── items := buildUpgradeToolItems(tools, catalog) - // ── Preview (huh Note) ─────────────────────────────────────────────────── - var previewLines strings.Builder upgradableCount := 0 for _, it := range items { if it.Upgradable { - fmt.Fprintf(&previewLines, " ✓ %s\n", it.Name) upgradableCount++ - } else { - fmt.Fprintf(&previewLines, " - %s (no upgradable)\n", it.Name) } } - confirmed, err := showUpgradePreviewAndConfirm(previewLines.String(), items, upgradableCount) + confirmed, err := showUpgradeConfirm(items, upgradableCount) if err != nil { return ToolUpgradeSummary{}, err } @@ -230,6 +225,7 @@ func showUpgradeEmptyState() (ToolUpgradeSummary, error) { form := huh.NewForm( huh.NewGroup( + BannerNote(), huh.NewNote(). Title("Upgrade Tools"). Description("No hay tools para actualizar.\n\n" + body), @@ -250,6 +246,7 @@ func showUpgradeEmptyState() (ToolUpgradeSummary, error) { func showAllNonUpgradable() (ToolUpgradeSummary, error) { form := huh.NewForm( huh.NewGroup( + BannerNote(), huh.NewNote(). Title("Upgrade Tools"). Description("Ninguna tool tiene un comando de upgrade efectivo.\n\nRevisa que tus tools tengan un campo 'command' en devrune.yaml o en el catálogo embebido."), @@ -266,26 +263,10 @@ func showAllNonUpgradable() (ToolUpgradeSummary, error) { return ToolUpgradeSummary{}, nil } -// showUpgradePreviewAndConfirm renders the preview note then an explicit yes/no -// confirmation. Returns true only when the user selects "Yes". -func showUpgradePreviewAndConfirm(previewLines string, items []upgradeToolItem, upgradableCount int) (bool, error) { - // Preview step. - previewForm := huh.NewForm( - huh.NewGroup( - huh.NewNote(). - Title("Upgrade Tools"). - Description("Estas tools están declaradas en devrune.yaml.\nSolo se ejecutarán las que tengan command efectivo.\n\n" + previewLines), - ), - ).WithTheme(tuistyles.DevRuneThemeFunc). - WithViewHook(func(v tea.View) tea.View { - v.AltScreen = true - return v - }) - - if err := previewForm.Run(); err != nil { - return false, err - } - +// showUpgradeConfirm renders an explicit yes/no confirmation listing each tool +// with its effective command (or marked as non-upgradable). Returns true only +// when the user selects "Yes". +func showUpgradeConfirm(items []upgradeToolItem, upgradableCount int) (bool, error) { if upgradableCount == 0 { // No need for the confirm step; caller handles the "all non-upgradable" case. return true, nil @@ -297,6 +278,8 @@ func showUpgradePreviewAndConfirm(previewLines string, items []upgradeToolItem, for _, it := range items { if it.Upgradable { fmt.Fprintf(&confirmLines, " %s → %s\n", it.Name, it.Command) + } else { + fmt.Fprintf(&confirmLines, " %s (no upgradable)\n", it.Name) } } confirmLines.WriteString("\n¿Ejecutar upgrades ahora?") @@ -304,6 +287,7 @@ func showUpgradePreviewAndConfirm(previewLines string, items []upgradeToolItem, var choice string confirmForm := huh.NewForm( huh.NewGroup( + BannerNote(), huh.NewSelect[string](). Title("Confirm Upgrade Tools"). Description(confirmLines.String()). @@ -462,6 +446,7 @@ func showUpgradeSummary(summary ToolUpgradeSummary) error { form := huh.NewForm( huh.NewGroup( + BannerNote(), huh.NewNote(). Title("Upgrade Tools Complete"). Description(sb.String()), diff --git a/internal/tui/steps/tool_upgrade_test.go b/internal/tui/steps/tool_upgrade_test.go index 904aeb6..97b283f 100644 --- a/internal/tui/steps/tool_upgrade_test.go +++ b/internal/tui/steps/tool_upgrade_test.go @@ -198,9 +198,11 @@ func catalogWith(name, command string) map[string]model.ToolDef { } } -// TestBuildUpgradeToolItems_YAMLOverrideWins verifica que el command del -// ToolRef (YAML) gana sobre el catálogo cuando ambos están presentes. -func TestBuildUpgradeToolItems_YAMLOverrideWins(t *testing.T) { +// TestBuildUpgradeToolItems_CatalogWinsOverManifest verifica que el command +// del catálogo gana sobre el ToolRef (manifest YAML) cuando ambos están +// presentes. El manifest queda como fallback únicamente para tools que NO +// están en el catálogo (extensibilidad para tools desconocidas). +func TestBuildUpgradeToolItems_CatalogWinsOverManifest(t *testing.T) { tools := []model.ToolRef{ {Name: "engram", Command: "custom-override-command"}, } @@ -211,14 +213,55 @@ func TestBuildUpgradeToolItems_YAMLOverrideWins(t *testing.T) { if len(items) != 1 { t.Fatalf("expected 1 item, got %d", len(items)) } - if items[0].Command != "custom-override-command" { - t.Errorf("expected YAML override to win, got %q", items[0].Command) + if items[0].Command != "catalog-command" { + t.Errorf("expected catalog command to win, got %q", items[0].Command) } if !items[0].Upgradable { t.Error("expected Upgradable=true for non-empty command") } } +// TestBuildUpgradeToolItems_CatalogWinsEvenWhenManifestDiffers verifica +// explícitamente que cuando catalog y manifest tienen comandos distintos +// (no vacíos), el catálogo siempre gana. +func TestBuildUpgradeToolItems_CatalogWinsEvenWhenManifestDiffers(t *testing.T) { + tools := []model.ToolRef{ + {Name: "crit", Command: "brew install crit-old"}, + } + catalog := catalogWith("crit", "brew install crit") + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Command != "brew install crit" { + t.Errorf("expected catalog command to win over manifest, got %q", items[0].Command) + } +} + +// TestBuildUpgradeToolItems_ManifestUsedWhenCatalogEntryEmpty verifica que +// si la entrada del catálogo existe pero su Command está vacío/whitespace, +// se usa el command del manifest como fallback. +func TestBuildUpgradeToolItems_ManifestUsedWhenCatalogEntryEmpty(t *testing.T) { + tools := []model.ToolRef{ + {Name: "crit", Command: "brew install crit"}, + } + catalog := catalogWith("crit", " ") // whitespace + + items := buildUpgradeToolItems(tools, catalog) + + if len(items) != 1 { + t.Fatalf("expected 1 item, got %d", len(items)) + } + if items[0].Command != "brew install crit" { + t.Errorf("expected manifest fallback when catalog empty, got %q", items[0].Command) + } + if !items[0].Upgradable { + t.Error("expected Upgradable=true when manifest provides command") + } +} + // TestBuildUpgradeToolItems_FallbackToCatalog verifica que cuando el ToolRef // tiene Command vacío, se usa el command del catálogo. func TestBuildUpgradeToolItems_FallbackToCatalog(t *testing.T) {