fix: delegate get input to orchestrator tool#132
fix: delegate get input to orchestrator tool#132haifeng-li-at-salesforce merged 11 commits intoforcedotcom:mainfrom
Conversation
There was a problem hiding this comment.
have you considered rather than embedding input logic in the orchestrator, keeping it in the node itself?
here is a spike I did on "Guidance-only" nodes - it might be a bit verbose but the idea is still encapsulating the guidance within the responsible node rather than have the orchestrator be responsible for everything
jhorst11@265265c#diff-26a7cf13d2b4b7ead6b447baa60686a300769ec0ad50a81d81c36ee4d7a70c5d
There was a problem hiding this comment.
I think we'll want to replicate the pattern of direct-guidance nodes beyond just input so I'm hestitant to embed this logic directly into the orchestrator tool
There was a problem hiding this comment.
Totally agree. Overloading the orchestrator tool is not a good idea. This logic should live at the node level so it can be applied consistently across all nodes in the future. I’m thinking each node could support two modes: a direct guidance mode and a tool delegation mode.
There was a problem hiding this comment.
The following changes have been implemented:
-
Added a direct guidance mode alongside the existing delegation mode. In direct guidance mode, the guidances are handled directly by the orchestrator.
-
Introduced
NodeGuidanceDatafor direct guidance mode, serving as the counterpart to MCPToolInvocationData. -
Added an
exampleOutputfield to NodeGuidanceData to improve LLM compliance and response consistency.
0ed63e1 to
dbb6f8b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 88.25% 88.29% +0.04%
==========================================
Files 165 165
Lines 7059 7119 +60
Branches 994 981 -13
==========================================
+ Hits 6230 6286 +56
- Misses 829 833 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| You are participating in a workflow orchestration process. The orchestrator is providing | ||
| you with direct guidance for the current task. | ||
|
|
||
| # TASK GUIDANCE |
There was a problem hiding this comment.
Unless there's specific evaluation that suggests the guidance should come first, I'd prefer that we put INPUT SCHEMA and INPUT DATA before TASK GUIDANCE. It's typical practice to put the context section of a prompt earlier than the guidance itself, so that the LLM doesn't have to "go backwards" and reconsider semantics it already considered, through the lens of the "new info" that is its inputs. It just simplifies the logical processing of the semantics of the prompt.
There was a problem hiding this comment.
The INPUT SCHEMA and INPUT DATA sections are removed from createDirectGuidancePrompt to reduce token usage without losing any functional information, since generateTaskGuidance is designed to be self-contained and already includes all the context the LLM needs. The test result looks solid.
There was a problem hiding this comment.
They are not removed at the moment—they are 4 lines down from here. :) My suggestion here was just to define them before the TASK GUIDANCE section, instead of after.
There was a problem hiding this comment.
I am not sure what happens. But it is removed in my branch
I understand your suggestions. However, after further investigation, it turns out that both INPUT_SCHEMA and INPUT_DATA are redundant.
There was a problem hiding this comment.
I understand why the changes aren’t visible. In the top-left selection box, choose “Changes from all commits” to see the latest updates.
There was a problem hiding this comment.
Ah, got it. I don't disagree that I think we can live without it. I was just confused because it still seemed to be there. Seems fine in the latest version.
… OrchestratorTool
There was a problem hiding this comment.
I'm not clear on the design pattern here. Why are we delegating the initiation of guidance creation to the orchestrator? What part is it meant to play in owning that process? Why would we not simply seed NodeGuidanceData.taskGuidance with each node's expression of guidance.
This data structure is making a round trip back to the node that's responsible for generating guidance anyway. I'm not seeing the value in that process.
There was a problem hiding this comment.
Actually, we're not even delegating responsibility to the orchestrator: we're just delegating our node class's API to a different data structure. That doesn't pass muster to me. I don't actually see value in codifying the way that nodes create guidance. I think each node can be left to its own devices as to how it needs to formulate the guidance it sends along. There is no value that I can see in a shared commonality here.
There was a problem hiding this comment.
The data flow is as following:
-
Service (e.g., GetInputService) calls
metadata.generateTaskGuidance!(input)to get the guidance string -
Service populates
NodeGuidanceData.taskGuidancewith that string -
Service calls
interrupt(nodeGuidanceData)- sending everything to the orchestrator -
Orchestrator receives the pre-populated
NodeGuidanceDataand wraps it with workflow continuation instructions via createDirectGuidancePrompt()
The orchestrator simply reads taskGuidance from the interrupt data - it doesn't generate it.
There was a problem hiding this comment.
The generateTaskGuidance function exists as a single source of truth for guidance generation that can be used by both execution modes:
-
Direct Guidance Mode (via Services): The service calls metadata.generateTaskGuidance() and populates NodeGuidanceData.taskGuidance before interrupting
-
Delegate Mode (via Tools): When the tool is invoked as a separate MCP tool, it calls metadata.generateTaskGuidance() directly:
There was a problem hiding this comment.
Once all remaining delegated tools (five in total) are removed, we can replace WorkflowToolMetadata with NodeGuidanceMetadata, which will live in the Node/Service.
There was a problem hiding this comment.
The generateTaskGuidance function exists as a single source of truth for guidance generation that can be used by both execution modes:
My argument is that it doesn't serve sufficient purpose to create such an abstraction. The creation of guidance lives pretty concretely with the code base that implements a given agent, be it a node, service, or MCP tool. These agentic foundations are mutually exclusive from each other, which is the only scenario I could think of where we may want to consolidate this guidance.
And the reason that I'm looking for a justification of it, is to offset the penalty for the implementation: a code maintainer has to follow code around through a few different passage points to find the source of this guidance, when logically speaking you'd expect to find it "close to" the agent code base that defines it.
I think it's telling that we're not implementing this all across our existing nodes, services, and MCP tools as part of this refactor, and yet the sky is not falling from failing to do so. This to me is an abstraction that does not benefit the code base—only obfuscates it unnecessarily.
There was a problem hiding this comment.
That makes total sense. I agree that the recommendation is much more intuitive; my current implementation is a bit counter-intuitive. I’ll refactor the code to move guidance creation from tool metadata into the Agent class (node/service). This change will apply specifically to the get Input and extract input services.
There was a problem hiding this comment.
@khawkins The following code changes have been made. Please take another look and let me know your thoughts. Thanks!
- Moving guidance generation logic from metadata classes into the services themselves (
InputExtractionServiceandGetInputService) - Simplifying the architecture - the
metadata.tsbase class no longer has tool-specific functions; guidance generation is now handled directly by the services
khawkins
left a comment
There was a problem hiding this comment.
This looks great overall! I'd like to answer the questions around generateTaskGuidance and the minor one around the ordering in the direct guidance prompt in the orchestrator. But I think overall this is solid.
…putExtractionService
…nctions from metadata class and consolidating logic in InputExtractionService and GetInputService
khawkins
left a comment
There was a problem hiding this comment.
This looks good to me!
79eb1bd
into
forcedotcom:main
Changes
Files Changed
What issues does this PR fix or reference?
it fixes the getInput off-trail issues