fix(template): escape GitHub Actions expressions + transactional deploy (#733)#736
fix(template): escape GitHub Actions expressions + transactional deploy (#733)#736
Conversation
…ransactional deploy (#733) The embedded template llm-panel.yml.tmpl had unescaped GitHub Actions ${{ }} expressions that collided with Go text/template delimiters, causing a parse error on line 28 during `moai update`. Additionally, the update pipeline cleaned managed files BEFORE template rendering, leaving the project in a broken state if rendering failed. Changes: - Fix template escaping in llm-panel.yml.tmpl using proper Go template string literals ({{ "{{" }} and {{ "}}" }}) - Replace multi-line Liquid-style conditionals with JavaScript ternary operators compatible with actions/github-script - Add Deployer.ValidateAll() method for pre-flight template validation - Add "Validate Templates" step to update pipeline BEFORE clean step - Update mock deployers in initializer_test.go for new interface method - Add reproduction test (llm_panel_test.go), integration test, and transactional deploy test (deployer_transactional_test.go) Fixes #733 🗿 MoAI <email@mo.ai.kr>
WalkthroughThis PR fixes issue Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/template/templates/.github/workflows/llm-panel.yml.tmpl (1)
19-19:⚠️ Potential issue | 🔴 CriticalTriple closing brace renders an extra
}into the workflow.Tracing the Go template render of
${{ "{{" }} secrets.GITHUB_TOKEN }}}:
${{ "{{" }}→${{secrets.GITHUB_TOKEN }}}is literal text →secrets.GITHUB_TOKEN }}}Final rendered YAML:
github-token: ${{ secrets.GITHUB_TOKEN }}}— three closing braces. GitHub Actions will evaluate${{ secrets.GITHUB_TOKEN }}and concatenate a literal}to the token value, breakingactions/github-script@v7authentication.The PR objective explicitly calls for auditing all
.yml.tmplescapes; line 22 already uses the correct two-brace form (}}), and the ternary lines 28–30 use the consistent{{ "}}" }}escape. Line 19 should match.🐛 Proposed fix (use the same escape pattern as lines 28–30)
- github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}} + github-token: ${{ "{{" }} secrets.GITHUB_TOKEN {{ "}}" }}Or simply drop one brace:
- github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}} + github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/template/templates/.github/workflows/llm-panel.yml.tmpl` at line 19, The GitHub Actions token template emits an extra closing brace due to the `${{ "{{" }} secrets.GITHUB_TOKEN }}}` escape; fix the github-token line in internal/template/templates/.github/workflows/llm-panel.yml.tmpl by replacing the triple closing brace escape with the correct two-brace escape (match the `{{ "}}" }}` pattern used on lines 28–30) so the rendered value becomes `${{ secrets.GITHUB_TOKEN }}` and does not append an extra `}`; update the github-token entry (search for the `github-token` key or the `${{ "{{" }} secrets.GITHUB_TOKEN }}}` fragment) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/update.go`:
- Around line 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.
In `@internal/template/deployer_mode.go`:
- Around line 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.
In `@internal/template/deployer_transactional_test.go`:
- Around line 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.
In `@internal/template/deployer.go`:
- Around line 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.
In `@internal/template/llm_panel_test.go`:
- Line 86: The "fixed" subtest in internal/template/llm_panel_test.go currently
allows the bad three-brace token because it only checks that the rendered text
contains `${{ secrets.GITHUB_TOKEN }}` as a substring; update the test to both
(1) fix the template file so the token is `${{ secrets.GITHUB_TOKEN }}` (no
extra `}`) and (2) tighten the assertions that reference the GitHub token (the
assertions that currently look for `${{ secrets.GITHUB_TOKEN }}` in the "fixed"
subtest and the sibling assertion) to verify exact presence and absence: assert
the rendered output contains the exact `${{ secrets.GITHUB_TOKEN }}` sequence
and assert that it does NOT contain `${{ secrets.GITHUB_TOKEN }}}` (or simply
assert exact equality for the whole line if the test builds the full expected
snippet) so the triple-`}` regression will fail the test.
- Around line 132-134: The custom recursive contains function is inefficient and
risky; replace the function contains(s, substr string) with calls to the
standard library strings.Contains and remove the hand-rolled implementation:
delete the contains function definition and update any calls like
contains(output, ...) to strings.Contains(output, ...), and add the "strings"
import to the test file's import block so tests compile.
---
Outside diff comments:
In `@internal/template/templates/.github/workflows/llm-panel.yml.tmpl`:
- Line 19: The GitHub Actions token template emits an extra closing brace due to
the `${{ "{{" }} secrets.GITHUB_TOKEN }}}` escape; fix the github-token line in
internal/template/templates/.github/workflows/llm-panel.yml.tmpl by replacing
the triple closing brace escape with the correct two-brace escape (match the `{{
"}}" }}` pattern used on lines 28–30) so the rendered value becomes `${{
secrets.GITHUB_TOKEN }}` and does not append an extra `}`; update the
github-token entry (search for the `github-token` key or the `${{ "{{" }}
secrets.GITHUB_TOKEN }}}` fragment) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0521ac8f-56ca-4477-83b0-0ba294d0a89c
📒 Files selected for processing (8)
internal/cli/update.gointernal/core/project/initializer_test.gointernal/template/deployer.gointernal/template/deployer_mode.gointernal/template/deployer_transactional_test.gointernal/template/llm_panel_integration_test.gointernal/template/llm_panel_test.gointernal/template/templates/.github/workflows/llm-panel.yml.tmpl
| { | ||
| 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 | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧹 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.
| // 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 | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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()) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
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.
| 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.
| 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] | ||
| } |
There was a problem hiding this comment.
🧹 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 -5Repository: 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.goRepository: 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.goRepository: 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.
| - name: Post LLM Panel Comment | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}} |
There was a problem hiding this comment.
Test passes despite the same }}} bug flagged in llm-panel.yml.tmpl.
Line 86 mirrors the production template's three-brace defect, and the substring-based assertions on lines 123 and 126 happily match ${{ secrets.GITHUB_TOKEN }} as a prefix of the actually-rendered ${{ secrets.GITHUB_TOKEN }}}. The "fixed" subtest therefore reports green while the rendered YAML is still wrong.
Once the template is fixed, please tighten the assertions so a regression doesn't slip through:
🐛 Proposed fix
- github-token: ${{ "{{" }} secrets.GITHUB_TOKEN }}}
+ github-token: ${{ "{{" }} secrets.GITHUB_TOKEN {{ "}}" }}- if !contains(output, "${{ secrets.GITHUB_TOKEN }}") {
- t.Error("output should contain escaped GitHub Actions token reference")
- }
+ if !strings.Contains(output, "${{ secrets.GITHUB_TOKEN }} ") {
+ t.Errorf("output should contain exactly two closing braces around secrets.GITHUB_TOKEN; got:\n%s", output)
+ }
+ if strings.Contains(output, "${{ secrets.GITHUB_TOKEN }}}") {
+ t.Errorf("output contains stray trailing brace after secrets.GITHUB_TOKEN; got:\n%s", output)
+ }Also applies to: 123-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/template/llm_panel_test.go` at line 86, The "fixed" subtest in
internal/template/llm_panel_test.go currently allows the bad three-brace token
because it only checks that the rendered text contains `${{ secrets.GITHUB_TOKEN
}}` as a substring; update the test to both (1) fix the template file so the
token is `${{ secrets.GITHUB_TOKEN }}` (no extra `}`) and (2) tighten the
assertions that reference the GitHub token (the assertions that currently look
for `${{ secrets.GITHUB_TOKEN }}` in the "fixed" subtest and the sibling
assertion) to verify exact presence and absence: assert the rendered output
contains the exact `${{ secrets.GITHUB_TOKEN }}` sequence and assert that it
does NOT contain `${{ secrets.GITHUB_TOKEN }}}` (or simply assert exact equality
for the whole line if the test builds the full expected snippet) so the
triple-`}` regression will fail the test.
| 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))) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace the hand-rolled recursive contains with strings.Contains.
The recursion does contains(s[1:], substr) on every miss, which is O(n·m) time and — more importantly — O(n) stack depth. For a multi-KB rendered template this risks both poor performance and stack overflow. The standard library already does this correctly.
♻️ Proposed refactor
-import (
- "testing"
- "testing/fstest"
-)
+import (
+ "strings"
+ "testing"
+ "testing/fstest"
+)-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)))
-}
+// contains is replaced by strings.Contains at all call sites.Replace contains(output, …) with strings.Contains(output, …).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/template/llm_panel_test.go` around lines 132 - 134, The custom
recursive contains function is inefficient and risky; replace the function
contains(s, substr string) with calls to the standard library strings.Contains
and remove the hand-rolled implementation: delete the contains function
definition and update any calls like contains(output, ...) to
strings.Contains(output, ...), and add the "strings" import to the test file's
import block so tests compile.
|
@claude CI 빌드가 실패했습니다. 오류를 분석하고 수정해주세요. 실패한 워크플로우: CI 실패 로그를 확인하고 근본 원인을 파악한 뒤 수정해주세요. Latte AI CI Monitor • latte@mo.ai.kr |
1 participant
Summary
${{ }}GitHub Actions expressions inllm-panel.yml.tmplthat collided with Gotext/templatedelimiters, causing parse failure onmoai updateValidateAll()pre-flight step to update pipeline, ensuring templates are validated BEFORE managed files are cleaned — prevents broken project state on parse errorsChanges
Template Fix
internal/template/templates/.github/workflows/llm-panel.yml.tmpl: Properly escape${{ }}using Go template string literals ({{ "{{" }}and{{ "}}" }}). Replaced broken multi-line Liquid-style conditionals with JavaScript ternary operators.Transactional Deploy
internal/template/deployer.go: AddedValidateAll(ctx, tmplCtx)method — renders all templates in memory without writing filesinternal/template/deployer_mode.go: AddedValidateAlltomodeAwareDeployerinternal/cli/update.go: Added "Validate Templates" step BEFORE "Clean Managed Paths" stepTests
internal/template/llm_panel_test.go: Reproduction test for issue moai update 2.18.0: embedded template.github/workflows/llm-panel.yml.tmpl:28parse failure — unexpected "}" #733 (broken vs fixed template)internal/template/llm_panel_integration_test.go: Integration test with embedded templateinternal/template/deployer_transactional_test.go: Transactional validation behavior testsinternal/core/project/initializer_test.go: Updated mock deployers for new interface methodTest plan
go test ./internal/template/...— all passgo test ./internal/core/project/...— all passgo build ./...— clean buildTestRunInit/default_execution) unrelated — fails with/without changesmake build && moai update— manual verification (requires binary rebuild)Fixes #733
🗿 MoAI email@mo.ai.kr
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes