fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #48
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#48hyperpolymath 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. (12)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test now passes each discovered file to its embedded Bash replacement script and uses the correct ChangesTemplate instantiation testing
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized test-script fix with no actionable merge-blocking risk remaining 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 explains the defect, impact, and intended fix, but it does not follow the required template. It omits the required section headings, checklist status, testing details, and applicable screenshots or terminal output. Full details: Docstring CoverageExplanation 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.
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 this PR resolves the immediate syntax error that rendered the find command a no-op, the template substitution logic remains non-functional. Findings indicate that shell variables are not correctly expanded inside the subshell, meaning the E2E test likely still fails to perform actual replacements. Additionally, there are maintenance concerns regarding the duplication of this script across multiple repositories and unused configuration variables.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21🟡 MEDIUM RISK
The variable TEST_OWNER is defined but never referenced. If it is intended for template configuration, ensure it is utilized in the substitution logic; otherwise, it should be removed.
Test suggestions
- Verify that template placeholders (e.g., {{project}}) are actually replaced in the generated files during the E2E test.
Low confidence findings
- This file is a stale duplicate of a script existing in 30 other repositories. Relying on manual updates in this repository rather than a centralized template upstream creates a maintenance burden and risk of configuration drift.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| sed -i "s|$placeholder|$value|g" "$file" | ||
| fi | ||
| done | ||
| ' _ "$file" | ||
| ' _ {} \; |
There was a problem hiding this comment.
🔴 HIGH RISK
The substitution logic is still a no-op. Because the subshell string is single-quoted, the parent shell's $placeholder and $value variables are not expanded. Within the subshell, you must use "$1" to refer to the file path passed by find, as $file is undefined. Recommendation: Pass the variables as arguments to the subshell (e.g., sh -c '...' _ "$placeholder" "$value" "{}") or use double quotes with appropriate escaping. Additionally, using '+' instead of ';' would be more efficient for large file sets.
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.