fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #72
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#72hyperpolymath 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. (23)
|
| Layer / File(s) | Summary |
|---|---|
Pass matched file paths to replacement command tests/e2e/template_instantiation_test.sh |
The find -exec bash -c command now uses {} for each discovered file path instead of $file. |
Estimated code review effort: 1 (Trivial) | ~2 minutes
Merge Risk: ⚪ Minimal · up to 76c60
This is a narrowly scoped test-script correction, and no actionable merge-blocking risk remains beyond normal checks and review.
Poem
A rabbit checked the file path with care
{}now carries it there
No empty shell variable remains
The test hops through its runs again
Clean paths make happy ears
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the defect, impact, and fix in detail, but it does not use the required Summary, Changes, RSR Quality Checklist, or Testing sections. It also provides no completed checklist o… | Rewrite the description using the repository template. Add Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections as applicable. Record the test, formatting, lint, licence, and other relevant check results explicitly. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the test fix and the corrected find -exec argument handling. It is concise and directly related to the main change. |
| 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, impact, and fix in detail, but it does not use the required Summary, Changes, RSR Quality Checklist, or Testing sections. It also provides no completed checklist or test results.
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 identifies that the find -exec command was syntactically invalid and acting as a no-op, the proposed solution is still functionally broken. The test will likely continue to pass silently (as a no-op) because the variables required for placeholder replacement ($placeholder, $value) are not visible inside the single-quoted subshell environment.
Furthermore, the script still references an unassigned "$file" variable instead of the positional parameter $1 passed by find. To resolve this, variables must be either exported or passed as positional arguments to the subshell. There is also a missing test scenario to verify that the replacement actually occurred, which would have caught this logic error.
1 comment outside of the diff
tests/e2e/template_instantiation_test.sh
line 21🟡 MEDIUM RISK
Multiple configuration variables (TEST_OWNER, TEST_FORGE, TEST_AUTHOR_EMAIL, etc.) are flagged as unused. Because they are used in a subshell (viafind -exec), they must be exported to be visible in that environment. Useexportfor these variables at the start of the script.
Test suggestions
- Verify that the
find -execcommand successfully executes for each matched file and passes the filename as the first argument to the subshell. - Ensure the template instantiation test fails if placeholders remain in the output files (verifying the test is no longer a no-op).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Ensure the template instantiation test fails if placeholders remain in the output files (verifying the test is no longer a no-op).
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 logic within the find -exec subshell will fail to execute as intended for two reasons:
- Variables like
$placeholderand$valuedo not expand inside single quotes and will be empty in the subshell. - The variable
"$file"is unassigned in this scope. You should use the positional parameter$1which receives the value from{}.
Refactor the command to pass the required variables as arguments:
find ... -exec sh -c 'placeholder="$1"; value="$2"; file="$3"; sed -i "s|$placeholder|$value|g" "$file"' _ "$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.