fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #32
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#32hyperpolymath 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. (2)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test now passes each matched file path to its inline Bash replacement script instead of passing the literal ChangesTemplate instantiation testing
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The test script now passes matched file paths to the placeholder replacement command, restoring the intended test behavior. No actionable merge-blocking risk remains beyond 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 defects, impact, and correction in detail, but it does not follow the repository template. It omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections. Resolution Restructure the description using the repository template. Add the required headings, complete the applicable checklist items, and state the tests or validation performed. Include terminal output or state that screenshots are not applicable. 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 addresses the syntax termination of the find -exec command, the implementation remains functionally a no-op. Cross-correlation between the Intent and Code Review agents reveals a significant logic gap: the filename is passed to the subshell but not correctly referenced, and the variables intended for sed substitution are not available in the subshell's environment.
These issues should prevent merging as the test suite will continue to pass without actually verifying template instantiation. Although Codacy reports the PR is up to standards, the functional findings indicate that the acceptance criteria for placeholder replacement are not yet met.
About this PR
- The changes introduce a pattern where subshell logic relies on parent-scope variables and positional arguments that are not correctly mapped or exported. This results in the script running without error but performing no work.
6 comments outside of the diff
tests/e2e/template_instantiation_test.sh
line 21🟡 MEDIUM RISK
The variable is assigned but never used. If it is intended for template substitution within the find -exec subshell, it must be exported to be visible to that process.export TEST_OWNER="test-owner"
line 22🟡 MEDIUM RISK
The variable is assigned but never used. If it is intended for template substitution within the find -exec subshell, it must be exported to be visible to that process.export TEST_FORGE="github"
line 24🟡 MEDIUM RISK
The variable is assigned but never used. If it is intended for template substitution within the find -exec subshell, it must be exported to be visible to that process.export TEST_AUTHOR_EMAIL="test@example.com"
line 26🟡 MEDIUM RISK
The variable is assigned but never used. If it is intended for template substitution within the find -exec subshell, it must be exported to be visible to that process.export TEST_DESCRIPTION="A test project instantiated from the RSR template"
line 27🟡 MEDIUM RISK
The variable is assigned but never used. If it is intended for template substitution within the find -exec subshell, it must be exported to be visible to that process.export TEST_PRIMARY_LANGUAGE="Rust"
line 32⚪ LOW RISK
Nitpick: This styling variable appears unused and can be removed to clean up the script.
Test suggestions
- find -exec correctly iterates over files and executes the bash block with valid arguments
- Placeholder tokens in the template are replaced via sed on the files identified by find
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 subshell script references "$file", but the filename found by find is passed as the first positional argument ($1). Because the command is in a single-quoted string, you must either assign the variable inside the subshell (e.g., sh -c 'file="$1"; ...') or replace "$file" with "$1" throughout the script block.
Try running the following prompt in your coding agent:
In
tests/e2e/template_instantiation_test.sh, update thefind -execshell command to assign the first argument to thefilevariable by addingfile="$1";to the start of thesh -cstring.
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.