Skip to content

fix(substitution): a suggestion containing PIPE was corrupted - #1146

Closed
VXNCXNX wants to merge 1 commit into
vale-cli:v3from
VXNCXNX:fix/substitution-pipe-sentinel
Closed

fix(substitution): a suggestion containing PIPE was corrupted#1146
VXNCXNX wants to merge 1 commit into
vale-cli:v3from
VXNCXNX:fix/substitution-pipe-sentinel

Conversation

@VXNCXNX

@VXNCXNX VXNCXNX commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

A substitution suggestion containing the letters PIPE comes out mangled.

swap: {pipeline: PIPELINE} on the text The pipeline is here.:

before:  "Message": "Use '|LINE' instead of 'pipeline'."
         "Params": [ "|LINE" ]

after:   "Message": "Use 'PIPELINE' instead of 'pipeline'."
         "Params": [ "PIPELINE" ]

So the replace action offers to insert |LINE, which is not what the rule author wrote.

Cause

getOptions protected escaped pipes by round-tripping through a sentinel:

temp := strings.ReplaceAll(match, `\|`, "PIPE")
for _, option := range strings.Split(temp, "|") {
    options = append(options, strings.ReplaceAll(option, "PIPE", `|`))
}

The sentinel is ordinary text a user can legitimately suggest, so the second ReplaceAll rewrites their own content. PIPE, PIPELINE, PIPELINING are all affected.

The fix

Scan for unescaped | instead of substituting a placeholder. No sentinel means nothing to collide with. The existing TODO about regexp2 lacking .Split is why the helper is hand-rolled either way, so this does not add a workaround, it replaces one that could corrupt data with one that cannot.

Escaping still works: a swap of A\|B still yields A|B, verified on the built binary.

Verification

Three cases added to TestOptions in internal/check/substitution_test.go: PIPELINE alone, PIPELINE|conduit (sentinel text plus a real split), and PIPE\|LINE (sentinel text plus an escaped pipe).

Reverting only the split back to the sentinel version fails it:

--- FAIL: TestOptions
    substitution_test.go:354: Expected 'PIPELINE', got '|LINE'

go build ./..., gofmt -l and go test ./... are clean, including the e2e and lint packages.

Disclosure: written with AI assistance (Claude Code). I built binaries before and after and produced the JSON output above by running them, checked that escaped pipes still resolve, and ran the mutation check myself.

getOptions swapped escaped pipes for the literal string PIPE before
splitting, then swapped back, so a swap value of PIPELINE came out as
|LINE. Scan for unescaped pipes instead of round-tripping a sentinel.
@jdkato

jdkato commented Aug 19, 2026

Copy link
Copy Markdown
Member

Thanks — but the TODO above the old code had already been resolved upstream (internal/regex has a Split), so the call site just needed to use it: split on (?<!\\)\| and unescape \|. Closing in favor of that version.

@jdkato jdkato closed this Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants