Skip to content

feat: project evaluation scoped tool - #14

Merged
zoedsoupe merged 14 commits into
mainfrom
feat/project-eval-tool
Sep 10, 2025
Merged

feat: project evaluation scoped tool#14
zoedsoupe merged 14 commits into
mainfrom
feat/project-eval-tool

Conversation

@zoedsoupe

Copy link
Copy Markdown

No description provided.

@zoedsoupe

zoedsoupe commented Sep 9, 2025

Copy link
Copy Markdown
Author

for now its failing since we're not setting up the IPC communication channel properly

we could try to communicate directly via STDIO but i think that's kinda tacky since node.js already handles IPC natively

@zoedsoupe zoedsoupe self-assigned this Sep 9, 2025
@zoedsoupe zoedsoupe linked an issue Sep 9, 2025 that may be closed by this pull request
@zoedsoupe
zoedsoupe requested a review from josevalim September 9, 2025 22:00
@zoedsoupe
zoedsoupe marked this pull request as ready for review September 9, 2025 22:00
@zoedsoupe

Copy link
Copy Markdown
Author

@josevalim arguments is a special value that refers to an array-like object that contains the values of all the arguments passed to that function when it was called, so to receive external arguments i needed to rename it to args

i also took the option to define it as a record, because it seems kinda strange the need of args[0] usage to access them when a named record is easier DX (AI-X in this case?) wdyt?

@zoedsoupe

Copy link
Copy Markdown
Author

hm some node specific version errors...

@josevalim

Copy link
Copy Markdown
Contributor

Do we want a separate execution environment? I'd say we actually don't want a separate environment, as the goal of project_eval is to run within the exact same instance that the web server is running.

arguments is a special value that refers to an array-like object that contains the values of all the arguments passed to that function when it was called, so to receive external arguments i needed to rename it to args

Couldn't we still use arguments somehow? Otherwise args is indeed better.

i also took the option to define it as a record, because it seems kinda strange the need of args[0] usage to access them when a named record is easier DX (AI-X in this case?) wdyt?

In any case, it should be an array, for consistency with the other implementations. It is important for the MCPs to be as consistent as possible.

Comment thread src/evalation/code_executor.ts Outdated
@jonatanklosko

Copy link
Copy Markdown
Contributor

Couldn't we still use arguments somehow? Otherwise args is indeed better.

We can have arguments as the MCP parameter, and args as the way to access it in the code. Hopefully with good description the LLM won't get confused? We could change to args everywhere but that's a breaking change for TC (we could send both, but that's not beautiful :p)

@josevalim

Copy link
Copy Markdown
Contributor

I am just wondering if we can do a trick like this:

const args = [1, 2, 3];
const code = "arguments[0] + arguments[1] + arguments[2]";
const result = eval(`(function() { return ${code}; })`).apply(null, args);

or:

const code = "return arguments[0] + arguments[1] + arguments[2]";
const fn = new Function(code);
const result = fn(1, 2, 3); // 6

Both seem to work on the repl.

@jonatanklosko

Copy link
Copy Markdown
Contributor

Oh, you are saying the actual arguments in that case have no use, so we may as well use those. Sounds good to me!

Btw. we want new Function over eval, since eval sees all variables in the scope, which we don't want.

@zoedsoupe

zoedsoupe commented Sep 10, 2025

Copy link
Copy Markdown
Author

We can have arguments as the MCP parameter

actually, neither as MCP parameter we can have since the input schema is derived from the type, so it complains that modules are automatically on strict mode ;-;

we need to go with args unfortunately

nevermind, we can rename the argument name internally, so for the MCP we have arguments and in our code args, but in the evaluated code it will be need to be args too

@zoedsoupe

Copy link
Copy Markdown
Author

Do we want a separate execution environment? I'd say we actually don't want a separate environment, as the goal of project_eval is to run within the exact same instance that the web server is running.

i think i didn't understand completely the concern. the evaluated code have the same scope of the current project (aka the web server too), so it can access all local modules and dependencies, although restricted with dynamic imports since we're evaluating as a function.

my main point here is security, it does have access to the project scope, although restricted. what is the real concern here? cc @josevalim

@zoedsoupe

Copy link
Copy Markdown
Author
const code = "return arguments[0] + arguments[1] + arguments[2]";
const fn = new Function(code);
const result = fn(1, 2, 3); // 6

i think we can expand the arguments array and pass it as separate ones to the function, would be that you're thinking on?

zoey added 3 commits September 10, 2025 10:26
…ave_javascript into feat/project-eval-tool

* 'feat/project-eval-tool' of github.com:tidewave-ai/tidewave_javascript:
  chore(deps-dev): bump vite from 7.1.2 to 7.1.5 in the npm_and_yarn group across 1 directory (#15)
Comment thread src/evalation/eval_worker.ts Outdated
Comment thread src/evalation/eval_worker.ts Outdated
@zoedsoupe
zoedsoupe requested a review from josevalim September 10, 2025 14:49
Comment thread src/tools.ts Outdated

@josevalim josevalim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, double check CI, and we are good!

Comment thread src/tools.ts Outdated
Comment thread src/tools.ts Outdated
Comment thread src/tools.ts Outdated
Comment thread test/integration.test.ts
@zoedsoupe
zoedsoupe force-pushed the feat/project-eval-tool branch from fad1673 to 40ec031 Compare September 10, 2025 15:36
@zoedsoupe
zoedsoupe merged commit 85c878c into main Sep 10, 2025
4 checks passed
@zoedsoupe
zoedsoupe deleted the feat/project-eval-tool branch September 10, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add project_eval tool

3 participants