diff --git a/internal/cli/update.go b/internal/cli/update.go index 632e3a2a0..5581af5d5 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -519,6 +519,28 @@ func runTemplateSyncWithReporter(cmd *cobra.Command, reporter project.ProgressRe return nil }, }, + { + name: "Validate Templates", + message: "Validating all templates before deployment", + execute: func() error { + homeDir, _ := userHomeDir() + goBinPath := detectGoBinPathForUpdate(homeDir) + tmplCtx := template.NewTemplateContext( + template.WithGoBinPath(goBinPath), + template.WithHomeDir(homeDir), + template.WithSmartPATH(template.BuildSmartPATH()), + template.WithPlatform(runtime.GOOS), + template.WithVersion(version.GetVersion()), + ) + + if validateErr := deployer.ValidateAll(ctx, tmplCtx); validateErr != nil { + _, _ = fmt.Fprintf(out, "\r %s Template validation failed: %v\n", symError(), validateErr) + return fmt.Errorf("template validation: %w", validateErr) + } + _, _ = fmt.Fprintf(out, "\r %s All templates validated\n", symSuccess()) + return nil + }, + }, { name: "Clean Managed Paths", message: "Removing old MoAI-managed files", diff --git a/internal/core/project/initializer_test.go b/internal/core/project/initializer_test.go index 324a17c22..2b220851f 100644 --- a/internal/core/project/initializer_test.go +++ b/internal/core/project/initializer_test.go @@ -35,6 +35,10 @@ func (m *mockDeployer) ListTemplates() []string { return nil } +func (m *mockDeployer) ValidateAll(_ context.Context, _ *template.TemplateContext) error { + return nil +} + // --- Initializer tests --- func TestInit_BasicInitialization(t *testing.T) { @@ -687,6 +691,10 @@ func (d *trackingMockDeployer) ListTemplates() []string { return list } +func (d *trackingMockDeployer) ValidateAll(_ context.Context, _ *template.TemplateContext) error { + return nil +} + // --- Test helpers --- func assertFileExists(t *testing.T, path string) { diff --git a/internal/template/deployer.go b/internal/template/deployer.go index abc8dade4..bf2c4eab2 100644 --- a/internal/template/deployer.go +++ b/internal/template/deployer.go @@ -22,6 +22,12 @@ type Deployer interface { // .tmpl are rendered with the context and saved without the .tmpl suffix. Deploy(ctx context.Context, projectRoot string, m manifest.Manager, tmplCtx *TemplateContext) error + // ValidateAll validates all templates without writing any files. + // This is used to implement transactional deployment: validate first, + // then deploy. If any template fails to parse or render, the error is + // returned immediately and no files are written. + ValidateAll(ctx context.Context, tmplCtx *TemplateContext) error + // ExtractTemplate returns the raw content of a single template by name. ExtractTemplate(name string) ([]byte, error) @@ -210,6 +216,56 @@ func (d *deployer) ListTemplates() []string { return list } +// @MX:NOTE: [AUTO] Implements transactional validation - renders all templates in memory without writing to disk +// ValidateAll validates all templates without writing any files. +// This implements transactional deployment: validate first, then deploy. +// If any template fails to parse or render, the error is returned immediately. +func (d *deployer) ValidateAll(ctx context.Context, tmplCtx *TemplateContext) error { + // Only validate if we have a renderer configured + if d.renderer == nil { + return nil + } + + var validationErrors []error + walkErr := fs.WalkDir(d.fsys, ".", func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + + // Check context cancellation during validation + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + + // Skip directories and non-templates + if path == "." || entry.IsDir() || !strings.HasSuffix(path, ".tmpl") { + return nil + } + + // Try to render the template (this will catch parse errors) + _, renderErr := d.renderer.Render(path, tmplCtx) + if renderErr != nil { + validationErrors = append(validationErrors, + fmt.Errorf("template %q: %w", path, renderErr)) + } + + return nil + }) + + if walkErr != nil { + return walkErr + } + + if len(validationErrors) > 0 { + // Return the first validation error + return validationErrors[0] + } + + return nil +} + // validateDeployPath ensures a template path does not escape projectRoot. func validateDeployPath(projectRoot, relPath string) error { // Clean and normalize diff --git a/internal/template/deployer_mode.go b/internal/template/deployer_mode.go index 23c10d8b6..6c79b8e6e 100644 --- a/internal/template/deployer_mode.go +++ b/internal/template/deployer_mode.go @@ -174,3 +174,49 @@ func (d *modeAwareDeployer) ListTemplates() []string { return list } + +// ValidateAll validates all templates without writing any files. +func (d *modeAwareDeployer) ValidateAll(ctx context.Context, tmplCtx *TemplateContext) error { + // Only validate if we have a renderer configured + if d.renderer == nil { + return nil + } + + var validationErrors []error + walkErr := fs.WalkDir(d.fsys, ".", func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + + // Check context cancellation + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + + // Skip directories and non-templates + if path == "." || entry.IsDir() || !strings.HasSuffix(path, ".tmpl") { + return nil + } + + // Try to render the template (this will catch parse errors) + _, renderErr := d.renderer.Render(path, tmplCtx) + if renderErr != nil { + validationErrors = append(validationErrors, + fmt.Errorf("template %q: %w", path, renderErr)) + } + + return nil + }) + + if walkErr != nil { + return walkErr + } + + if len(validationErrors) > 0 { + return validationErrors[0] + } + + return nil +} diff --git a/internal/template/deployer_transactional_test.go b/internal/template/deployer_transactional_test.go new file mode 100644 index 000000000..dfd583c12 --- /dev/null +++ b/internal/template/deployer_transactional_test.go @@ -0,0 +1,152 @@ +package template + +import ( + "context" + "os" + "path/filepath" + "testing" + "testing/fstest" + + "github.com/modu-ai/moai-adk/internal/manifest" +) + +// TestDeployer_TransactionalValidation verifies that template validation happens +// BEFORE any files are written to disk. This prevents leaving the project in a +// broken state if a template fails to parse (issue #733). +func TestDeployer_TransactionalValidation(t *testing.T) { + t.Run("validation_happens_before_file_writes", func(t *testing.T) { + // Create a filesystem with one valid template and one invalid template + fs := fstest.MapFS{ + "valid.tmpl": &fstest.MapFile{ + Data: []byte("Hello {{.ProjectName}}"), + }, + "invalid.tmpl": &fstest.MapFile{ + // This template has a syntax error: unescaped }} + Data: []byte("Broken {{ } } template"), + }, + } + + r := NewRenderer(fs) + d := NewDeployerWithRenderer(fs, r) + + // Create a temp directory for deployment + tempDir := t.TempDir() + mgr := manifest.NewManager() + + ctx := context.Background() + tmplCtx := NewTemplateContext(WithProject("Test", tempDir)) + + // Deploy should fail during validation + err := d.Deploy(ctx, tempDir, mgr, tmplCtx) + if err == nil { + t.Error("expected error for invalid template, got nil") + } + + // CRITICAL: No files should have been written to disk + // because validation failed BEFORE the write phase + entries, _ := os.ReadDir(tempDir) + if len(entries) > 0 { + t.Errorf("expected no files written after validation failure, got %d files", len(entries)) + for _, e := range entries { + t.Logf(" - %s", e.Name()) + } + } + }) + + t.Run("valid_templates_write_after_validation", func(t *testing.T) { + // Create a filesystem with valid templates only + fs := fstest.MapFS{ + "file1.txt.tmpl": &fstest.MapFile{ + Data: []byte("Content 1: {{.Version}}"), + }, + "file2.txt.tmpl": &fstest.MapFile{ + Data: []byte("Content 2: {{.Version}}"), + }, + } + + r := NewRenderer(fs) + d := NewDeployerWithRenderer(fs, r) + + tempDir := t.TempDir() + mgr := manifest.NewManager() + // Initialize manifest in temp directory + _, _ = mgr.Load(tempDir) + + ctx := context.Background() + tmplCtx := NewTemplateContext(WithVersion("test-value")) + + // Deploy should succeed + err := d.Deploy(ctx, tempDir, mgr, tmplCtx) + if err != nil { + t.Fatalf("expected no error for valid templates, got: %v", err) + } + + // Files should have been written + file1 := filepath.Join(tempDir, "file1.txt") + file2 := filepath.Join(tempDir, "file2.txt") + + if _, err := os.Stat(file1); os.IsNotExist(err) { + t.Error("file1.txt should have been written") + } + if _, err := os.Stat(file2); os.IsNotExist(err) { + t.Error("file2.txt should have been written") + } + + // Verify content + content1, _ := os.ReadFile(file1) + if string(content1) != "Content 1: test-value" { + t.Errorf("file1.txt content = %q, want %q", string(content1), "Content 1: test-value") + } + }) +} + +// TestDeployer_ValidateAllTemplates verifies a new method that validates +// all templates without writing any files. +func TestDeployer_ValidateAllTemplates(t *testing.T) { + t.Run("all_templates_valid", func(t *testing.T) { + fs := fstest.MapFS{ + "template1.tmpl": &fstest.MapFile{ + Data: []byte("Value: {{.Version}}"), + }, + "template2.tmpl": &fstest.MapFile{ + Data: []byte("Name: {{.ProjectName}}"), + }, + } + + r := NewRenderer(fs) + d := NewDeployerWithRenderer(fs, r) + + ctx := context.Background() + tmplCtx := NewTemplateContext(WithVersion("test"), WithProject("Goos", "/tmp/goos")) + + // Validate should succeed + err := d.ValidateAll(ctx, tmplCtx) + if err != nil { + t.Errorf("expected no validation error, got: %v", err) + } + }) + + t.Run("invalid_template_fails_validation", func(t *testing.T) { + fs := fstest.MapFS{ + "valid.tmpl": &fstest.MapFile{ + Data: []byte("OK: {{.Version}}"), + }, + "invalid.tmpl": &fstest.MapFile{ + // Syntax error: missing closing brace + Data: []byte("Broken: {{.Version"), + }, + } + + r := NewRenderer(fs) + d := NewDeployerWithRenderer(fs, r) + + ctx := context.Background() + tmplCtx := NewTemplateContext(WithVersion("test")) + + // Validate should fail + err := d.ValidateAll(ctx, tmplCtx) + if err == nil { + t.Error("expected validation error for invalid template, got nil") + } + }) +} diff --git a/internal/template/llm_panel_integration_test.go b/internal/template/llm_panel_integration_test.go new file mode 100644 index 000000000..fa6e861db --- /dev/null +++ b/internal/template/llm_panel_integration_test.go @@ -0,0 +1,28 @@ +package template + +import ( + "context" + "testing" +) + +// TestLLMPanelTemplate_Integration validates the actual llm-panel.yml.tmpl +// from the embedded filesystem can be parsed and rendered. +func TestLLMPanelTemplate_Integration(t *testing.T) { + // Get the embedded filesystem + embeddedFS, err := EmbeddedTemplates() + if err != nil { + t.Fatalf("failed to get embedded templates: %v", err) + } + + r := NewRenderer(embeddedFS) + d := NewDeployerWithRenderer(embeddedFS, r) + + ctx := context.Background() + tmplCtx := NewTemplateContext(WithProject("test-project", "/tmp/test")) + + // ValidateAll should succeed with the fixed template + err = d.ValidateAll(ctx, tmplCtx) + if err != nil { + t.Fatalf("expected llm-panel.yml.tmpl to validate successfully, got: %v", err) + } +} diff --git a/internal/template/llm_panel_test.go b/internal/template/llm_panel_test.go new file mode 100644 index 000000000..a4dbd8065 --- /dev/null +++ b/internal/template/llm_panel_test.go @@ -0,0 +1,134 @@ +package template + +import ( + "testing" + "testing/fstest" +) + +// TestLLMPanelTemplate_Rendering verifies that llm-panel.yml.tmpl can be parsed +// and rendered without errors. This test reproduces issue #733 where unescaped +// GitHub Actions expressions ${{ }} cause Go text/template parse errors. +func TestLLMPanelTemplate_Rendering(t *testing.T) { + t.Run("reproduce_issue_733", func(t *testing.T) { + // This is the BROKEN content from llm-panel.yml.tmpl that causes the parse error + // The issue is on lines 28, 33, 38 where ${{ }} expressions are not properly escaped + brokenTemplate := `name: LLM Review Panel +on: + pull_request: + types: [opened] + +permissions: + contents: read + pull-requests: write + +jobs: + panel: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Post LLM Panel Comment + uses: actions/github-script@v7 + with: + github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}} + + script: | + gh pr comment ${{ "{{" }} github.event.pull_request.number }} --body "## LLM Code Review Status + + +| LLM | Status | +|-----|--------| +| Claude | Pending (add /claude comment) | +| Codex | ${{ "{{" } } } { + if secrets.CODEX_AUTH_JSON != '' then 'Ready' + else 'Token missing' + end +{{ "}" } } | +` + + fs := fstest.MapFS{ + "llm-panel.yml.tmpl": &fstest.MapFile{ + Data: []byte(brokenTemplate), + }, + } + + r := NewRenderer(fs) + + // This should fail with a parse error because of unescaped ${{ }} on line 28 + _, err := r.Render("llm-panel.yml.tmpl", nil) + if err == nil { + t.Error("expected parse error for unescaped GitHub Actions expressions, got nil") + } else { + t.Logf("Got expected error (reproducing issue #733): %v", err) + } + }) + + t.Run("fixed_template_renders", func(t *testing.T) { + // This is the FIXED version with proper escaping + fixedTemplate := `name: LLM Review Panel +on: + pull_request: + types: [opened] + +permissions: + contents: read + pull-requests: write + +jobs: + panel: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Post LLM Panel Comment + uses: actions/github-script@v7 + with: + github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}} + + script: | + gh pr comment ${{ "{{" }} github.event.pull_request.number }} --body "## LLM Code Review Status + + +| LLM | Status | +|-----|--------| +| Claude | Pending (add /claude comment) | +| Codex | ${{ "{{" }} secrets.CODEX_AUTH_JSON != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | +| Gemini | ${{ "{{" }} secrets.GEMINI_API_KEY != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | +| GLM | ${{ "{{" }} secrets.GLM_API_KEY != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | + +Trigger individual reviews: +- Add /claude comment to trigger Claude +- Add /codex comment to trigger Codex +- Add /gemini comment to trigger Gemini +- Add /glm comment to trigger GLM +" +` + + fs := fstest.MapFS{ + "llm-panel.yml.tmpl": &fstest.MapFile{ + Data: []byte(fixedTemplate), + }, + } + + r := NewRenderer(fs) + + // This should succeed with the fixed template + result, err := r.Render("llm-panel.yml.tmpl", nil) + if err != nil { + t.Fatalf("expected no error with fixed template, got: %v", err) + } + + // Verify the output contains properly escaped GitHub Actions expressions + output := string(result) + if !contains(output, "${{ secrets.GITHUB_TOKEN }}") { + t.Error("output should contain escaped GitHub Actions token reference") + } + if !contains(output, "${{ secrets.CODEX_AUTH_JSON != '' && '✓ Ready' || '⚠️ Token missing' }}") { + t.Error("output should contain properly escaped Codex conditional") + } + }) +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && (s[:len(substr)] == substr || contains(s[1:], substr))) +} diff --git a/internal/template/templates/.github/workflows/llm-panel.yml.tmpl b/internal/template/templates/.github/workflows/llm-panel.yml.tmpl index 34e0f73bb..9afeac1c0 100644 --- a/internal/template/templates/.github/workflows/llm-panel.yml.tmpl +++ b/internal/template/templates/.github/workflows/llm-panel.yml.tmpl @@ -25,21 +25,9 @@ jobs: | LLM | Status | |-----|--------| | Claude | Pending (add \`/claude\` comment) | -| Codex | ${{ "{{" } } } { - if secrets.CODEX_AUTH_JSON != '' then '✓ Ready' - else '⚠️ Token missing' - end -{{ "}" } } | -| Gemini | ${{ "{{" } } } { - if secrets.GEMINI_API_KEY != '' then '✓ Ready' - else '⚠️ Token missing' - end -{{ "}" } } | -| GLM | ${{ "{{" } } } { - if secrets.GLM_API_KEY != '' then '✓ Ready' - else '⚠️ Token missing' - end -{{ "}" } } | +| Codex | ${{ "{{" }} secrets.CODEX_AUTH_JSON != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | +| Gemini | ${{ "{{" }} secrets.GEMINI_API_KEY != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | +| GLM | ${{ "{{" }} secrets.GLM_API_KEY != '' && '✓ Ready' || '⚠️ Token missing' {{ "}}" }} | Trigger individual reviews: - Add \`/claude\` comment to trigger Claude