ADFA-3689: Terminate project creation on Pebble template errors#1553
Merged
Conversation
Mechanical commit, no functional change. The file predates the Spotless ratchet (space-indented), so touching it forces a full reformat; done separately to keep the behavioral change reviewable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pebble parse, evaluation, and file write/copy errors previously logged and continued, producing a partially rendered project that was reported as created successfully; the leftover directory also made retries return the broken project as success. - Introduce TemplateExecutionException; parse/evaluate/write/copy failures now abort execution instead of skipping the file - Delete the partial project directory on failure so a retry does not hit the projectDir.exists() early-return - Surface a user-facing error (failing file, line, cause, and next steps) through the existing ProjectCreationManager error path - Keep identifier-default warnings non-fatal (post-open notice) - Add ZipRecipeExecutorTest covering parse error, evaluation error, success path, and the existing-directory guard Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
Risks / best-practice considerations
Walkthrough
ChangesTemplate execution lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ZipRecipeExecutor
participant Pebble
participant ProjectDirectory
ZipRecipeExecutor->>Pebble: render template entry
Pebble-->>ZipRecipeExecutor: rendering failure
ZipRecipeExecutor->>ProjectDirectory: delete partially created project
ZipRecipeExecutor-->>ZipRecipeExecutor: throw TemplateExecutionException
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt (1)
546-549: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNon-fatal extension-load failure can surface later as a misleading fatal error.
loadExtensionFromArchivelogs and returnsemptyList()on failure. If a template depends on functions/filters provided by that extension, rendering continues without it and (withstrictVariables(true)) later fails ingetTemplate/evaluate, producing a fatal parse/evaluation error that doesn't mention the real cause (the extension failed to load). Consider whether extension-load failure should also be fatal, or at least reflected in the eventual error message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt` around lines 546 - 549, Update loadExtensionFromArchive so extraction failures do not silently return emptyList() when the extension is required for template rendering. Propagate the original exception or record it in a failure state consumed by getTemplate/evaluate, ensuring the eventual error identifies the extension-load failure instead of only reporting a misleading parse or evaluation error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 66-79: Handle failures from keystore(executor) separately from the
fatal renderProject(ctx, projectDir) path in the surrounding execution flow.
Preserve cleanup and TemplateExecutionException behavior for rendering failures,
but catch keystore copy/property errors as non-fatal, issue the existing
warning, and set hasErrorsWarnings so project creation still succeeds without
release keystore artifacts.
- Around line 559-563: Validate templateRef.path/basePath before constructing
optimizedDir, preventing absolute paths, traversal segments, or separators from
escaping Environment.TEMPLATES_DIR. Reuse the canonical containment-check
approach already applied to outFile, and reject invalid paths before any
filesystem access.
---
Nitpick comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 546-549: Update loadExtensionFromArchive so extraction failures do
not silently return emptyList() when the extension is required for template
rendering. Propagate the original exception or record it in a failure state
consumed by getTemplate/evaluate, ensuring the eventual error identifies the
extension-load failure instead of only reporting a misleading parse or
evaluation error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0480472a-df1e-4194-99e2-50719455ce8f
📒 Files selected for processing (4)
resources/src/main/res/values-in-rID/strings.xmlresources/src/main/res/values/strings.xmltemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kttemplates-impl/src/test/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutorTest.kt
Review feedback (CodeRabbit): the release keystore is auxiliary, so a copy failure now warns and sets hasErrorsWarnings instead of deleting the fully rendered project. Test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt (1)
66-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch narrowly and rethrow
CancellationException.Broad exception catches that do not rethrow
CancellationExceptionswallow coroutine cancellation, breaking the coroutine hierarchy. Based on learnings and as per coding guidelines, prefer narrow exception handling and explicitly rethrowCancellationException.
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt#L66-L78: Insertif (e is CancellationException) throw eat the beginning of the catch block.templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt#L80-L86: Change the broadcatch (e: Exception)tocatch (e: IOException)sincekeystore()only performs file operations.templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt#L530-L537: Update thewrapextension to returnException(rather thanTemplateExecutionExceptionexplicitly) and addif (this is CancellationException) return thisso allthrow e.wrap(...)call sites automatically rethrow cancellations without needing individual modifications.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt` around lines 66 - 78, Preserve coroutine cancellation and narrow exception handling in ZipRecipeExecutor.kt: at lines 66-78, immediately rethrow CancellationException before cleanup or wrapping; at lines 80-86, change the keystore() catch from Exception to IOException; at lines 530-537, make wrap return Exception and return CancellationException unchanged so all throw e.wrap(...) call sites propagate cancellation automatically.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 66-78: Preserve coroutine cancellation and narrow exception
handling in ZipRecipeExecutor.kt: at lines 66-78, immediately rethrow
CancellationException before cleanup or wrapping; at lines 80-86, change the
keystore() catch from Exception to IOException; at lines 530-537, make wrap
return Exception and return CancellationException unchanged so all throw
e.wrap(...) call sites propagate cancellation automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79462c3e-944a-4ee9-95f1-21bdadae70ea
📒 Files selected for processing (2)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kttemplates-impl/src/test/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutorTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- templates-impl/src/test/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutorTest.kt
Review feedback (CodeRabbit): basePath comes from the archive's templates.json, so a crafted path could escape TEMPLATES_DIR via the dex-opt directory. Apply the same canonical containment check used for outFile and skip extension loading on violation. Test added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jatezzz
requested changes
Jul 20, 2026
Review feedback: extract processEntry / renderTemplateEntry / copyBinaryEntry from the entry loop. No behavior change; the loop body now reads skip checks -> path containment -> dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jatezzz
approved these changes
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes ADFA-3689: Pebble template errors during project creation previously flooded the log and let creation "succeed" with a partially rendered project. They now terminate creation with meaningful feedback.
TemplateExecutionExceptioninstead of logging and skipping the file. The blanket per-entry catch that swallowed parse errors now rethrows.projectDir.exists()early-return reported the broken leftover as a successful creation on the next attempt.template_exec_error_terminatedstring, English + Indonesian), delivered through the existingProjectCreationManager->flashErrorpath instead of "Project created successfully".Review by commit
Testing
ZipRecipeExecutorTest(Robolectric, real zips built on the fly): parse error terminates + removes project dir; evaluation error ditto; valid template renders; existing project dir untouched.:templates-impl:testV8DebugUnitTestandspotlessCheckgreen.🤖 Generated with Claude Code