fix(core): don't strip shell/YAML # unless it starts a comment - #1752
fix(core): don't strip shell/YAML # unless it starts a comment#1752serhiizghama wants to merge 5 commits into
Conversation
Shell and YAML files were routed through the 'perl' strip profile, which
treats every unquoted # as a comment. That corrupts common syntax such as
${name##*/}, $#, and unquoted URLs like a/b#readme, and drops trailing
content on the first stray #. Strip them with a boundary-aware scanner that
only treats # as a comment at line start or after whitespace, and skips
single/double quoted strings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds boundary-aware ChangesHash comment stripping
Estimated code review effort: 4 (Complex) | ~40 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/file/fileManipulate.ts`:
- Around line 55-102: Extend the comment-removal scanner around its existing
quote and `#` handling to track shell heredoc bodies and YAML block-scalar
bodies, preserving every line of those literal sections—including indented or
standalone `#` content—until each construct ends. Apply the boundary rule only
outside these literal-body states, and add regression coverage for quoted
heredocs and YAML `|` block scalars.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d5dba17-66b6-4409-929e-916c00c9b8d5
📒 Files selected for processing (2)
src/core/file/fileManipulate.tstests/core/file/fileManipulate.test.ts
…tripping The boundary-aware scan treated every line-start `#` as a comment, which corrupted shell heredocs (cat <<EOF ... EOF) and YAML block scalars (key: | ...) since their literal bodies can contain lines starting with `#`. Track these as line/indentation-scoped states so body content is copied verbatim instead of scanned for comments.
|
Good catch — heredocs and block scalars are literal content, the boundary rule shouldn't touch them. Added line/indentation-scoped tracking for both (shell |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/file/fileManipulate.ts (1)
144-153: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftQuote-open logic is shell-specific but applied unconditionally to YAML too.
'/"unconditionally flipinSingle/inDoubleregardless of position. That's correct for shell (an unescaped quote anywhere outside quotes really does open a string), but wrong for YAML: a plain scalar may contain a literal, unescaped'/"mid-value (e.g.note: it's working # todo) — only a quote as the first character of a value starts a quoted scalar in YAML. With the current logic, the apostrophe init'sflipsinSingle = true, and since no closing'follows on that line, the state persists across subsequent lines (inSingle/inDoublecarry over viaprocessed.inSingle/inDouble), silently disabling#-comment stripping for everything until another'happens to appear. This is a real, unexercised gap — none of the added tests use an apostrophe/contraction value.🐛 Proposed fix: only treat quotes as YAML value-openers at a value boundary
- if (char === "'") { - inSingle = true; - result += char; - continue; - } - if (char === '"') { - inDouble = true; - result += char; - continue; - } + if (char === "'" || char === '"') { + const prev = line[i - 1]; + const atValueBoundary = + i === 0 || prev === ' ' || prev === '\t' || prev === ':' || prev === '-' || prev === ',' || prev === '[' || prev === '{'; + if (this.language !== 'yaml' || atValueBoundary) { + if (char === "'") inSingle = true; + else inDouble = true; + } + result += char; + continue; + }Consider adding a regression test for a YAML value containing an apostrophe (e.g.
note: it's fine # comment) once fixed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/file/fileManipulate.ts` around lines 144 - 153, Update the quote handling in the relevant file-scanning/parser function so YAML mode only opens inSingle or inDouble when the quote is the first character of a value or follows a YAML value boundary, while preserving the existing unconditional opening behavior for shell mode. Ensure apostrophes or double quotes inside YAML plain scalars do not persist quote state across lines, and add a regression test for a contraction followed by a YAML comment if the surrounding tests support it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/file/fileManipulate.ts`:
- Around line 48-50: Update YAML_BLOCK_SCALAR_START to require the block scalar
indicator to occur in a valid value position—after a mapping colon,
sequence-item marker, or at the start of the line—while preserving support for
optional chomp and indentation indicators and trailing comments. Ensure bare
scalars such as version: >5 and range: |3 no longer match.
- Around line 76-111: Change the output assembly in the file-processing method
so rtrimLines is applied only to normally processed lines, not heredoc or YAML
block-scalar lines pushed verbatim in the heredocDelimiter and blockScalarIndent
branches. Preserve trailing whitespace for those literal body lines while
retaining existing trimming behavior for ordinary content.
---
Outside diff comments:
In `@src/core/file/fileManipulate.ts`:
- Around line 144-153: Update the quote handling in the relevant
file-scanning/parser function so YAML mode only opens inSingle or inDouble when
the quote is the first character of a value or follows a YAML value boundary,
while preserving the existing unconditional opening behavior for shell mode.
Ensure apostrophes or double quotes inside YAML plain scalars do not persist
quote state across lines, and add a regression test for a contraction followed
by a YAML comment if the surrounding tests support it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62a39c93-af72-4856-a53d-5e29a07a421e
📒 Files selected for processing (2)
src/core/file/fileManipulate.tstests/core/file/fileManipulate.test.ts
…aries An apostrophe inside a YAML plain scalar (note: it's fine # x) flipped the single-quote state and swallowed every following # comment until the next quote. Only open a quoted scalar at a value boundary in YAML, and require the block scalar |/> indicator to follow a : or - introducer so a plain scalar ending in | isn't mistaken for one.
|
Good catches on the YAML edge cases. Fixed both: quotes now only open a scalar at a value boundary in YAML, so an apostrophe in a plain scalar ( Left |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
--remove-commentssilently corrupts shell and YAML files. Both are routed to theperlstrip profile, which treats every unquoted#as a line comment. But in shell and YAML a#is only a comment at the start of a line or after whitespace — not inside${name##*/},$#, or an unquoted URL likea/b#readme.So a shell script like:
comes out as
base=${name,ext=${name,count=$— the rest of each line is dropped at the first#. Same for YAML:repo: https://github.com/a/b#readmeloses#readme. The packed output no longer matches the source, which is exactly what you don't want when feeding it to an LLM.I replaced the
perlrouting for.sh,.yaml, and.ymlwith a small boundary-aware scanner that only cuts at a#when it's at line start or preceded by whitespace, and skips single/double quoted strings (soecho "a # b"andcolor: "#ff0000"are kept). Real comments — full-line, whitespace-preceded trailing ones, and shebangs — are still stripped, so the existing shell/YAML tests stay green. Added tests for the parameter-expansion,$#, quoted-hash, and URL-fragment cases.