fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #64
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🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test exports placeholder values for the nested Bash process and passes each matched file directly to the replacement command. ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized test-script fix corrects the file-replacement command without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the defect, its impact, and the correction. However, it does not follow the repository template because it omits the required section headings, the RSR Quality Checklist, and a dedicated Testing section. Resolution Restructure the description using the repository template. Add Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections as applicable. Mark the checklist items accurately and state which tests or validation commands were run.
✨ Finishing Touches📝 Generate docstrings
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}
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
While the syntax errors in the find -exec command have been addressed, the PR fails to achieve its functional goal. The subshell script uses a single-quoted string, which prevents it from accessing the $file variable from the parent shell. Consequently, the sed command operates on an empty filename, and the placeholder replacement remains a no-op. Despite Codacy indicating the PR is 'up to standards', this logic error must be fixed to ensure the test suite actually validates template instantiation.
Test suggestions
- Verify that 'template_instantiation_test.sh' correctly replaces placeholders in a generated template structure.
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 find -exec syntax is now correct, but the subshell logic remains broken. The subshell receives the file path from find as $1, but the script uses "$file". Since the subshell script is single-quoted, it does not have access to the outer shell's $file variable. To fix this, use "$1" directly in the sed command or assign file="$1" at the start of the command string (e.g., sh -c 'file="$1"; sed -i ...').
|
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.