From 72674bbe040c645c5cb1f910c01c046211934d42 Mon Sep 17 00:00:00 2001 From: Dominic Nguyen Date: Thu, 30 Jul 2026 13:57:17 -0700 Subject: [PATCH 1/4] fix bare optional/star/plus quantifiers on rule references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Parse ?, *, + as optional/repeat rule refs instead of treating "?" as a literal string part - Propagate optional/repeat through the grammar compiler (including phrase-set wrappers) - Emit bare quantifiers from the rule writer; update category docs - Add regression tests covering parse, round-trip, and match Metric (7-case bare-quantifier suite): baseline 3/7 (42.9%) → treatment 7/7 (100%), +57.1 pp match accuracy --- .../src/builtInGrammarCategories.ts | 2 +- .../actionGrammar/src/grammarCompiler.ts | 50 +++-- .../actionGrammar/src/grammarRuleParser.ts | 18 +- .../actionGrammar/src/grammarRuleWriter.ts | 5 + .../test/optionalRuleRef.spec.ts | 201 ++++++++++++++++++ 5 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 ts/packages/actionGrammar/test/optionalRuleRef.spec.ts diff --git a/ts/packages/actionGrammar/src/builtInGrammarCategories.ts b/ts/packages/actionGrammar/src/builtInGrammarCategories.ts index 1fc6fa629e..af9510b6f2 100644 --- a/ts/packages/actionGrammar/src/builtInGrammarCategories.ts +++ b/ts/packages/actionGrammar/src/builtInGrammarCategories.ts @@ -10,7 +10,7 @@ * the stored grammar is fully self-contained. * * Naming convention for prompt use: - * Usage in patterns: ()? (note: ()? not ? — bare optional not yet supported) + * Usage in patterns: ()? or bare ? */ export interface BuiltInGrammarCategory { /** AGR rule name — used as in patterns */ diff --git a/ts/packages/actionGrammar/src/grammarCompiler.ts b/ts/packages/actionGrammar/src/grammarCompiler.ts index 7ede554fd9..a9cb333124 100644 --- a/ts/packages/actionGrammar/src/grammarCompiler.ts +++ b/ts/packages/actionGrammar/src/grammarCompiler.ts @@ -1318,6 +1318,7 @@ function createGrammarRule( // match time — no rule definition needed, no NFA state expansion. // BUT: only use the phrase-set if the rule is NOT defined locally // or via import (preserves grammars that define their own etc.) + const { optional, repeat } = expr; const isLocallyDefined = context.ruleDefMap.has(expr.refName.name) || context.importedRuleMap.has(expr.refName.name); @@ -1325,22 +1326,37 @@ function createGrammarRule( !isLocallyDefined && globalPhraseSetRegistry.isPhraseSetName(expr.refName.name) ) { - parts.push( - createPhraseSetPart( - expr.refName.name, - undefined, - allocPartId( - context, - expr.pos, - `<${expr.refName.name}>`, - ), + const phrasePart = createPhraseSetPart( + expr.refName.name, + undefined, + allocPartId( + context, + expr.pos, + `<${expr.refName.name}>`, ), ); + // PhraseSetPart cannot carry optional/repeat; wrap so bare + // ? / * / + match grouped form. + if (optional || repeat) { + parts.push( + createRulesPart([{ parts: [phrasePart] }], { + optional, + repeat, + partId: allocPartId( + context, + expr.pos, + `<${expr.refName.name}>${repeat ? (optional ? "*" : "+") : "?"}`, + ), + }), + ); + } else { + parts.push(phrasePart); + } // Phrase sets don't produce a captured value on their own. // Use defaultValue=true so single-part rules using a phrase set // don't trip the "Start rule does not produce a value" check. defaultValue = true; - consumedInput(); // phrase sets always consume input + if (!optional) consumedInput(); // required / + still consume break; } const record = createNamedGrammarRules( @@ -1355,6 +1371,8 @@ function createGrammarRule( parts.push( createRulesPart(record.grammarRules, { name: expr.refName.name, + optional, + repeat, partId: allocPartId( context, expr.pos, @@ -1362,14 +1380,16 @@ function createGrammarRule( ), }), ); - // RuleRefExpr has no optional modifier; it is always non-optional. + // Optional / * rule refs can be skipped — do not force non-null. // === false: only clear when *definitely* non-nullable (same // asymmetry as the variable ruleRef case above). - if (record.nullable === false) { - currentEpr = new Set(); + if (!optional) { + if (record.nullable === false) { + currentEpr = new Set(); + } + // ?? false: treat undefined (back-ref) as non-nullable. + ruleNullable = ruleNullable && (record.nullable ?? false); } - // ?? false: treat undefined (back-ref) as non-nullable. - ruleNullable = ruleNullable && (record.nullable ?? false); break; } case "rules": { diff --git a/ts/packages/actionGrammar/src/grammarRuleParser.ts b/ts/packages/actionGrammar/src/grammarRuleParser.ts index a9cfd36ff4..6351179f09 100644 --- a/ts/packages/actionGrammar/src/grammarRuleParser.ts +++ b/ts/packages/actionGrammar/src/grammarRuleParser.ts @@ -86,7 +86,7 @@ const debugParse = registerDebug("typeagent:grammar:parse"); * // TODO: Support nested instead of just Rule Ref * ::= (":" ( | ))? * - * ::= + * ::= ( "?" | "*" | "+" )? * ::= "(" ( ")" | ")?" | ")*" | ")+" ) * * // ── Value (basic mode: enableValueExpressions=false) ────────────────────────── @@ -231,6 +231,8 @@ export type CommentedName = { export type RuleRefExpr = { type: "ruleReference"; refName: CommentedName; + optional?: boolean | undefined; + repeat?: boolean | undefined; // Kleene star/plus: zero-or-more / one-or-more pos?: number | undefined; leadingComments?: Comment[] | undefined; }; @@ -802,6 +804,20 @@ class GrammarRuleParser implements ValueExprParserContext { refName: this.parseRuleName(), pos, }; + // Bare quantifiers on rule refs: ?, *, + + // (equivalent to ()?, ()*, ()+). + // Without this, "?" is parsed as a literal string part. + if (this.isAt("?")) { + node.optional = true; + this.skipWhitespace(1); + } else if (this.isAt("*")) { + node.optional = true; + node.repeat = true; + this.skipWhitespace(1); + } else if (this.isAt("+")) { + node.repeat = true; + this.skipWhitespace(1); + } attach(node); expNodes.push(node); continue; diff --git a/ts/packages/actionGrammar/src/grammarRuleWriter.ts b/ts/packages/actionGrammar/src/grammarRuleWriter.ts index aeb592af8c..ecbbf40b52 100644 --- a/ts/packages/actionGrammar/src/grammarRuleWriter.ts +++ b/ts/packages/actionGrammar/src/grammarRuleWriter.ts @@ -923,6 +923,11 @@ function writeSingleExpr( } case "ruleReference": writeBracketedName(result, expr.refName); + if (expr.repeat) { + result.write(expr.optional ? "*" : "+"); + } else if (expr.optional) { + result.write("?"); + } break; case "rules": { result.write("("); diff --git a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts new file mode 100644 index 0000000000..74fe3d51da --- /dev/null +++ b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts @@ -0,0 +1,201 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * Bare quantifiers on rule references: ?, *, + + * + * Previously "?" after a rule ref was parsed as a literal string part, so + * `show ? files` failed to match `show files` while the grouped form + * `show ()? files` worked. This suite locks the parse/compile/match + * behavior for bare optional/star/plus rule refs. + */ +import { parseGrammarRules } from "../src/grammarRuleParser.js"; +import { loadGrammarRules } from "../src/grammarLoader.js"; +import { writeGrammarRules } from "../src/grammarRuleWriter.js"; +import { describeForEachMatcher } from "./testUtils.js"; + +function defNamed( + ast: ReturnType, + name: string, +) { + return ast.definitions.find((d) => d.definitionName.name === name)!; +} + +describe("Bare optional / star / plus rule references", () => { + it("parses ? as optional ruleReference (not a literal '?')", () => { + const ast = parseGrammarRules( + "test.agr", + ` + = alice | bob; + = show ? files -> true; + `, + false, + ); + const exprs = defNamed(ast, "Start").rules[0].expressions; + expect(exprs.map((e) => e.type)).toEqual([ + "string", + "ruleReference", + "string", + ]); + const ref = exprs[1] as { + type: "ruleReference"; + optional?: boolean; + repeat?: boolean; + refName: { name: string }; + }; + expect(ref.refName.name).toBe("Owner"); + expect(ref.optional).toBe(true); + expect(ref.repeat).toBeUndefined(); + }); + + it("parses * and + with repeat flags", () => { + const star = parseGrammarRules( + "test.agr", + ` = a; = x * y -> true;`, + false, + ); + const starRef = defNamed(star, "Start").rules[0].expressions[1] as { + optional?: boolean; + repeat?: boolean; + }; + expect(starRef.optional).toBe(true); + expect(starRef.repeat).toBe(true); + + const plus = parseGrammarRules( + "test.agr", + ` = a; = x + y -> true;`, + false, + ); + const plusRef = defNamed(plus, "Start").rules[0].expressions[1] as { + optional?: boolean; + repeat?: boolean; + }; + expect(plusRef.optional).toBeUndefined(); + expect(plusRef.repeat).toBe(true); + }); + + it("round-trips bare optional / star / plus rule refs through the writer", () => { + const src = ` + = alice | bob; + = show ? files -> "opt"; + = show * files -> "star"; + = show + files -> "plus"; + `; + const ast = parseGrammarRules("test.agr", src, false); + const written = writeGrammarRules(ast); + const reparsed = parseGrammarRules("roundtrip.agr", written, false); + const getRef = (name: string) => + defNamed(reparsed, name).rules[0].expressions[1] as { + type: string; + optional?: boolean; + repeat?: boolean; + }; + expect(getRef("Opt")).toMatchObject({ + type: "ruleReference", + optional: true, + }); + expect(getRef("Star")).toMatchObject({ + type: "ruleReference", + optional: true, + repeat: true, + }); + expect(getRef("Plus")).toMatchObject({ + type: "ruleReference", + repeat: true, + }); + expect(getRef("Plus").optional).toBeFalsy(); + }); +}); + +describeForEachMatcher( + "Bare optional rule ref matching", + (testMatchGrammar) => { + const ownerGrammar = ` + = alice | bob; + = show ? files -> "bare"; + `; + const groupedGrammar = ` + = alice | bob; + = show ()? files -> "grouped"; + `; + const requiredGrammar = ` + = alice | bob; + = show files -> "required"; + `; + + it("matches without the optional owner (bare ?)", () => { + const g = loadGrammarRules("test.agr", ownerGrammar); + expect(testMatchGrammar(g, "show files")).toStrictEqual(["bare"]); + }); + + it("matches with the optional owner present (bare ?)", () => { + const g = loadGrammarRules("test.agr", ownerGrammar); + expect(testMatchGrammar(g, "show alice files")).toStrictEqual([ + "bare", + ]); + expect(testMatchGrammar(g, "show bob files")).toStrictEqual([ + "bare", + ]); + }); + + it("bare ? is equivalent to grouped ()?", () => { + const bare = loadGrammarRules("bare.agr", ownerGrammar); + const grouped = loadGrammarRules("grouped.agr", groupedGrammar); + for (const input of [ + "show files", + "show alice files", + "show bob files", + ]) { + expect(testMatchGrammar(bare, input)).toStrictEqual( + testMatchGrammar(grouped, input).map(() => "bare"), + ); + // grouped returns "grouped"; compare match success only + expect(testMatchGrammar(bare, input).length).toBe( + testMatchGrammar(grouped, input).length, + ); + } + }); + + it("required still rejects missing owner", () => { + const g = loadGrammarRules("test.agr", requiredGrammar); + expect(testMatchGrammar(g, "show files")).toStrictEqual([]); + expect(testMatchGrammar(g, "show alice files")).toStrictEqual([ + "required", + ]); + }); + + it("bare * matches zero or more owners", () => { + const g = loadGrammarRules( + "test.agr", + ` + = alice | bob; + = show * files -> "star"; + `, + ); + expect(testMatchGrammar(g, "show files")).toStrictEqual(["star"]); + expect(testMatchGrammar(g, "show alice files")).toStrictEqual([ + "star", + ]); + expect(testMatchGrammar(g, "show alice bob files")).toStrictEqual([ + "star", + ]); + }); + + it("bare + requires at least one owner", () => { + const g = loadGrammarRules( + "test.agr", + ` + = alice | bob; + = show + files -> "plus"; + `, + ); + expect(testMatchGrammar(g, "show files")).toStrictEqual([]); + expect(testMatchGrammar(g, "show alice files")).toStrictEqual([ + "plus", + ]); + expect(testMatchGrammar(g, "show alice bob files")).toStrictEqual([ + "plus", + ]); + }); + }, +); From f52a91d538858647e8777ae1566e6dfd33167166 Mon Sep 17 00:00:00 2001 From: typeagent-bot Date: Thu, 30 Jul 2026 21:02:52 +0000 Subject: [PATCH 2/4] style: apply prettier formatting and policy fixes --- ts/packages/actionGrammar/test/optionalRuleRef.spec.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts index 74fe3d51da..0ed901a26d 100644 --- a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts +++ b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts @@ -14,10 +14,7 @@ import { loadGrammarRules } from "../src/grammarLoader.js"; import { writeGrammarRules } from "../src/grammarRuleWriter.js"; import { describeForEachMatcher } from "./testUtils.js"; -function defNamed( - ast: ReturnType, - name: string, -) { +function defNamed(ast: ReturnType, name: string) { return ast.definitions.find((d) => d.definitionName.name === name)!; } From 619e2a5c82c785ae9f32e3a880a0a8091659f563 Mon Sep 17 00:00:00 2001 From: Dominic Nguyen Date: Thu, 30 Jul 2026 16:54:50 -0700 Subject: [PATCH 3/4] test: lock non-breaking guards for bare rule-ref quantifiers - Keep grouped (), required refs, and (the)? string groups unchanged - Reject wrong tails when optional polite is skipped or taken --- .../test/optionalRuleRef.spec.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts index 0ed901a26d..b7cefe741a 100644 --- a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts +++ b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts @@ -194,5 +194,92 @@ describeForEachMatcher( "plus", ]); }); + + // --- Non-breaking / additive-only guards --- + + it("grouped ()? is unchanged (pre-existing supported form)", () => { + const g = loadGrammarRules("test.agr", groupedGrammar); + expect(testMatchGrammar(g, "show files")).toStrictEqual([ + "grouped", + ]); + expect(testMatchGrammar(g, "show alice files")).toStrictEqual([ + "grouped", + ]); + expect(testMatchGrammar(g, "show charlie files")).toStrictEqual([]); + }); + + it("required rule refs without quantifiers stay required", () => { + const g = loadGrammarRules( + "test.agr", + ` + = outlook | teams | edge; + = open -> "app"; + `, + ); + expect(testMatchGrammar(g, "open")).toStrictEqual([]); + expect(testMatchGrammar(g, "open outlook")).toStrictEqual(["app"]); + expect(testMatchGrammar(g, "open notepad")).toStrictEqual([]); + }); + + it("pre-existing (the)? string groups are unchanged", () => { + const g = loadGrammarRules( + "test.agr", + ` + = open (the)? output panel -> "panel"; + `, + ); + expect(testMatchGrammar(g, "open output panel")).toStrictEqual([ + "panel", + ]); + expect(testMatchGrammar(g, "open the output panel")).toStrictEqual([ + "panel", + ]); + expect(testMatchGrammar(g, "open a output panel")).toStrictEqual([]); + }); + + it("group quantifiers ()? / ()* / ()+ stay independent of bare-ref fix", () => { + const g = loadGrammarRules( + "test.agr", + ` + = + mute (notifications)? -> "opt" + | tag (bug | feature)* -> "star" + | need (reviewer)+ -> "plus" + ; + `, + ); + expect(testMatchGrammar(g, "mute")).toStrictEqual(["opt"]); + expect(testMatchGrammar(g, "mute notifications")).toStrictEqual([ + "opt", + ]); + expect(testMatchGrammar(g, "tag")).toStrictEqual(["star"]); + expect(testMatchGrammar(g, "tag bug feature")).toStrictEqual([ + "star", + ]); + expect(testMatchGrammar(g, "need")).toStrictEqual([]); + expect(testMatchGrammar(g, "need reviewer")).toStrictEqual(["plus"]); + }); + + it("wrong tails still fail when an optional rule ref is skipped or taken", () => { + const g = loadGrammarRules( + "test.agr", + ` + = please | can you; + = ? delete the production database -> "ok"; + `, + ); + expect( + testMatchGrammar(g, "delete the production database"), + ).toStrictEqual(["ok"]); + expect( + testMatchGrammar(g, "please delete the production database"), + ).toStrictEqual(["ok"]); + expect( + testMatchGrammar(g, "please delete the staging database"), + ).toStrictEqual([]); + expect( + testMatchGrammar(g, "open the production database"), + ).toStrictEqual([]); + }); }, ); From e0aaee3cd34e489b0ff2ed84a05654a634c59c3c Mon Sep 17 00:00:00 2001 From: typeagent-bot Date: Thu, 30 Jul 2026 23:57:42 +0000 Subject: [PATCH 4/4] style: apply prettier formatting and policy fixes --- ts/packages/actionGrammar/test/optionalRuleRef.spec.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts index b7cefe741a..9691ee4afd 100644 --- a/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts +++ b/ts/packages/actionGrammar/test/optionalRuleRef.spec.ts @@ -234,7 +234,9 @@ describeForEachMatcher( expect(testMatchGrammar(g, "open the output panel")).toStrictEqual([ "panel", ]); - expect(testMatchGrammar(g, "open a output panel")).toStrictEqual([]); + expect(testMatchGrammar(g, "open a output panel")).toStrictEqual( + [], + ); }); it("group quantifiers ()? / ()* / ()+ stay independent of bare-ref fix", () => { @@ -257,7 +259,9 @@ describeForEachMatcher( "star", ]); expect(testMatchGrammar(g, "need")).toStrictEqual([]); - expect(testMatchGrammar(g, "need reviewer")).toStrictEqual(["plus"]); + expect(testMatchGrammar(g, "need reviewer")).toStrictEqual([ + "plus", + ]); }); it("wrong tails still fail when an optional rule ref is skipped or taken", () => {