fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #77
Conversation
…as a no-op
tests/e2e/template_instantiation_test.sh ran:
find ... -exec bash -c '
file="$1"
... grep/sed over $file ...
' _ "$file"
Two defects in that one line:
1. No ';' or '+' terminator, so the file does not parse (SC2067).
2. "$file" is passed where {} belongs. $file is assigned ONLY inside the
-exec body, so in the outer scope it is UNSET — $1 arrived empty, file=""
and every grep/sed operated on an empty path.
⚠ The consequence is worse than a lint error: the placeholder-replacement step
SILENTLY DID NOTHING, then logged "All placeholder tokens replaced". A test
whose whole purpose is to prove instantiation worked was passing without
replacing a single token. That is a plausible cause of estate repos shipping
with literal {{project}} tokens still in their sources.
Corrected to "' _ {} \;" so find passes each matched path.
Found by an estate-wide shellcheck sweep of 5,111 scripts across 375 repos:
this identical stale copy exists in 30 repositories. rsr-template-repo's own
copy is already correct and restructured (371 lines vs the 268 here), so these
are stale duplicates that never picked up the upstream fix.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (18)
|
| Layer / File(s) | Summary |
|---|---|
Pass matched paths to Bash tests/e2e/template_instantiation_test.sh |
The find -exec command uses {} to provide each matched pathname as the embedded Bash script’s file argument. |
Estimated code review effort: 1 (Trivial) | ~2 minutes
Merge Risk: ⚪ Minimal · up to dc536
This localized test change fixes the placeholder replacement command, and no actionable merge-blocking risk remains after normal checks and review.
Poem
A rabbit checks each file in line
The path now lands in Bash just fine
findsends the name along
The test runs where it belongs
One small fix makes the trail align
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the defect, its impact, and the correction. However, it does not follow the repository template because it omits the required Summary, Changes, RSR Quality Checklist, Testing,… | Restructure the description to use the repository template. Add the Summary, Changes, RSR Quality Checklist, and Testing sections, and complete the applicable checklist items. Add Screenshots or terminal output if applicable. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the test fix and the corrected find -exec behaviour. It is specific and related to the main change. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Full details: Description check
Explanation
The description explains the defect, its impact, and the correction. However, it does not follow the repository template because it omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections.
Full details: Docstring Coverage
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
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 help to get the list of available commands.
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull Request Overview
While this PR correctly identifies and fixes the syntax error in the find -exec command (adding the terminator and placeholder), the implementation of the subshell script introduces new logic errors that prevent the placeholders from being replaced. Specifically, the script references an uninitialized variable $file and relies on parent-shell variables within a single-quoted string where they cannot expand. Although Codacy reports the PR is up to standards, these logic flaws represent a significant gap in the acceptance criteria. The test likely still fails to perform actual template instantiation.
About this PR
- The pattern of using
sh -c '...'with single quotes prevents the expansion of variables like$placeholderand$valuefrom the parent shell. These must be either exported or passed as additional positional arguments to the subshell to ensure thesedcommand behaves as expected.
Test suggestions
- Template instantiation test correctly identifies files and replaces placeholders
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| fi | ||
| done | ||
| ' _ "$file" | ||
| ' _ {} \; |
There was a problem hiding this comment.
🔴 HIGH RISK
The sh -c command string uses the variable "$file", but the filename from find is passed as a positional argument ($1). Inside the subshell, "$file" will be empty. Additionally, because the script block is single-quoted, shell variables like $placeholder and $value will not expand.
Update the command to assign the file path (e.g., file="$1") and ensure all variables are correctly scoped or passed into the subshell. Try this prompt in your IDE agent:
In
tests/e2e/template_instantiation_test.sh, update thefind -execblock to correctly assignfile="$1"and ensure$placeholderand$valueare accessible inside the subshell script.
tests/e2e/template_instantiation_test.shranfind … -exec bash -c '…' _ "\$file", which has two defects on one line:;or+terminator — the file does not parse (SC2067)."\$file"where{}belongs —\$fileis assigned only inside the-execbody, so in the outer scope it is unset.\$1arrived empty,file="", and everygrep/sedoperated on an empty path.⚠ The consequence is worse than a lint error. The placeholder-replacement step silently did nothing, then logged "All placeholder tokens replaced". A test whose entire purpose is to prove instantiation worked was passing without replacing a single token — a plausible cause of estate repos shipping with literal
{{project}}still in their sources.Corrected to
' _ {} \;sofindpasses each matched path.Found by an estate-wide sweep of 5,111 scripts across 375 repos: this identical stale copy exists in 30 repositories.
rsr-template-repo's own copy is already correct and restructured (371 lines vs the 268 here), so these are stale duplicates that never picked up the upstream fix.