var/renameStep writes optional-group notation into documents, silently deleting the assertion

[aslakhellesoy]

Found while reworking 15 oaths in a real project (@varar/varar + @varar/vitest + @varar/lsp 0.8.1).

var/renameStep renders the new expression's source text into every matching sentence, rather than the text that would actually match it. When the new expression contains an optional group, the notation lands in the document verbatim — and checkout(s) matches neither checkout nor checkouts.

The assertion does not fail. It stops existing.

Repro

Step definition:

sensor('sent to {int} checkout(s)', async (world) =>
  world.adapters.paymentProcessorWeb.getAllCheckouts().length)

Document, before — note Varar rendered the optional correctly when the sentence was written by hand:

...they are sent to 1 checkout, and the checkout lists:

Rename sent to {int} checkout(s) → handed {int} checkout(s) to pay. The LSP reports 1 site(s) and writes:

...they are handed 1 checkout(s) to pay, and the checkout lists:

Why this is worse than a broken example

The sentence still contains other steps that match, so the paragraph stays live. It is not a delimiter, the example is not split, and drift sees nothing wrong. The sensor simply never runs again:

Document says: "they are handed 99 checkout(s) to pay"
System creates: 1 checkout
Result:         Tests  3 passed (3)

I changed 1 to 99 specifically to check, and the suite stayed green. A rename — the one operation the LSP exists to make safe — silently converted a verified claim into decorative prose. That is the failure mode explanation/no-theatre is built to prevent, arriving through the refactoring tool.

Suggestions

In rough order of how much I'd want them:

1. Render per site from the matched text. renameStep already has each site's paramValues and span. For an optional group it has to decide checkout vs checkouts; the old site's text is the evidence — if the old expression had the same optional group and the site rendered it as checkout, keep checkout. renderExpressionText presumably needs the same treatment.
2. Refuse rather than corrupt. If rendering an optional (or alternation) is ambiguous, return { ok: false, error }. Refusing a rename is a minor annoyance; silently deleting an assertion is not.
3. Verify the write. After computing each site's new text, re-match it against the new expression and fail the whole rename if any site no longer matches. That is a cheap invariant — a rename must leave every site matching — and it would have caught this class of bug generally, not just optionals.

(3) feels like the one worth having regardless of (1), since it turns "the rename tool can silently break documents" into "it cannot".

[aslakhellesoy]

Confirmed, root cause located, and this is now the first code change of the batch — ahead of everything else filed yesterday.

The cause is explicit in the source. renderExpression builds each site from the new expression's segments, and expression-segments.ts says what happens to the non-parameter ones:

// Optional ( `(foo)` ) and alternation ( `red/green` ) groups don't
// carry a runtime value the way a parameter does; we keep them as
// opaque literal-ish slices so the differ can still detect rewrites.

and then, on renderExpression itself: "Opaque (optional/alternation) segments are emitted verbatim." So checkout(s) goes into the document as checkout(s), which matches neither branch. Your diagnosis was exactly right, including that renderExpressionText needs the same treatment.

Why this got more urgent overnight. The answer to #118 — a sentence can only open with a step if the expression says so — is to write the first word as an alternation: The/the visitor checks out the basket. That's about to become recommended idiom in the docs. Alternations are the same opaque segment as optionals, so a rename of any such step would write The/the visitor checks out the basket into the prose, matching nothing, silently. The idiom I'm about to teach is booby-trapped by this bug.

So it's both of your first two fixes, not one:

1. Render per site from the matched text. The old site's text is the evidence for which branch to emit, as you say. This has to handle alternation as well as optionals now.
2. Refuse rather than corrupt where the old site gives no evidence (a group that didn't exist in the old expression) — { ok: false, error } beats a guess.
3. Verify the write, which I agree is the one worth having regardless. Re-match every rewritten site against the new expression and fail the whole rename if any site no longer matches. It's cheap, it's the correct invariant — a rename must leave every site matching — and it turns "the rename tool can silently break documents" into "it cannot", which is the property I actually want.

That is the failure mode explanation/no-theatre is built to prevent, arriving through the refactoring tool.

Yes. That's the sentence that reordered the plan.