Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions internal/cli/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
},
Comment on lines +522 to +543
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Validate-before-clean ordering is the right fix; consider extracting the context builder.

Placement after Backup and before Clean Managed Paths correctly closes the transactional gap from #733: a parse error now aborts before any destructive op runs.

The TemplateContext construction here (lines 526–534) is byte-for-byte identical to the one inside the Deploy Templates step at lines 558–566. If a future field (e.g., WithUserName) is added in one place and forgotten in the other, validation and deployment will diverge silently — exactly the kind of drift this PR is trying to prevent.

♻️ Proposed refactor — extract a single context builder
// buildUpdateTemplateContext constructs the TemplateContext used by both
// the Validate Templates and Deploy Templates steps.
func buildUpdateTemplateContext() *template.TemplateContext {
	homeDir, _ := userHomeDir()
	goBinPath := detectGoBinPathForUpdate(homeDir)
	return template.NewTemplateContext(
		template.WithGoBinPath(goBinPath),
		template.WithHomeDir(homeDir),
		template.WithSmartPATH(template.BuildSmartPATH()),
		template.WithPlatform(runtime.GOOS),
		template.WithVersion(version.GetVersion()),
	)
}

Then both steps call tmplCtx := buildUpdateTemplateContext().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/cli/update.go` around lines 522 - 543, The TemplateContext
construction in the Validate Templates step is duplicated in the Deploy
Templates step; extract a single helper (e.g., buildUpdateTemplateContext) that
calls userHomeDir(), detectGoBinPathForUpdate(), and returns
template.NewTemplateContext(...) with the same
template.WithGoBinPath/WithHomeDir/WithSmartPATH/WithPlatform/WithVersion
options, then replace the inline constructions in both the "Validate Templates"
and "Deploy Templates" steps with tmplCtx := buildUpdateTemplateContext() so
both use the identical context builder.

{
name: "Clean Managed Paths",
message: "Removing old MoAI-managed files",
Expand Down
8 changes: 8 additions & 0 deletions internal/core/project/initializer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
56 changes: 56 additions & 0 deletions internal/template/deployer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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]
}
Comment on lines +229 to +264
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🌐 Web query:

Is errors.Join available in the Go standard library and what is the minimum Go version that supports it?

💡 Result:

Yes, errors.Join is available in the Go standard library in the errors package. It was added in Go 1.20 and is the minimum version that supports it.

Citations:


🏁 Script executed:

# Check Go version requirement
cat go.mod | head -5

Repository: modu-ai/moai-adk

Length of output: 115


🏁 Script executed:

# Check imports in deployer.go
head -50 internal/template/deployer.go | grep -E "^import|errors"

Repository: modu-ai/moai-adk

Length of output: 69


🏁 Script executed:

# Get the full import block from deployer.go
sed -n '1,30p' internal/template/deployer.go

Repository: modu-ai/moai-adk

Length of output: 1338


🏁 Script executed:

# Check the actual code at lines 229-264
sed -n '229,264p' internal/template/deployer.go

Repository: modu-ai/moai-adk

Length of output: 874


Consider reporting all validation errors, not just the first.

The walk continues past the first failure and accumulates validationErrors, but only validationErrors[0] is returned. Either short-circuit on the first error or use errors.Join to surface the full list. Returning all errors is friendlier: when a template audit catches three broken .tmpl files, the user fixes them in one cycle instead of three.

♻️ Proposed change
 	if walkErr != nil {
 		return walkErr
 	}

-	if len(validationErrors) > 0 {
-		// Return the first validation error
-		return validationErrors[0]
-	}
-
-	return nil
+	return errors.Join(validationErrors...)
 }

(Add "errors" to the import block.)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/template/deployer.go` around lines 229 - 264, The validation
currently accumulates errors in validationErrors inside the fs.WalkDir loop (in
deployTemplates / the validation block using d.renderer.Render) but returns only
validationErrors[0]; change the final return to surface all problems by
returning errors.Join(validationErrors...) when len(validationErrors) > 0, and
add "errors" to the imports. Alternatively, short-circuit by returning the first
renderErr from inside the WalkDir callback, but preferred is using errors.Join
to return the combined error so users see every template failure at once.


return nil
}

// validateDeployPath ensures a template path does not escape projectRoot.
func validateDeployPath(projectRoot, relPath string) error {
// Clean and normalize
Expand Down
46 changes: 46 additions & 0 deletions internal/template/deployer_mode.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +178 to +222
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Duplicated ValidateAll — extract a shared helper.

This implementation is identical to (*deployer).ValidateAll in internal/template/deployer.go:223-267. Both struct types share fsys fs.FS and renderer Renderer fields, so a package-level helper avoids divergence drift (e.g., if one side adds parallelism, retry, or errors.Join and the other doesn't).

♻️ Proposed shared helper
// validateAllTemplates renders every *.tmpl file in fsys with the provided
// renderer/tmplCtx and returns aggregated parse/render errors, if any.
// Returns nil if renderer is nil (no rendering configured).
func validateAllTemplates(ctx context.Context, fsys fs.FS, renderer Renderer, tmplCtx *TemplateContext) error {
	if renderer == nil {
		return nil
	}

	var validationErrors []error
	walkErr := fs.WalkDir(fsys, ".", func(path string, entry fs.DirEntry, err error) error {
		if err != nil {
			return err
		}
		select {
		case <-ctx.Done():
			return ctx.Err()
		default:
		}
		if path == "." || entry.IsDir() || !strings.HasSuffix(path, ".tmpl") {
			return nil
		}
		if _, renderErr := renderer.Render(path, tmplCtx); renderErr != nil {
			validationErrors = append(validationErrors,
				fmt.Errorf("template %q: %w", path, renderErr))
		}
		return nil
	})
	if walkErr != nil {
		return walkErr
	}
	return errors.Join(validationErrors...)
}

Then both (*deployer).ValidateAll and (*modeAwareDeployer).ValidateAll become one-liners delegating to this helper.

Also: as per coding guidelines, please add an @MX:NOTE annotation here to mirror the one on (*deployer).ValidateAll in internal/template/deployer.go:219 for consistency.

As per coding guidelines: Create and update @mxcode annotations:@mx:NOTE for context, @mx:WARN for danger zones (requires @mx:REASON), @mx:ANCHOR for invariant contracts, @mx:TODO for incomplete work.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/template/deployer_mode.go` around lines 178 - 222, Extract the
duplicated logic in (*deployer).ValidateAll and (*modeAwareDeployer).ValidateAll
into a package-level helper function validateAllTemplates(ctx context.Context,
fsys fs.FS, renderer Renderer, tmplCtx *TemplateContext) that performs the
WalkDir, context cancellation check, *.tmpl filtering, calls renderer.Render and
aggregates errors with errors.Join (return nil if renderer is nil); then change
both ValidateAll methods to a one-line return of validateAllTemplates(ctx,
d.fsys, d.renderer, tmplCtx). Also add the same `@MX`:NOTE annotation above the
new helper (mirroring the `@MX`:NOTE on (*deployer).ValidateAll) to keep
annotations consistent.

152 changes: 152 additions & 0 deletions internal/template/deployer_transactional_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
}
})
Comment on lines +17 to +54
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

This test passes by alphabetical luck — Deploy is not transactional on its own.

Deploy in internal/template/deployer.go:72-185 renders and writes each file inside the fs.WalkDir callback; there is no upfront validation pass. The reason no files appear on disk here is that fs.WalkDir visits entries lexically and "invalid.tmpl" < "valid.tmpl", so the broken template is hit first and aborts the walk before valid.tmpl is ever reached.

Rename valid.tmpl to a-valid.tmpl and the assertion len(entries) > 0 fails — a-valid.txt would be written to disk before invalid.tmpl errors out. The actual transactional guarantee in this PR lives at the CLI layer (internal/cli/update.go calls ValidateAll before Deploy); this test should reflect that.

🐛 Two ways to fix

Option A — Test the real transactional pattern (preferred):

 		fs := fstest.MapFS{
-			"valid.tmpl": &fstest.MapFile{
+			"a-valid.tmpl": &fstest.MapFile{
 				Data: []byte("Hello {{.ProjectName}}"),
 			},
-			"invalid.tmpl": &fstest.MapFile{
+			"b-invalid.tmpl": &fstest.MapFile{
 				Data: []byte("Broken {{ } } template"),
 			},
 		}
 		...
-		// Deploy should fail during validation
-		err := d.Deploy(ctx, tempDir, mgr, tmplCtx)
+		// Transactional pattern: ValidateAll first, then Deploy only if valid.
+		if err := d.ValidateAll(ctx, tmplCtx); err == nil {
+			t.Fatal("expected ValidateAll to reject invalid template")
+		}
+		// Caller must NOT proceed to Deploy after validation failure.
+		// Verify nothing was written by ValidateAll itself.
+		entries, _ := os.ReadDir(tempDir)
+		if len(entries) > 0 {
+			t.Errorf("ValidateAll must not write files, got %d", len(entries))
+		}

Option B — Keep the Deploy-only test but make ordering explicit (renames force the valid file to come first so a regression in transactionality is actually observable).

Either way, the current test name validation_happens_before_file_writes should change to match what's actually being asserted.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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("validation_happens_before_file_writes", func(t *testing.T) {
// Create a filesystem with one valid template and one invalid template
fs := fstest.MapFS{
"a-valid.tmpl": &fstest.MapFile{
Data: []byte("Hello {{.ProjectName}}"),
},
"b-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))
// Transactional pattern: ValidateAll first, then Deploy only if valid.
if err := d.ValidateAll(ctx, tmplCtx); err == nil {
t.Fatal("expected ValidateAll to reject invalid template")
}
// Caller must NOT proceed to Deploy after validation failure.
// Verify nothing was written by ValidateAll itself.
entries, _ := os.ReadDir(tempDir)
if len(entries) > 0 {
t.Errorf("ValidateAll must not write files, got %d", len(entries))
}
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/template/deployer_transactional_test.go` around lines 17 - 54, The
test currently relies on lexicographic visit order; make the ordering explicit
and rename the test to reflect that Deploy alone is not transactional: in the
test function (was validation_happens_before_file_writes) rename it to something
like TestDeployIsNotTransactionalWhenRunDirectly, rename "valid.tmpl" to
"a-valid.tmpl" in the fstest.MapFS so the valid file is visited and written
before the invalid one, and update the assertion messages to match the new
expectation (i.e., expect at least one file written when a valid entry precedes
the invalid entry). Reference symbols: the test function name, NewRenderer,
NewDeployerWithRenderer, and the Deploy call.


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")
}
})
}
28 changes: 28 additions & 0 deletions internal/template/llm_panel_integration_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading
Loading