fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #98
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#98hyperpolymath wants to merge 1 commit into
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. (5)
|
| Layer / File(s) | Summary |
|---|---|
Pass file paths to replacement shell tests/e2e/template_instantiation_test.sh |
The placeholder replacement command now supplies the current file path with _ {} while retaining the existing replacement loop and error handling. |
Estimated code review effort: 1 (Trivial) | ~2 minutes
Merge Risk: ⚪ Minimal · up to aa85e
This is a localized test-script correction that restores the intended placeholder replacement behavior; no actionable merge-blocking risk remains after normal checks and review.
Suggested reviewers: joshuajewell
Poem
A rabbit checks each file in line
The paths now pass through shells just fine
Placeholders hop from place to place
The test runs on with steady pace
One small fix, neat and bright
All burrows build tonight
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the defect, impact, and correction. It does not follow the repository template because it omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots … | Update the description to use the repository template. Add the required section headings, list the changes, mark applicable checklist items, and document the tests that were run. State when screenshots or terminal output are not applicable. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the find -exec fix and the placeholder replacement failure. It is concise and specific. |
| 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, impact, and correction. 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
Although the syntax for the find -exec command has been corrected and the repository is currently marked as 'up to standards' by Codacy, the implementation remains a no-op.
The core issue is that the shell variables $placeholder and $value are contained within a single-quoted string passed to sh -c, which prevents the parent shell from expanding them. Furthermore, the filename passed via {} is not correctly mapped to the $file variable used in the sed command. This PR should not be merged in its current state as it fails to achieve its stated intent of enabling placeholder replacement.
About this PR
- The fix addresses the syntax of the
findcommand but fails to resolve the underlying logic issue. Variables within the subshell execution are treated as literals, meaning no replacement occurs in the e2e tests.
Test suggestions
- Execute template instantiation and verify that placeholders (e.g., {{project}}) are actually replaced in the output files.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Execute template instantiation and verify that placeholders (e.g., {{project}}) are actually replaced in the output files.
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
While adding \; and {} fixes the find syntax, the command remains a no-op. The variables $placeholder and $value are not expanded by the parent shell because they are enclosed in a single-quoted string. Furthermore, the filename {} is passed to the subshell as a positional parameter, but the script tries to reference it as $file, which is undefined.
To fix this, pass the variables as arguments to the subshell:
find ... -exec sh -c 'placeholder="$1"; value="$2"; file="$3"; sed -i "s|$placeholder|$value|g" "$file"' _ "$placeholder" "$value" {} \;Alternatively, use {} + for better performance if processing many files.
1 participant
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.