Fix NFA compilation failure for grammars with shared optional prefixes#2629
Fix NFA compilation failure for grammars with shared optional prefixes#2629GeorgeNgMsft wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an @typeagent/action-grammar NFA compilation failure triggered by optimizer output for grammars whose top-level alternatives share a fully-optional common prefix (e.g. optional “please”), and aligns the agc default optimization preset with NFA compatibility (agent-server’s default grammar system).
Changes:
- Teach the NFA compiler to derive an implicit forwarding value for value-less multi-part rules when exactly one part is variable-bearing (and error clearly when ambiguous).
- Add an
nfaSafeOptimizationspreset (recommended minustailFactoring/promoteTailRulesParts) and switchagc’s default optimizations to that preset. - Add regression tests covering the repro, the still-unsupported
tailCallpath, and the ambiguous implicit-value guard.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ts/packages/actionGrammarCompiler/src/commands/compile.ts | Switch agc default optimizations to nfaSafeOptimizations for NFA compatibility. |
| ts/packages/actionGrammar/test/nfaFactoredPrefixValues.spec.ts | New regression tests for shared optional-prefix factoring, tailCall refusal, and ambiguous implicit value. |
| ts/packages/actionGrammar/src/nfaCompiler.ts | Derive implicit values for value-less multi-part rules with a single variable-bearing part; improve diagnostics. |
| ts/packages/actionGrammar/src/index.ts | Export nfaSafeOptimizations from the package entrypoint. |
| ts/packages/actionGrammar/src/grammarOptimizer.ts | Introduce the nfaSafeOptimizations preset and document why it exists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…es' into dev/georgeng/fix_nfa_with_subrules
Line-number shifts from the NFA fix broke file:line matches for pre-existing baselined offenders (nfaCompiler.ts, grammarOptimizer.ts). Regenerated via 'npm run code-complexity:update-exceptions'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| "Disable grammar optimizations (produces an unoptimized AST that preserves the 1:1 correspondence between top-level rules and the original source — useful for diagnostics).", | ||
| default: false, | ||
| }), | ||
| "nfa-safe": Flags.boolean({ |
There was a problem hiding this comment.
I kinda prefer not to introduce this flag because it indicate that this is the long term picture, instead of transient NYI. But if it is useful right now, make a TODO to make sure we get rid of it once NFA has paritity.
There was a problem hiding this comment.
This allows the nfa path to be used with excel agent (and any other similar scenario that triggers the problematic path) right now while keeping the optimizations, so we do need it. I have added a TODO for removal as suggested
| throw new Error( | ||
| `${describeRule()} has ${rule.parts.length} terms but no value expression, ` + | ||
| `and more than one part carries a variable - the implicit value is ambiguous. ` + | ||
| `Multi-term rules must have an explicit value expression (using ->).`, |
There was a problem hiding this comment.
From my recollection, not all rule needs to have a value. It is only an error if a rule without a value is assign to a value.
So this is not true?
There was a problem hiding this comment.
…om/microsoft/TypeAgent into dev/georgeng/fix_nfa_with_subrules
See issue #2499
Problem: Grammars where every top-level alternative starts with the same optional sub-rule (e.g. an optional "please" politeness prefix) failed to compile to an NFA. The default agc optimizer factored the shared prefix into a shape ( [, RulesPart] ) that the NFA compiler rejected, silently falling back to LLM translation for affected schemas (or logging loudly for dynamic grammars like excel's).
Root cause: Two separate incompatibilities between the AST-walking matcher and the NFA compiler:
Changes
• nfaCompiler.ts — The compiler now derives an implicit forwarding value for multi-term rules that have exactly one variable-bearing part, mirroring the matcher's existing implicit-default rule. Genuinely ambiguous rules (2+ variable-bearing parts) still fail with a clear error.
• grammarOptimizer.ts — Added a new nfaSafeOptimizations preset: all of recommendedOptimizations except tailFactoring / promoteTailRulesParts , which remain NFA-incompatible by design.
• actionGrammarCompiler/compile.ts — agc now defaults to nfaSafeOptimizations instead of recommendedOptimizations , since NFA is agent-server's default grammar system.
• New test ( nfaFactoredPrefixValues.spec.ts ) covering the exact repro from the bug report, the still-unsupported tailCall case, and the ambiguous-value guard.
Validation
• action-grammar suite: 73/73 suites, 16,144/16,144 tests passing.
• Full monorepo build succeeds, recompiling every agent grammar under the new default.
• Bulk NFA-compile sweep: 48/48 real compiled agent grammars now compile cleanly (previously several silently failed).