fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #48
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🧰 Additional context used📓 Path-based instructions (1)SPDX: `PMPL-1.0-or-later` on all new files.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe end-to-end template instantiation test now exports its configuration variables and passes each file found by ChangesTemplate instantiation test
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to The change corrects the test’s file-passing command so placeholder replacement operates on matched files; no actionable merge-blocking risk remains beyond normal checks. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the defect, impact, correction, and discovery context. However, it does not follow the repository template and omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections. Resolution Restructure the description using the repository template. Add the Summary and Changes sections, complete the applicable RSR Quality Checklist items, and document the tests or validation performed. Add Screenshots or terminal output if applicable.
✨ 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
While this PR addresses the syntax errors that previously made the 'find' command a no-op, the fix is incomplete. The subshell script currently attempts to use an undefined variable '$file' instead of the positional parameter '$1' passed by the '{}' placeholder. This ensures that the 'sed' commands still fail to execute on the intended files. Additionally, the script continues to log success unconditionally, which masks potential execution failures in the test suite.
About this PR
- The test script logs 'All placeholder tokens replaced' unconditionally. If the 'find' command fails to match files or the subshell encounters an error, the test will still report success. Error handling or a check on the number of files processed should be added to prevent false positives in CI.
Test suggestions
- Verify that the replacement logic inside the 'find -exec' block correctly modifies files with placeholder tokens.
- Verify that the test script fails if the 'find' command fails to locate files or if the replacement logic fails.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that the replacement logic inside the 'find -exec' block correctly modifies files with placeholder tokens.
2. Verify that the test script fails if the 'find' command fails to locate files or if the replacement logic fails.
Low confidence findings
- Consider adding a specific unit or integration test case that validates the replacement logic to prevent future regressions where the test silently skips file modifications.
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 filename from 'find' is passed as $1 to the subshell via '{}', but the script uses '$file'. Update the subshell script to use '$1' or assign 'file="$1"' at the start of the block to ensure the sed command processes the correct files.
🔍 Hypatia Security ScanFindings: 35 issues detected
View findings[
{
"reason": "Issue in label-triage.yml",
"type": "missing_timeout_minutes",
"file": "label-triage.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"reason": "Issue in labels.yml",
"type": "missing_timeout_minutes",
"file": "labels.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"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": "6 workflow(s) with tag-pinned (not SHA-pinned) actions in cafescripto",
"type": "DependencyPinning",
"file": "/home/runner/work/cafescripto/cafescripto",
"action": "auto_fix",
"rule_module": "scorecard",
"severity": "medium",
"remediation": "Pin GitHub Actions and Docker base images by SHA hash.",
"scorecard_check": "Pinned-Dependencies"
},
{
"reason": "Code scanning (Hypatia): hypatia/workflow_audit/missing_timeout_minutes -- Hypatia workflow_audit: missing_timeout_minutes -- 1 day(s) old",
"type": "CSA001",
"file": "labels.yml",
"action": "review",
"rule_module": "code_scanning_alerts",
"severity": "medium"
},
{
"reason": "Code scanning (Hypatia): hypatia/workflow_audit/missing_timeout_minutes -- Hypatia workflow_audit: missing_timeout_minutes -- 1 day(s) old",
"type": "CSA001",
"file": "label-triage.yml",
"action": "review",
"rule_module": "code_scanning_alerts",
"severity": "medium"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 1 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/PLAYBOOK.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 1 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/NEUROSYM.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 1 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/AGENTIC.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 1 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/ECOSYSTEM.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
|
🔍 Hypatia Security ScanFindings: 35 issues detected
View findings[
{
"reason": "Issue in label-triage.yml",
"type": "missing_timeout_minutes",
"file": "label-triage.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"reason": "Issue in labels.yml",
"type": "missing_timeout_minutes",
"file": "labels.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"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": "6 workflow(s) with tag-pinned (not SHA-pinned) actions in cafescripto",
"type": "DependencyPinning",
"file": "/home/runner/work/cafescripto/cafescripto",
"action": "auto_fix",
"rule_module": "scorecard",
"severity": "medium",
"remediation": "Pin GitHub Actions and Docker base images by SHA hash.",
"scorecard_check": "Pinned-Dependencies"
},
{
"reason": "Code scanning (Hypatia): hypatia/workflow_audit/missing_timeout_minutes -- Hypatia workflow_audit: missing_timeout_minutes -- 2 day(s) old",
"type": "CSA001",
"file": "labels.yml",
"action": "review",
"rule_module": "code_scanning_alerts",
"severity": "medium"
},
{
"reason": "Code scanning (Hypatia): hypatia/workflow_audit/missing_timeout_minutes -- Hypatia workflow_audit: missing_timeout_minutes -- 2 day(s) old",
"type": "CSA001",
"file": "label-triage.yml",
"action": "review",
"rule_module": "code_scanning_alerts",
"severity": "medium"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 3 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/PLAYBOOK.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 3 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/NEUROSYM.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 3 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/AGENTIC.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
},
{
"reason": "Code scanning (Hypatia): hypatia/structural_drift/SD004 -- Hypatia structural_drift: SD004 -- 3 day(s) old",
"type": "CSA001",
"file": ".machine_readable/6a2/ECOSYSTEM.a2ml",
"action": "update",
"rule_module": "code_scanning_alerts",
"severity": "high"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
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.