fix(scripts): stop wrap composition looping on a token in core content - #4396
Open
Yash-Chindam wants to merge 1 commit into
Open
fix(scripts): stop wrap composition looping on a token in core content#4396Yash-Chindam wants to merge 1 commit into
Yash-Chindam wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The bounded replacement correctly matches existing cross-runtime semantics and has targeted regression coverage.
Pull request overview
Fixes Bash wrap composition hangs when inserted core content contains {CORE_TEMPLATE}, aligning behavior with Python and PowerShell.
Changes:
- Performs single-pass Bash wrapper substitution.
- Adds timeout-enabled regression coverage across script variants.
File summaries
| File | Description |
|---|---|
scripts/bash/common.sh |
Prevents rescanning inserted content. |
tests/parity_helpers.py |
Adds optional subprocess timeout. |
tests/test_resolve_template_python_parity.py |
Tests literal token preservation and termination. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The bash wrap strategy rewrote layer_content in place and then re-tested
the string it had just modified. When the resolved core content held a
literal {CORE_TEMPLATE}, every pass reintroduced the token and the loop
never terminated.
Consume the wrapper left to right instead, appending each segment and the
core content to an accumulator. Work is bounded by the placeholders in the
original wrapper and inserted content is never re-examined, matching the
single-pass semantics the PowerShell (.Replace) and Python (.replace)
ports already have -- so this aligns bash with the other two rather than
introducing new behaviour.
The regression mode is a hang rather than a wrong value, so the new parity
test passes a timeout; run() grows an optional timeout parameter for that.
Without it a reintroduced bug would stall the suite instead of failing it.
Fixes github#4385
Yash-Chindam
force-pushed
the
fix/4385-wrap-infinite-loop
branch
from
September 1, 2026 23:08
314cc5e to
86fd717
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4385
Problem
scripts/bash/common.shrewrotelayer_contentin place and then re-tested the string it had just modified:When the resolved core content holds a literal
{CORE_TEMPLATE}, every pass reinserts it and the condition is true forever.Why this shape of fix
The PowerShell and Python ports were already correct, so the intended semantics did not need inventing — only matching:
String.Replaceandstr.replacescan the original subject once and never revisit inserted text. The new loop consumes the wrapper left to right into an accumulator, so work is bounded by the placeholders in the original wrapper and inserted content is never re-examined. This aligns bash with the other two ports rather than introducing a third behaviour.tests/parity_helpers.pyalready treats.replace()as the oracle for expected wrap output (install_composition_stack), so the three ports now agree with the harness by construction.Verification
I extracted the wrap block verbatim from
upstream/mainand from this branch and ran both under Git Bash:BEFORE\n{CORE_TEMPLATE}\nAFTERBASEBEFORE\nBASE\nAFTERBEFORE\n{CORE_TEMPLATE}\nAFTERBASE {CORE_TEMPLATE}BEFORE\nBASE {CORE_TEMPLATE}\nAFTERA{CORE_TEMPLATE}B{CORE_TEMPLATE}CXAXBXCA and C confirm no behaviour change on the paths that already worked, including multiple placeholders in the wrapper — the fix is not "replace the first occurrence". B matches
str.replacebyte for byte.Test
test_all_variants_treat_core_token_in_core_content_as_literalruns all three variants through the existing parity harness with core content carrying a literal token.The regression mode here is a hang, not a wrong value, so a plain assertion would never fail — it would just never finish.
run()therefore grows an optionaltimeoutparameter (defaultNone, so every existing call is unchanged) and the new test passestimeout=30. Without it, a reintroduced bug would stall the suite instead of failing it.Notes on local test runs
The bash parity tests skip on my machine: Windows resolves
bashto the WSL launcher, whichtests/conftest.py::_has_working_bashcorrectly rejects. I verified the bash behaviour by driving Git Bash directly, as in the table above; CI will exercise the new test through the normal harness.pytest tests/shows 20 pre-existing failures locally, allWinError 1314: A required privilege is not held by the clientfrom symlink creation. I confirmed the identical set fails on unmodifiedupstream/main, so they are environmental and unrelated.Disclosure: this change was developed with AI assistance (Claude). The AI helped locate the defect, compare the three ports, draft the fix and test, and write this description. All reasoning and results above were verified by running the code; I reviewed the change before submitting.