fix(path-extender-posix): greedy regex causes data loss when duplicate config blocks exist

[Zelys-DFKH]

Both updateShellConfig and replaceSection used [\s\S]* (greedy) to locate the pnpm-managed config block. When a shell config contains two # pnpm sections — from running pnpm setup twice or a manual duplicate — the greedy match spans from the first # pnpm to the last # pnpm end, treating the entire range as one section. On the next pnpm setup --force run, that whole span is replaced with a single updated block, silently destroying anything between the two markers.

Two changes address this:

• Detection in updateShellConfig: [\s\S]* → [\s\S]*? (non-greedy), and the vestigial 'g' flag removed. The regex now stops at the first # pnpm end and reads only that block's content.
• replaceSection: also non-greedy, and updated to use a replace callback that writes the new content into the first match and removes any subsequent duplicate blocks, leaving exactly one # pnpm section in the config.

Reported by @Dzhuneyt in pnpm/pnpm#12282.

Fixes pnpm/pnpm#12282.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of duplicate configuration blocks in shell configuration files; now properly updates the first block, removes duplicates, and preserves intervening content when overwriting enabled.
  • Fixed configuration detection logic to use settings from the first block when duplicates exist.

• Tests

  • Added test coverage for duplicate configuration block scenarios.

[coderabbitai]

Warning

Review limit reached

@Zelys-DFKH, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 51 minutes and 10 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2df5f8f6-0ae5-4f90-9a97-fa61d8137d87

📥 Commits

Reviewing files that changed from the base of the PR and between 0dd9b8a and e830933.

📒 Files selected for processing (1)

• os/env/path-extender-posix/path-extender-posix.spec.ts

📝 Walkthrough

Walkthrough

Fixes a greedy regex bug in path-extender-posix.ts where updateShellConfig now uses a non-greedy, non-global pattern for section detection and replaceSection is rewritten to replace only the first matched pnpm block while deleting subsequent duplicates. Two new Bash tests cover both the overwrite and skip-detection paths for files with duplicate pnpm blocks.

Changes

Duplicate pnpm block handling fix

Layer / File(s)	Summary
Non-greedy regex and first-match-only replacement os/env/path-extender-posix/path-extender-posix.ts	updateShellConfig changes the section-extraction regex to non-greedy without the g flag so match[0]/match[1] reflect only the first pnpm block. replaceSection is rewritten with a callback-based approach that replaces the first matched block with newSection and removes any additional matched instances.
Tests: overwrite and skip with duplicate blocks os/env/path-extender-posix/path-extender-posix.spec.ts	Adds two Bash tests: one verifies that overwrite updates the first block, removes duplicates, and preserves between-block content; another verifies that without overwrite the operation is skipped with oldSettings from the first block.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Two pnpm blocks sat side by side,
A greedy regex swallowed them wide!
Now non-greedy eyes find just the first,
The duplicate gone — content dispersed.
The rabbit hops happy, config intact! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the primary fix: addressing a greedy regex bug that causes data loss when duplicate pnpm config blocks exist.
Linked Issues check	✅ Passed	The changes successfully address all four coding objectives from #12282: using non-greedy regex to correctly identify single pnpm sections, preserving non-pnpm content by replacing only the first block and removing duplicates, supporting safe shell config migrations, and maintaining symmetric detection and replacement logic.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the greedy regex bug: test cases validate duplicate block handling, and implementation changes convert the regexes to non-greedy patterns with deduplication logic.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix greedy regex replacement to avoid data loss with duplicate pnpm blocks
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Description

• Make pnpm config-block matching non-greedy to stop at the first end marker.
• Replace only the first pnpm block and remove duplicate blocks safely.
• Add regression tests to ensure content between duplicate blocks is preserved.

Diagram

graph TD
  A["addDirToPosixEnvPath"] --> B["updateShellConfig"] --> C["replaceSection (dedupe)"] --> D[("Shell config file")]
  B --> D

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Marker-based scanning (indexOf) instead of regex

• ➕ Deterministic first-block selection without regex backtracking risks
• ➕ Easier to reason about multiple blocks and partial/corrupt markers
• ➖ More bespoke code to maintain
• ➖ Needs careful handling of line endings and marker formatting

2. Parse into blocks and normalize all managed sections

• ➕ Can preserve formatting more predictably and support future metadata
• ➕ Makes it explicit which blocks are removed/kept
• ➖ Heavier implementation than necessary for a small fix
• ➖ Higher risk of introducing new formatting regressions

Recommendation: Keep the current approach: non-greedy detection plus a single-pass global replace callback is minimal, fixes the data-loss bug, and adds coverage for duplicates. Consider marker-based scanning only if more complex block formats or additional section types are introduced.

Files changed (2) +52 / -2

Bug fix (1) +7 / -2

path-extender-posix.tsFix non-greedy block detection and dedupe duplicate sections on replace +7/-2

Fix non-greedy block detection and dedupe duplicate sections on replace

• Switches pnpm block detection in updateShellConfig to a non-greedy capture so it stops at the first end marker. Updates replaceSection to replace only the first matched block and remove any additional duplicate blocks found in the same file.

os/env/path-extender-posix/path-extender-posix.ts

Tests (1) +45 / -0

path-extender-posix.spec.tsAdd regression coverage for duplicate pnpm blocks in shell configs +45/-0

Add regression coverage for duplicate pnpm blocks in shell configs

• Adds tests to ensure only the first pnpm block is updated while any subsequent duplicate blocks are removed. Verifies that unrelated content between blocks is preserved and that oldSettings detection comes from the first block only.

os/env/path-extender-posix/path-extender-posix.spec.ts

[coderabbitai]

🧹 Nitpick comments (1)

os/env/path-extender-posix/path-extender-posix.spec.ts (1)

270-294: ⚡ Quick win

Assert marker de-duplication explicitly in overwrite test.

This test checks content preservation/removal, but it doesn’t verify there is exactly one # pnpm ... # pnpm end section left. Adding that assertion will better lock the contract.

🧪 Suggested test hardening

     expect(configContent).toContain('# content between blocks that must not be deleted')
     expect(configContent).toContain(`export PNPM_HOME="${pnpmHomeDir}"`)
     expect(configContent).not.toContain('duplicate_block')
+    expect(configContent.match(/^# pnpm$/gm) ?? []).toHaveLength(1)
+    expect(configContent.match(/^# pnpm end$/gm) ?? []).toHaveLength(1)

🤖 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 `@os/env/path-extender-posix/path-extender-posix.spec.ts` around lines 270 -
294, The test `should update first pnpm block and remove duplicate blocks,
preserving content between them` currently verifies content preservation and
variable values, but does not explicitly assert that exactly one pnpm
configuration block remains after deduplication. Add assertions after reading
configContent to verify that the string `# pnpm` and `# pnpm end` each appear
exactly once in the final config file, ensuring the marker de-duplication
contract is properly tested and locked in.

🤖 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.

Nitpick comments:
In `@os/env/path-extender-posix/path-extender-posix.spec.ts`:
- Around line 270-294: The test `should update first pnpm block and remove
duplicate blocks, preserving content between them` currently verifies content
preservation and variable values, but does not explicitly assert that exactly
one pnpm configuration block remains after deduplication. Add assertions after
reading configContent to verify that the string `# pnpm` and `# pnpm end` each
appear exactly once in the final config file, ensuring the marker de-duplication
contract is properly tested and locked in.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 160775fb-6bda-4780-b511-18a65fa3c143

📥 Commits

Reviewing files that changed from the base of the PR and between a9b32aa and 0dd9b8a.

📒 Files selected for processing (2)

• os/env/path-extender-posix/path-extender-posix.spec.ts
• os/env/path-extender-posix/path-extender-posix.ts

📜 Review details

🧰 Additional context used

🪛 OpenGrep (1.22.0)

os/env/path-extender-posix/path-extender-posix.ts

[ERROR] 224-224: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🔇 Additional comments (2)

os/env/path-extender-posix/path-extender-posix.ts (1)

224-224: LGTM!

Also applies to: 256-261

os/env/path-extender-posix/path-extender-posix.spec.ts (1)

295-314: LGTM!

[Zelys-DFKH]

Good catch. Added both assertions.