fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #54
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#54hyperpolymath 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. (24)
|
| Layer / File(s) | Summary |
|---|---|
Pass matched paths to the replacement script tests/e2e/template_instantiation_test.sh |
The find -exec command now uses {} to provide each matched file path to the inline bash -c script. |
Estimated code review effort: 1 (Trivial) | ~3 minutes
Merge Risk: ⚪ Minimal · up to c9f5f
This localized test-script fix restores the intended placeholder replacement behavior without changing production code or runtime configuration; no actionable merge-blocking risk remains after normal checks and review.
Poem
A rabbit checks each template file,
With paths now passed in proper style.
The script receives each name it finds,
And replaces text through matching lines.
One small fix, the test runs bright.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the defect, its impact, and the correction. However, it does not use the required template sections and omits the RSR Quality Checklist, Testing section, and Screenshots secti… | Reformat the description with the required ## Summary, ## Changes, ## RSR Quality Checklist, ## Testing, and ## Screenshots sections. Complete the applicable checklist items and state how the change was tested. Include screenshots… |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the test fix: it terminates find -exec and passes {} to the placeholder step. |
| Docstring Coverage | ✅ Passed | 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… |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Full details: Description check
Explanation
The description explains the defect, its impact, and the correction. However, it does not use the required template sections and omits the RSR Quality Checklist, Testing section, and Screenshots section.
Resolution
Reformat the description with the required ## Summary, ## Changes, ## RSR Quality Checklist, ## Testing, and ## Screenshots sections. Complete the applicable checklist items and state how the change was tested. Include screenshots or terminal output if applicable, and add a related issue link if one exists.
Full details: Docstring Coverage
Explanation
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.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
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 help to get the list of available commands.
![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 addresses the syntax errors in the find -exec command by adding the required terminator and placeholder, the implementation remains functionally broken. The subshell executed by find does not correctly reference the matched file path, meaning the template substitution logic will likely still fail to execute as intended.
Codacy reports that the code is up to standards, but the logic gap identified in the sh -c block is a high-risk issue that prevents the test from actually verifying template instantiation. Furthermore, several test variables are defined but not used, suggesting the test coverage for specific placeholders is incomplete.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 22⚪ LOW RISK
The variablesTEST_FORGE,TEST_OWNER,TEST_AUTHOR_EMAIL,TEST_DESCRIPTION, andTEST_PRIMARY_LANGUAGEare assigned but never referenced. If these are intended to be part of the template substitution, ensure they are added to thePLACEHOLDERSassociative array. Otherwise, they should be removed to clean up the test script.
Test suggestions
- Verify that the template instantiation test correctly modifies files by replacing placeholders when executed via find -exec.
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 find -exec syntax is now syntactically correct, but the logic within the subshell is flawed. The filename passed by find via {} is available as $1 inside the subshell, but the script uses "$file". Since the sh -c block is single-quoted, it does not inherit the outer shell's $file variable. Unless you explicitly assign file="$1" at the start of the block, the sed command will operate on an empty string.
Try updating the sh -c block to: sh -c 'file="$1"; sed ...' _ {} \;
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.