fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #49
fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op#49hyperpolymath 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. (4)
|
| Layer / File(s) | Summary |
|---|---|
Forward matched template paths tests/e2e/template_instantiation_test.sh |
The find -exec bash -c invocation now uses {} for each matched path instead of "$file". |
Estimated code review effort: 1 (Trivial) | ~2 minutes
Merge Risk: ⚪ Minimal · up to da6a2
This localized test correction restores the intended placeholder replacement behavior, with no actionable merge-blocking risk remaining after normal checks and review.
Poem
A rabbit checks each path in line
Braces guide the files just fine
The template test runs clear
No misplaced paths appear
One small fix makes results align
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description explains the defect, impact, and correction, but it omits the required template headings and RSR Quality Checklist. It also gives no explicit testing details. | Use the repository template. Add the Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections as applicable. Mark the relevant checklist items and state which tests were run and their results. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the main fix: terminating find -exec and passing the {} placeholder. |
| 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: 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}
🔍 Hypatia Security ScanFindings: 36 issues detected
View findings[
{
"reason": "Issue in push-email-notify.yml",
"type": "missing_timeout_minutes",
"file": "push-email-notify.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/PLAYBOOK.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/NEUROSYM.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/AGENTIC.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/ECOSYSTEM.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/META.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 11 day(s) old [STALE]",
"type": "CSA001",
"file": ".machine_readable/6a2/STATE.a2ml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Scorecard): TokenPermissionsID -- Token-Permissions -- 35 day(s) old [STALE]",
"type": "CSA001",
"file": ".github/workflows/scorecard.yml",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Scorecard): BranchProtectionID -- Branch-Protection -- 62 day(s) old [STALE]",
"type": "CSA001",
"file": "no file associated with this alert",
"action": "escalate",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/workflow_audit/missing_timeout_minutes -- Hypatia workflow_audit: missing_timeout_minutes -- 62 day(s) old [STALE]",
"type": "CSA001",
"file": "push-email-notify.yml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "medium"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
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 error that caused the find command to fail, the implementation remains effectively a no-op. Both reviewers identified that variables like $placeholder and $value will not expand inside the single-quoted sh -c subshell, meaning the sed command will likely execute with empty parameters.
Furthermore, the test lacks an explicit assertion (e.g., grep) to verify that the template placeholders have actually been replaced. Without this check, future regressions or scoping issues like the one in this PR will continue to fail silently. It is also noted that this script appears to be a duplicate of one maintained in rsr-template-repo, which introduces a risk of maintenance divergence.
About this PR
- The test script currently lacks an explicit assertion to verify that placeholders (e.g.,
{{project}}) are actually replaced. Adding agrepcheck after the replacement loop would ensure that the test fails if the logic is a no-op.
Test suggestions
- Verify that template placeholders (e.g., {{project}}) are actually replaced in files by asserting file content after the find/sed loop.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that template placeholders (e.g., {{project}}) are actually replaced in files by asserting file content after the find/sed loop.
Low confidence findings
- This script appears to be a stale duplicate of a script already fixed in
rsr-template-repo. Consider consolidating these or pulling the logic from a central source to prevent further divergence.
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.
🟡 MEDIUM RISK
The find syntax is now valid, but the replacement logic remains a no-op because variables like $placeholder and $value are inside single quotes, which prevents the parent shell from expanding them. Additionally, using \; executes the shell script once per file; using + is more efficient for processing multiple files.
Refactor the find command to pass variables as positional arguments to the subshell. For example:
find ... -exec sh -c 'p="$1"; v="$2"; shift 2; for file in "$@"; do sed -i "s/$p/$v/g" "$file"; 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.