fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #74
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#74hyperpolymath 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. (1)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe template instantiation end-to-end test now passes each matched file to the placeholder replacement command and correctly terminates the ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized test-script fix is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Full details: Description checkExplanation The description clearly explains the defect, impact, correction, and discovery context. However, it does not follow the repository template because it omits the
✨ 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 this PR correctly identifies and fixes the syntax error preventing the 'find -exec' command from executing, the current implementation remains non-functional due to subshell scope limitations.
There are two primary concerns: the subshell cannot access the local 'TEST_*' variables defined in the parent script, and the filename passed via '{}' is not assigned to the '$file' variable used in the command string. These issues result in a continued 'no-op' state where no placeholders are actually replaced. These logic errors should be addressed to fulfill the intent of the fix.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21-32🟡 MEDIUM RISK
Suggestion: The variablesTEST_OWNER,TEST_FORGE,TEST_AUTHOR_EMAIL,TEST_DESCRIPTION,TEST_PRIMARY_LANGUAGE, andYELLOWare assigned but never used. Since this PR fixes thefind -execsub-shell logic used for placeholder replacement, ensure these variables are exported or passed as arguments if they are intended to be accessed within that sub-shell.Suggested prompt for your agent:
Review the script to see if the unused TEST_* variables should be exported for use in the find -exec sub-shell or passed as arguments to ensure placeholder replacement works correctly.
Test suggestions
- Verify that placeholders (e.g., {{project}}) are actually replaced in generated files during template instantiation.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| @@ -137,7 +137,7 @@ find "$TEST_REPO_PATH" -type f \ | |||
| sed -i "s|$placeholder|$value|g" "$file" | |||
There was a problem hiding this comment.
🔴 HIGH RISK
The subshell script refers to "$file" which is likely unset in this scope. Since find passes the filename as the first positional parameter, you should assign file="$1" at the start of the sh -c block or use "$1" directly.
Suggested prompt for your agent:
In tests/e2e/template_instantiation_test.sh, find the find ... -exec sh -c block and ensure that file="$1" is assigned at the beginning of the subshell script so that subsequent sed and grep commands use the correct filename matched by find.
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.