fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #192
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#192hyperpolymath wants to merge 2 commits 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.
|
|
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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. (3)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe template instantiation end-to-end test now passes each matched file to the inline shell script as ChangesTemplate instantiation testing
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This corrects the test’s file replacement command so matched paths are passed and processed; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. ✨ 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
Although this PR correctly fixes the find -exec termination syntax to resolve the immediate shell error, the resulting implementation remains non-functional (a 'no-op') due to how variables are handled in the subshell. Specifically, shell variables $placeholder and $value will not expand inside the single-quoted subshell, and the file path passed via {} is not correctly mapped to the $file variable.
Furthermore, the sed command lacks portability for macOS/BSD environments and does not safely handle special characters like | or &. While Codacy indicates the PR is 'up to standards' based on static analysis, the functional logic errors identified in the review represent a high risk of failure in the test suite. Additionally, the file is noted to be significantly behind its upstream reference, which may lead to further divergence issues.
Test suggestions
- Verify
find -execcorrectly iterates over template files and executes the subshell logic. - Verify the bash subshell correctly maps the passed
{}path to the variable used insedcommands.
Low confidence findings
- This script is currently a 'stale copy' (268 lines) compared to the upstream reference (371 lines). While this PR fixes a syntax bug, the divergence suggests the file may require a full synchronization to prevent missing other critical logic or modern standards present in the upstream version.
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 find syntax is now valid, but the subshell logic (lines 137-140) remains broken for several reasons:
- Variable Scoping: Because the
sh -ccommand is wrapped in single quotes,$placeholderand$valueare not expanded by the parent shell and will be empty in the subshell. - Path Mapping: The path passed via
{}becomes$1in the subshell, but the script attempts to use$file, which is undefined. - Sed Portability & Safety: The
sed -icommand is not portable to macOS/BSD (which requiressed -i '') and will fail if$valuecontains delimiters like|or the special character&.
Recommendation: Refactor the command to pass variables as positional arguments:
find ... -exec sh -c 'p="$1"; v="$2"; shift 2; for f in "$@"; do sed -i "s|$p|$v|g" "$f"; done' _ "$placeholder" "$value" {} +
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.