Draft
Conversation
curtisman
added a commit
that referenced
this pull request
Mar 26, 2026
…g; add invariant docs and test infrastructure (microsoft#2082) ## Summary Fix a bug where `directionSensitive` recompute was skipped when `openWildcard=true`, and add comprehensive invariant documentation and automated checking infrastructure. ### Bug fix (`grammarMatcher.ts`) The `directionSensitive` recompute for backed-up backward positions was gated on `processRangeCandidates`, which included `rangeCandidateGateOpen = !openWildcard || partialKeywordBackup`. When `openWildcard=true` and no partial keyword backup, the recompute was skipped, leaving a potentially stale `directionSensitive` value. Changed the guard to explicitly check `direction === "backward" && backwardEmitted && maxPrefixLength < prefix.length`, decoupling the recompute from range-candidate processing. ### Documentation - **New Invariants section in `completion.md`** — full catalog of 12 correctness invariants (#1–microsoft#12) across grammar-matcher, cross-direction, field-specific, and merge layers, with a quick-reference symptom→invariant table and a Known Gaps section - **Cross-reference trailing-separator-commits** to invariant #3 (`directionSensitive` biconditional) in `actionGrammar.md` - **Clarify `properties` scope** in `actionGrammar.md` — `properties` is a grammar-matcher concept; at the dispatcher layer, `closedSet=false ↔ properties non-empty` does not hold - **Expand `directionSensitive=false`** description in `completion.md` noting backward can back up past trailing whitespace into wildcards (→ `directionSensitive=true`) - **Fix stale claim** in `actionGrammar.md` about recompute being skipped when `partialKeywordBackup` or `openWildcard` is set (that condition was removed from the code) - **Add cross-reference** from `actionGrammar.md` metadata section to `completion.md` § Invariants ### Test infrastructure - **`withInvariantChecks` wrapper** (`testUtils.ts`) — wraps the grammar completion function to run both forward and backward on every call, automatically checking invariants #1–microsoft#4; wired into `createTestCompletion` for the `"grammar"` variant so all existing grammar tests gain invariant checking for free - **Replace `expect()`-based equality** with `node:util` `isDeepStrictEqual` so the invariant checker works outside a Jest assertion context - **Regression test** for `directionSensitive` recompute when `openWildcard=true` — uses `play $(track:TrackName) now` grammar to verify backward correctly reports `directionSensitive=true` at the backed-up position
curtisman
added a commit
that referenced
this pull request
Mar 27, 2026
Production fix (grammarCompletion.ts): - Add post-loop recomputation: at any 0 < P < prefix.length with completions/properties and not openWildcard, set directionSensitive=true. backward(input[0..P]) always backs up at end-of-string boundary, so the position is inherently direction-sensitive. - Applies to both forward and backward directions. Invariant overhaul (testUtils.ts): Replace old cross-direction invariants (#3 biconditional, microsoft#4 two-pass) with 5 position-local invariants: #1: same matchedPrefixLength -> identical results #2: fwd.directionSensitive=false -> fwd === backward(input[0..fwd.mpl]) #3: bwd.directionSensitive=false -> bwd === forward(input[0..bwd.mpl]) microsoft#4: fwd.directionSensitive=true -> backward(truncated) backs up microsoft#5: different mpl + bwd.directionSensitive -> forward reaches bwd pos Test expectations (38 tests across 3 spec files): Update directionSensitive: false -> true for cases where 0 < matchedPrefixLength < prefix.length with completions.
curtisman
added a commit
that referenced
this pull request
Mar 27, 2026
- Update completion.md invariants section to match decomposed cross-direction checks (#1-microsoft#5) in testUtils.ts, with test-to-doc number mapping table - Remove stale closedSet mention from EOI guard description in completion.md - Restore inline explanation for 'play Never b' table row in completion.md - Clarify shouldPreferNewResult comment about caller's equal-case guard - Rename partialKeywordBackupAgreesWithForward → partialKeywordAgrees - Add inline /* paramName */ comments on triple-boolean collectStringCandidate call - Add 'Return value intentionally ignored' comment at Category 1 tryCollect call - Add type comment on forwardEoiCandidates explaining only wildcardString used - Add Phase B debug labels for clear/displace/merge modes - Add anchor terminology comment (anchor → matchedPrefixLength) - Add defensive-guard comment on direction !== backward in Phase B - Comment Infinity sentinel in mergeCompletionResults tests
curtisman
added a commit
that referenced
this pull request
Mar 28, 2026
- Group trailingSepAdvanced with the directionSensitive decision tree under a shared section header for better locality (microsoft#7) - Add invariant comment: openWildcard requires a preceding keyword match so P > 0 whenever openWildcard is true (#1) - Add JSDoc to couldBackUp return field in tryPartialStringMatch (microsoft#5) - Compact equivalence analysis tables in actionGrammar.md from 5-column wide format to 3-column for readability in narrow contexts (microsoft#6)
curtisman
added a commit
that referenced
this pull request
Mar 29, 2026
- Remove redundant from>0 guard in isSeparatorOnlyGap (to>from suffices) - Remove redundant anchor!==maxPrefixLength conjunct at Phase B - Use 'forward|backward' union type instead of string for direction param - Add consolidated invariant index (#1-microsoft#8) at top of test utils - Renumber invariants: single-result #1-#2, cross-direction #3-microsoft#7, truncated-forward microsoft#8 - Replace mpl abbreviation with matchedPrefixLength in comments - Standardize on 'fallback' terminology (was mixed fallthrough/fallback) - Extract normalizeCompletionResult helper alongside completionResultsEqual - Fix stale test name that still referenced old expected completions - Fix 'Backup failed' wording to name the function explicitly
curtisman
added a commit
that referenced
this pull request
Mar 29, 2026
Align test code and doc numbering so both use the same continuous sequence matching the order invariants appear in completion.md: #1-#2 per-result (assertSingleResultInvariants) #3 truncated-forward idempotency (assertTruncatedForwardInvariant) microsoft#4-microsoft#8 cross-direction (assertCrossDirectionInvariants) microsoft#9-microsoft#10 field-specific (doc-only) microsoft#11-microsoft#15 merge invariants (doc-only) Update cross-references in actionGrammar.md and grammarCompletion.ts to match.
curtisman
added a commit
that referenced
this pull request
Mar 29, 2026
…crosoft#15 (microsoft#2097) ## Summary Fix two backward completion bugs in the grammar completion engine, extract a helper, add a new invariant (# 3 — truncated-forward idempotency), and renumber all invariants to a continuous # 1–# 15 scheme. ## Changes ### Bug fixes - **Backward property fallback**: when `tryCollectBackwardCandidate` returns false (all keyword words fully matched with trailing separator), the engine now falls back to `collectPropertyCandidate` so unfilled wildcard slots (e.g. `$(artist)`) are still offered. - **Range candidate truncation**: range candidate processing now truncates the prefix to `maxPrefixLength` before calling `tryPartialStringMatch`, preventing peeking at input beyond what was consumed. - **Separator-only gap preservation**: `updateMaxPrefixLength` is bypassed when the gap to the new position contains only separator characters, preserving legitimate existing candidates instead of clearing them. - **Phase B EOI merge with partial keywords**: removes the `!hasPartialKeyword` guard — partial keyword paths with separator-only gaps now merge rather than displacing. ### Refactor - Extract `isSeparatorOnlyGap(text, from, to)` helper for reuse across Phase B partial-keyword and Phase B EOI anchor merge paths. - Add `normalizeCompletionResult` (sort completions before comparing) in test utils. - Strengthen `direction` parameter types from `string` to `"forward" | "backward"` literal union. ### New invariant - **# 3 — Truncated-forward idempotency**: `matchedPrefixLength < prefix.length` → `result === completion(input[0..matchedPrefixLength], "forward")`. Checked automatically by `assertTruncatedForwardInvariant` for both directions. ### Documentation - Renumber all invariants to continuous # 1–# 15 in document order across `completion.md`, `actionGrammar.md`, and test code. - Update the quick-reference symptom→invariant table. - Add unified invariant mapping table replacing the old test↔doc mapping. ## Test coverage - New test: `"play hello by "` backward — property fallback via Category 3a. - New test: `"play something play"` backward — separator-only gap preserves "good". - New test: `"play something played"` backward — range candidate uses truncated prefix. - Updated test: `"play something play"` forward — now expects `["good", "played"]` (merge). - All invariants (# 1–# 8) automatically checked via `withInvariantChecks` wrapper.
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
1 participant
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.
No description provided.