fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #67
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. (10)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template test now passes each matched file from ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized test-script correction, and no actionable merge-blocking risk remains 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 consequence, and the correction. However, it does not use the required template sections and does not provide the required checklist status or testing details. Resolution Update the description to include the 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📝 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
The PR correctly identifies a syntax error in the 'find' command terminator, but the current implementation contains logic flaws that will prevent the template instantiation from working. Specifically, using single quotes for the subshell prevents the expansion of $placeholder and $value, and the configuration variables are not exported to the subshell's environment. While Codacy reports that the code is up to standards, these functional gaps will result in a no-op or failure during actual execution.
Key issues to address before merging:
- Fix shell variable expansion inside the 'find -exec' subshell.
- Export or pass configuration variables to the subshell to ensure template tokens are replaced.
About this PR
- Systemic issue with subshell environment: Several configuration variables (TEST_OWNER, TEST_FORGE, etc.) are defined but not available to the logic inside the 'find -exec' subshell. Consider a more robust way to pass environment context to subshells.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21-32🟡 MEDIUM RISK
These configuration variables appear unused because they are not used in the parent shell scope. If they are intended to be used inside the 'find -exec sh -c' subshell to perform template replacements, they must be exported to the environment.Try running the following prompt in your IDE agent:
Check if TEST_OWNER, TEST_FORGE, TEST_AUTHOR_EMAIL, TEST_DESCRIPTION, TEST_PRIMARY_LANGUAGE, and YELLOW are used within the find -exec subshell. If so, add 'export' before their definitions at the top of the script so they are available to the subshell.
Test suggestions
- Verify that 'template_instantiation_test.sh' successfully replaces template tokens (e.g., {{project}}) in generated source files.
- Verify that the 'find' command correctly iterates over files and does not exit with a syntax error (SC2067).
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 variables $placeholder and $value are inside a single-quoted string, meaning the parent shell will not expand them. Additionally, using -exec ... {} \; runs the command once per file, making the internal for file in "$@" loop redundant.
Try running the following prompt in your coding agent:
In
tests/e2e/template_instantiation_test.sh, modify thefindcommand to pass$placeholderand$valueas the first two arguments to thesh -cscript,shift 2inside the script to clear them from the positional parameters, and change the terminator to{} +so theforloop can efficiently process all 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.