fix(tests): terminate find -exec and pass {} — the placeholder step was a no-op - #50
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 comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe template instantiation end-to-end test now exports placeholder-replacement settings and passes each matched file path to the embedded Bash command. ChangesTemplate instantiation test
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: 🔵 Low · up to The change is localized and intended to make each matched file reach the replacement command, but merge readiness still depends on confirming that the embedded command consumes the matched path through $1; otherwise replacements could still target an empty path. This is a bounded correctness follow-up and is otherwise mergeable with owner awareness. 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, but it does not follow the required template. It omits the required Summary, Changes, RSR Quality Checklist, Testing, and Screenshots sections.
✨ 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
The PR attempts to fix a non-functional placeholder replacement step in the E2E tests by correcting the syntax of the find -exec command. However, the current implementation is still non-functional because the subshell invoked by sh -c does not correctly map the passed file path to the $file variable used in the script block. This results in the sed command operating on an empty variable.
While Codacy reports that the PR is up to standards, this automated check does not detect the runtime logic error in the shell script. Furthermore, the fix lacks a corresponding test case to verify that placeholders are now being replaced as intended. Addressing the scoping issue is critical to ensuring the E2E tests provide the intended coverage.
About this PR
- There is no new test case or verification step included in this PR to prove that the placeholder replacement now functions as expected. Relying solely on the code change without verifying the output in the E2E environment leaves the fix unvalidated.
- This stale or broken code exists in 30 other repositories. Given the systemic nature of this issue, consider whether this test utility should be centralized or if a coordinated update across all affected repositories is planned to prevent logic drift.
Test suggestions
- Verify that template placeholders (e.g., {{project}}) are correctly replaced in generated files during the E2E test run.
- Verify that the
find -execcommand executes without ShellCheck SC2067 errors or syntax failures.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that template placeholders (e.g., {{project}}) are correctly replaced in generated files during the E2E test run.
2. Verify that the `find -exec` command executes without ShellCheck SC2067 errors or syntax failures.
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 variable "$file" inside the subshell is undefined. Since find now passes the filename via {}, you should assign it at the start of the script block (e.g., sh -c 'file="$1"; ...') or use "$1" directly in the sed command. Try updating the sh -c command string to assign file="$1" at the beginning of the script so that the subsequent sed command uses the path provided by find.
|
🔍 Hypatia Security ScanFindings: 37 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 lucidscript",
"type": "DependencyPinning",
"file": "/home/runner/work/lucidscript/lucidscript",
"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 |
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.