fix(js/core): Dap Action updates #4050
Conversation
Summary of ChangesHello @ifielker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Dynamic Action Provider (DAP) to align with new architectural requirements, specifically that DAP actions must accept no input and return an array of 'ActionMetadata'. This change impacts how action data is fetched, cached, and traced, leading to a necessary double invocation of the action provider in some cases. The modifications ensure compliance with the updated API contract while acknowledging a minor risk of data inconsistency between tracing and actual usage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dynamic Action Provider (DAP) action to align with new requirements, specifically to take no input and return an array of ActionMetadata. This change correctly necessitates calling the action provider twice, which is a noted trade-off for satisfying the new contract. The changes are well-reflected in the updated tests. My review includes a suggestion to improve the robustness and conciseness of the transformDapValue function.
| { | ||
| ...cfg, | ||
| inputSchema: z.any(), | ||
| outputSchema: z.any(), |
There was a problem hiding this comment.
Please define ActionMetadataSchema and make this z.array(ActionMetadataSchema)
We probably want to have a shared (across languages) definition of ActionMetadataSchema, so make sure it's defined in
| registry, | ||
| { | ||
| ...cfg, | ||
| inputSchema: z.any(), |
| if (!params?.skipTrace) { | ||
| // Also run the action | ||
| // This action actually does nothing, with the important side | ||
| // effect of logging its input and output (which are the same). | ||
| // It does not change what we return, it just makes | ||
| // the content of the DAP visible in the DevUI and logging trace. | ||
| await this.dap.run(transformDapValue(this.value)); | ||
| // The action goes and gets the same data again, but it | ||
| // doesn't cache it. (It doesn't have access to the Cache and | ||
| // we are not allowed pass it in). | ||
| // We are not allowed to pass in either the DapValue we just got or | ||
| // the cache because the action input MUST be void | ||
| // and the output MUST be an array of ActionMetadata. | ||
| // And we can't construct the DapValue (which contains full Actions) | ||
| // from just the ActionMetadata. | ||
| // So unfortunately we need to fetch from the DAP twice, and there's | ||
| // a small chance that what gets traced is not what gets cached/used. | ||
| await this.dap.run(); | ||
| } |
There was a problem hiding this comment.
why not avoid duplicate dapFn call by doing something like:
if (params?.skipTrace) {
this.value = await this.dapFn();
} else {
this.value = await this.dap.run();
}There was a problem hiding this comment.
dapFn returns a DapValue like { tools: [<tool1Action>, <tool2Action>], resources: [<resource1Action>, <resource2Action>]}. dap.run() gets a new DapValue, flattens it and maps the actions to their metadata only. like this: [<tool1MetadataOnly>,<tool2MetadataOnly>,<resource1MetadataOnly>,<resource2MetadataOnly>]. I can't get the actions back from just the metadata. And I can't access the cache from the action, and I can't pass in either the DapValue or the cache to the action... so yeah. And we can't just save the metadata because it's an ActionProvider, not an ActionMetdataProvider. There's a getAction which needs to return the whole action.
There was a problem hiding this comment.
Unless you optionally let me pass in the cache? like if it's there, save it to cache, but if not then, ignore it. So whatever is calling the dap.run() is not required to pass anything, but it can? Or is that "breaking the contract"?
There was a problem hiding this comment.
I'm not sure I understand how cache is relevant to my question... Let me rephrase:
Right now you are doing:
this.value = await this.dapFn();
if (!params?.skipTrace) {
// this calls dapFn again, but you ignore the output.
await this.dap.run();
}This would do the same thing without calling dapFn twice:
if (params?.skipTrace) {
this.value = await this.dapFn();
} else {
this.value = await this.dap.run();
}Cache is not involved in that logic as far as I can see, no?
There was a problem hiding this comment.
Did you not read my response? this.value is type DapValue. this.dap.run() does NOT return a DapValue and we cannot construct a DapValue from the array of Metadata. so this.value = await this.dap.run(); is Type mismatch. It will not build. Maybe this will be more clear:
if (params?.skipTrace) {
this.value = await this.dapFn(); // returns DapValue - a struct containing Actions
} else {
this.value = await this.dap.run(); // TYPE MISMATCH: returns ActionMetadata[].
}
so unless there is some magic:
const metadataArray = await this.dap.run();
this.value = magicallyReconstructTheActionFromMetadataAlone(metadataArray);
Then we can't do this.
There was a problem hiding this comment.
I see, I missed the "metadata only" part and then cache confused me further.
You can easily access the cache from inside the action with a simple closure:
export function defineDynamicActionProvider(...): DynamicActionProviderAction {
//...
const cache = new SimpleCache(...);
const a = defineAction(
registry,
{ /* ... */ },
async (_options) => {
// access `cache` here
}
);
implementDap(a as DynamicActionProviderAction, cfg, fn, cache);
return a as DynamicActionProviderAction;
}There was a problem hiding this comment.
Well, it's not quite that simple, but sure.
pavelgj
changed the title
2 participants
New requirements that the DAP action takes no input and returns an ActionMetadata array are now satisfied.
Unfortunately this means that we need to call the Action Provider twice instead of once. It also means
there is a slight possibility that what is in the trace and what is being used is different.
Checklist (if applicable):