Suggestion: Use an explicit return type. Otherwise, TS will implicitly union all the return statement types and wouldn't prevent from accidentally returning the wrong type.

https://techhub.social/@develwithoutacause/111842243141199473

Getting the factory function types is actually trickier than it seems and is cross-cutting across all tools (including ones not included in this change) so I don't want to include it in this PR.

Consider: Should this message contain the port number too?

These errors are worded around how the agent should correct its argument for the next call, the port is not needed for that. Also presumably the agent should have all the ports already reported from all the start commands.

Consider: Might be better to leave this in devserver-stop.ts since it specifically relates to devserver functionality.

It's used for both stop and wait_for_build so I wanted it in a central place. I did move it to devserver.ts though.

Consider: Do these helpers need to be exported? Will they ever realistically be called outside this file if resolveWorkspaceAndProject is doing most of the work?

Going a step further, if these errors are only created in one place, would it be more clear to just inline them at their individual call sites?

Inlined them.

Suggestion: I don't think we need the Input suffix here, I'd just call these workspacePath and projectName.

If the goal is to have a local variable with the same name, you can do that by aliasing in the destructure without altering the public API of the function.

function func({foo: bar}: {foo: string}): string {
    const foo = bar;
    return foo;
}

func({foo: 'test'});

I wanted to separate them because the input varieties are 1-to-1 with the function input, so for example they could be missing, while the non-input versions are non-nullable results of looking the inputs up.

Suggestion: I'm a bit wary of this utils file getting a bit long and have a nebulous purpose. Could we split it up into separate (or at least rename to a well-defined) domain-specific file(s)? Something like projects.ts or workspaces.ts? Could also combined shared-options.ts into the same file.

Split into a separate workspace-utils.ts file, now only some small stuff remained in utils.ts.

Consider: Can/should we use async APIs here for improved parallelism? Not sure how much of a concern multiple MCP requests would be.

I don't think it's critical here, and if anything I feel like if this becomes noticeable, some sort of caching would be a better direction.

Consider: Can we split this implementation into separate resolveWorkspace and resolveProject functions? I think this would make the internals easier to follow as we can leverage early returns rather than stateful local variables.

My original implementation was split! But then I realized (1) practically all tools want them both together, and (2) it would lead to duplicate lines of code running. So batched them together for now.

Consider: Do we really care about this as a distinct error case from ${workspacePathInput}/angular.json as checked below? I'm not sure this is worth the maintenance effort / performance cost of an additional stat call.

I wanted very precise error messages for the agent. I think it also helps clarify that the correct input is a workspace path and not an angular.json path. Ack regarding the cost but I don't think the added check is significant here.

Consider: Is this option just for testing purposes? Can we drop it from the public API or at least make it more clearly not for general usage?

I'm thinking you can either directly depend on AngularWorkspace.load and then spy on it in the test, or define this as a test only parameter like:

function resolveWorkspaceAndProject(
  {/* regular options */}: {/* ... */},
  testOnlyOptions: {
    workspaceLoader = AngularWorkspace.load,
  } = {},
): Promise<{ /* ... */}>;

Done, now spying on it.