Add CacheVariables feature to TreeWalker by ilang898 · Pull Request #87 · microsoft/Forge

ilang898 · 2026-06-05T20:30:59Z

Summary

Adds a new CacheVariables property to TreeNode that enables declarative, node-scoped variable caching within workflow schemas. Schema authors can evaluate expressions after actions complete and bind results to named variables accessible in ShouldSelect routing expressions via the Cache object.

Problem

Current ShouldSelect expressions must reference action outputs inline with repeated Session.GetOutput() calls, leading to verbose, hard-to-read routing logic.

Solution

\\json
"CacheVariables": {
"scsSucceeded": "C#|Session.GetOutput("GetBypassSafetyCheckToken").Output.Succeeded",
"scsIsSafe": "C#|Session.GetOutput("GetBypassSafetyCheckToken").Output.IsSafe"
},
"ChildSelector": [
{ "ShouldSelect": "C#|Cache.scsSucceeded && Cache.scsIsSafe", "Child": "Next" }
]
\\

Key Design Decisions

Node-scoped: Cache is cleared at the start of each \VisitNode\ — no cross-node leakage, no persistence to ForgeState
Uses existing EvaluateDynamicProperty: Typed as \dynamic, evaluated the same way as \Properties, \Input, and \TreeInput\
JObject-backed Cache: Supports \Cache.x\ dot-notation in Roslyn expressions via \IDynamicMetaObjectProvider\
Fully backward compatible: Optional property (null by default), no changes to existing contracts or behavior

Files Changed

File	Change
\ForgeTree.cs\	Added \CacheVariables\ (dynamic) to \TreeNode\
\ExpressionExecutor.cs\	Added \Cache\ property, \SetCache/\ClearCache/\GetCache\ methods
\ITreeSession.cs\	Added \GetCache(string name)\ to interface
\TreeWalkerSession.cs\	Cache clear/evaluate/set in \VisitNode, \GetCache\ implementation
\ForgeSchemaValidationRules.json\	Added \CacheVariables\ to node definitions

Testing

10 new unit tests covering: static expressions, Session.GetOutput access, UserContext binding, node-scoped isolation, invalid expressions, multiple variables, string literals, GetCache API, backward compatibility
All 105 tests pass (95 original + 10 new)

Add CacheVariables property on TreeNode that evaluates Roslyn expressions after actions complete and makes results available in ShouldSelect via Cache.x. Key design: - Node-scoped: Cache is cleared at the start of each node visit - Uses EvaluateDynamicProperty: supports C#| expressions and string literals - Accessible as Cache.x in Roslyn expressions or Session.GetCache(name) - No persistence to ForgeState (ephemeral within node execution) Changes: - ForgeTree.cs: Add CacheVariables (Dictionary<string, string>) to TreeNode - ExpressionExecutor.cs: Add Cache (ExpandoObject) to CodeGenInputParams - ITreeSession.cs: Add GetCache(name) method - TreeWalkerSession.cs: Add ResolveCacheVariables, ClearCache in VisitNode - ForgeExceptions.cs: Add CacheVariableException - ForgeSchemaValidationRules.json: Add CacheVariables to node definitions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Simplify CacheVariables evaluation to match the existing pattern used by Properties, Input, and TreeInput. Instead of a custom ResolveCacheVariables method with per-variable iteration, CacheVariables is now typed as dynamic and evaluated via EvaluateDynamicProperty(treeNode.CacheVariables, null). Changes: - ForgeTree.cs: CacheVariables type changed from Dictionary<string,string> to dynamic - ExpressionExecutor.cs: Cache uses SetCache/ClearCache with JObject instead of ExpandoObject - TreeWalkerSession.cs: Removed ResolveCacheVariables, inline EvaluateDynamicProperty call - ForgeExceptions.cs: Removed CacheVariableException (uses existing EvaluateDynamicPropertyException) - Tests: Updated InvalidExpression test to expect EvaluateDynamicPropertyException Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- CacheVariables type: Dictionary<string,string> -> dynamic - Cache implementation: ExpandoObject -> JObject via EvaluateDynamicProperty - Error handling: CacheVariableException -> EvaluateDynamicPropertyException - Status: Failed_CacheVariable -> Failed_EvaluateDynamicProperty - Design decision 7.4: Updated rationale for JObject approach Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for node-scoped CacheVariables that are evaluated after a node’s actions complete and made available to selection logic (e.g., ShouldSelect) via a Cache dynamic object and a new Session.GetCache(...) API.

Changes:

Introduces CacheVariables on TreeNode and validates it in the schema rules.
Adds cache lifecycle management (clear per node, set after actions) and exposes cache access via ITreeSession.GetCache.
Adds unit tests covering common CacheVariables scenarios and backward compatibility.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Forge.TreeWalker/src/TreeWalkerSession.cs	Clears cache per node visit, evaluates `CacheVariables`, exposes `GetCache(name)` API.
Forge.TreeWalker/src/ITreeSession.cs	Adds `GetCache(string name)` to the public session interface.
Forge.TreeWalker/src/ExpressionExecutor.cs	Adds `Cache` to Roslyn globals and cache get/set/clear helpers.
Forge.TreeWalker/contracts/ForgeTree.cs	Adds `TreeNode.CacheVariables` contract field.
Forge.TreeWalker/contracts/ForgeSchemaValidationRules.json	Allows `CacheVariables` as an object of string expressions in relevant node schemas.
Forge.TreeWalker.UnitTests/test/TreeWalkerUnitTests.cs	Adds test coverage for `CacheVariables` behavior.
Forge.TreeWalker.UnitTests/test/ForgeSchemaHelper.cs	Adds schema fixtures for `CacheVariables` tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+        [DataMember]
+        public dynamic CacheVariables { get; set; }


- GetCache: Use TryGetValue for safe null-return on missing keys (JObject and IDictionary) - ForgeTree.cs: Change CacheVariables to private set for contract immutability - ForgeSchemaHelper.cs: Fix comment to reference EvaluateDynamicPropertyException Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…y JObject - Add Newtonsoft.Json.Linq.JObject assembly reference to Roslyn script options so dynamic member access on the Cache JObject resolves at runtime - Initialize Cache to empty JObject (not null) to prevent NullReferenceException when ShouldSelect references Cache on nodes without CacheVariables - ClearCache resets to empty JObject instead of null Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

TravisOnGit · 2026-06-06T00:05:38Z

            TreeNode treeNode = this.Schema.Tree[treeNodeKey];

+            // Clear node-scoped cache variables at the start of each node visit.
+            this.expressionExecutor.ClearCache();


Let's consider adding this ClearCache call directly after awaiting SelectChild. Doing that would definitely lock it down to only being used during SelectChild.

Having ClearCache at the top of VisitNode here means that AfterVisitNode would also have access to the Cache. And also if folks directly call VisitNode (without WalkTree), the Cache would be maintained after returning VisitNode, so app (or UTs) could then access the Cache as well.

Let's discuss offline if there are valid use-cases or any drawbacks to calling ClearCache at the beginning of VisitNode vs at the end.

TravisOnGit · 2026-06-06T00:06:41Z


+            // Evaluate CacheVariables after actions complete, before selecting child.
+            // Uses the same EvaluateDynamicProperty pattern as Properties/Input.
+            object cacheResult = await this.EvaluateDynamicProperty(treeNode.CacheVariables, null).ConfigureAwait(false);


nit: Let's add this after the if Leaf statement. Since it won't be needed if that returns.

TravisOnGit · 2026-06-06T00:18:26Z

+            // Evaluate CacheVariables after actions complete, before selecting child.
+            // Uses the same EvaluateDynamicProperty pattern as Properties/Input.
+            object cacheResult = await this.EvaluateDynamicProperty(treeNode.CacheVariables, null).ConfigureAwait(false);
+            this.expressionExecutor.SetCache(cacheResult);


Idea to have a SetCache method in TreeWalkerSession. It would do these 2 operations, EvaluateDynamicProperty and SetCache in ExpressionExecutor.

The benefit of having a SetCache method is for UTs that validate schemas by evaluating all the expressions. For example, a UT that evaluates the ShouldSelect statements by calling EvaluateDynamicProperty. With the new Cache feature, ShouldSelect statements would fail unless the Cache is filled with the expected values. So we need some way to update the Cache with expected values. Something like:
await treeWalkerSession.SetCache(forgeTreeNode.CacheVariables); await treewalkerSession.EvaluateDynamicProperty(childSelector, typeof(bool));

TravisOnGit · 2026-06-06T00:20:33Z

+        /// </summary>
+        /// <param name="name">The variable name as defined in CacheVariables.</param>
+        /// <returns>The cached value if it exists, otherwise null.</returns>
+        public object GetCache(string name)


Is this called anywhere? I don't understand the need for this method.

TravisOnGit · 2026-06-06T00:32:05Z

                Session = session,
-                TreeInput = treeInput
+                TreeInput = treeInput,
+                Cache = new Newtonsoft.Json.Linq.JObject()


TreeInput is already dynamic and is handled. Can we have Cache treated similarly? I don't think we need to set it as a JObject or add an additional assembly for that.

I think setting to null here is fine. If folks accidentally try to use the Cache (from ShouldSelect) when there's nothing inside (Cache in the schema), it would throw a NullReferenceException, which should be pretty clear that it's because there's no Cache defined (in the schema). Versus if we set this Cache to a default Object instead, it would throw a more confusing exception in the same scenario.

TravisOnGit · 2026-06-06T00:33:17Z

+        /// </summary>
+        public void ClearCache()
+        {
+            this.parameters.Cache = new Newtonsoft.Json.Linq.JObject();


I think setting to null here is fine.

TravisOnGit · 2026-06-06T00:35:14Z

+        /// Gets the Cache object (a JObject with evaluated CacheVariable values, or null).
+        /// </summary>
+        /// <returns>The cache object.</returns>
+        public dynamic GetCache()


I think this can return "object" type instead of dynamic. I see that GetOrCommitTreeInput returns object.

TravisOnGit · 2026-06-06T00:46:42Z

+        /// Cache variables are scoped to the current node only — they are cleared when moving to the next node.
+        /// </summary>
+        [DataMember]
+        public dynamic CacheVariables { get; private set; }


Consider just "Cache" for the name.

There's an argument that "CacheVariables" is more clear to what it does. At first glance at a schema, who's to say what a "Cache" could represent..
"Variables" takes forever to say though, 😄.. Since this is what is shown to the user in the schema, let's brainstorm best name. Here's some ideas:

Cache

CacheVariables

CacheVars

TreeNodeCache

TravisOnGit · 2026-06-06T00:53:08Z

                "Properties": {
                    "type": "object"
+                },
+                "CacheVariables": {


I think this is right, setting type as object and setting patternProperties. This should effectively enforce it is used like a dictionary with named, string properties. As opposed to "Properties" above that's simply an object. It can be set to a dynamic object, a string, an int, a dictionary, etc..

In theory, we could also set CacheVariables to object type like Properties. It would be more flexible/versatile for users, like if they want to simply set a single value instead of using the named properties. But I think I like the guardrails of enforcing a dictionary-style pattern. It'll make it more consistent and less error-prone.

TravisOnGit · 2026-06-06T00:55:35Z

+                ""Tree"": {
+                    ""Root"": {
+                        ""Type"": ""Action"",
+                        ""Actions"": {


Recommend removing the Actions and just making these Selection TreeNode types, unless you are utilizing the ActionResponse. Only include things that contribute to each UT in some way and cut the fluff please.

TravisOnGit · 2026-06-06T00:58:30Z

+                    ""SecondNode"": {
+                        ""Type"": ""Selection"",
+                        ""CacheVariables"": {
+                            ""secondNodeCheck"": ""C#|Session.GetCache(\""firstNodeVar\"") == null ? \""isolated\"" : \""leaked\""""


Session.GetCache I don't believe is a use-case. You should not be able to access the Cache from inside the Cache. Please remove this.

TravisOnGit · 2026-06-06T01:10:29Z

+        /// <summary>
+        /// CacheVariables with string literal (non-Roslyn value).
+        /// </summary>
+        public const string CacheVariables_StringLiteral = @"


More UT ideas:

CacheVariables_IsString_ExpectException - Set the Cache directly to a string instead of a dictionary, like: ""Cache"": ""value"". Maybe put it in a separate InvalidTestSchemas and confirm exception is thrown. Or maybe a forge schema validation test would be appropriate to ensure schema validation catches that issue.

Set a CacheVariable value as an object, like ActionResponse. Then confirm ShouldSelect can use it like Cache.DiagnosticsActionResponse.Status

Set a CacheVariable value as a boolean expression like Session.GetOutput(""Root_CollectDiagnosticsAction"").Status == ""Success"". Then confirm ShouldSelect can use it like Cache.IsSuccess.

Using all the TreeNode Types, like Selection, Leaf, and Subroutine. Just simple tests to confirm Cache works on all the types.

Have 2 TreeNodes that have the same exact named property in their Cache. Confirm that the second TreeNode's ShouldSelect statement contains its expected value. This is a good confirmation that the Cache truly gets wiped.

TravisOnGit · 2026-06-06T01:12:44Z

+                        ""CacheVariables"": {
+                            ""a"": ""C#|10"",
+                            ""b"": ""C#|20"",
+                            ""sum"": ""C#|30""


nit: It's minor, but it could lead to confusion. You list variables, "a", "b", and "sum". This could imply that you could calculate sum by USING what you just set in a and b. But this is not possible, you can't view what's in the Cache from CacheVariables. So just to avoid that possible confusion, can you change the example?

TravisOnGit

Left feedback on iteration 7. Please have a look, thanks!

Ian Lang and others added 3 commits June 4, 2026 17:45

Copilot AI review requested due to automatic review settings June 5, 2026 20:31

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Comment thread Forge.TreeWalker/src/TreeWalkerSession.cs

Comment thread Forge.TreeWalker/contracts/ForgeTree.cs Outdated

Comment on lines +68 to +69

[DataMember]

public dynamic CacheVariables { get; set; }

Comment thread Forge.TreeWalker.UnitTests/test/ForgeSchemaHelper.cs

Ian Lang and others added 4 commits June 5, 2026 13:35

Merge branch 'master' into feature/cache-variables

637f2bf

Remove design document from PR

56a1bcb

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

TravisOnGit reviewed Jun 6, 2026

View reviewed changes

Conversation

ilang898 commented Jun 5, 2026

Summary

Problem

Solution

Key Design Decisions

Files Changed

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisOnGit left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants