feat: CP-955 migrate ProjectErrorsTool #3688

samueld-activator · 2025-07-17T16:19:44Z

No description provided.

cmd/state-mcp/internal/toolregistry/tools.go

pkg/platform/api/graphql/graphql.go

… was fixed

Naatan · 2025-07-18T18:48:42Z

cmd/state-mcp/internal/toolregistry/tools.go

+		),
+		Handler: func(ctx context.Context, p *primer.Values, mcpRequest mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+			namespace, _ := mcpRequest.RequireString("namespace")


Don't throw away errors.

Naatan · 2025-07-18T18:53:47Z

cmd/state-mcp/internal/toolregistry/tools.go

+			ns, err := project.ParseNamespace(namespace)
+			if err != nil {
+				return mcp.NewToolResultError(fmt.Errorf("invalid namespace format. Use 'owner/project' format: %w", err).Error()), nil


Suggested change

return mcp.NewToolResultError(fmt.Errorf("invalid namespace format. Use 'owner/project' format: %w", err).Error()), nil

return mcp.NewToolResultError(fmt.Sprintf("Could not parse namespace: %s", errs.JoinMessage(err))), nil

Don't make assumptions about what the underlying errors is, the error it returns should (and does) give the format error. But it doesn't necessarily have to be a format error.

No point passing it through Errorf() if you're just going to grab the message at the end.

Always use errs.JoinMessage(err) or equivalent to get all underlying errors. You don't know if the direct error you're receiving has the actual cause of the issue in it.

Naatan · 2025-07-18T18:54:19Z

cmd/state-mcp/internal/toolregistry/tools.go

+			err = runner.Run()
+			if err != nil {
+				return mcp.NewToolResultError(fmt.Errorf("error executing GraphQL query: %v", err).Error()), nil


Suggested change

return mcp.NewToolResultError(fmt.Errorf("error executing GraphQL query: %v", err).Error()), nil

return mcp.NewToolResultError(fmt.Sprintf("error executing GraphQL query: %s", errs.JoinMessage(err))), nil

Naatan · 2025-07-18T18:56:37Z

internal/runners/mcp/projecterrors/projecterrors.go

+)
+
+type ProjectErrorsRunner struct {
+	primer    *primer.Values


Please record the underlying values here. The primers use-case is passing large amount of context. This runner only needs some of that context, which it should specify with an interface.

See for example:

cli/internal/runners/hello/hello_example.go

Line 55 in a293c02

return &Hello{

Added output, auth and svcModel, which are coming from the primer

Naatan · 2025-07-18T18:58:15Z

internal/runners/mcp/projecterrors/projecterrors.go

+			Name:      artifact.Name(),
+			Version:   artifact.Version(),
+			Namespace: artifact.Ingredients[0].Namespace,
+			LogURL:    artifact.LogURL,
+			WasFixed:  artifact.Revision() < int(*ingredient.Revision),


This is called FailedIngredient but other than the namespace is talking about an artifact. We should clearly differentiate between the two. Ingredient names and Artifact names are not always direct mappings.

I renamed it to FailedArtifact

Naatan · 2025-07-18T19:05:41Z

internal/runners/mcp/projecterrors/projecterrors.go

+		ingredient, err := model.GetIngredientByNameAndVersion(
+			artifact.Ingredients[0].Namespace, artifact.Name(), artifact.Version(), nil, runner.primer.Auth())
+		if err != nil {
+			return fmt.Errorf("error searching ingredient: %w", err)
+		}


This might be slow if you have a lot of failing artifacts. Probably fine for a first iteration, but might make sense to use hasura here to query multiple ingredients at the same time.

Yes I thought about that too. For now it doesn't seem to bad. I will add a note so we don't forget about it.

Naatan · 2025-07-18T19:08:41Z

internal/runners/mcp/projecterrors/projecterrors.go

+	// Check if ingredients are failing due to missing dependencies.
+	err = CheckDependencyErrors(&failedIngredients)
+	if err != nil {
+		return fmt.Errorf("error checking dependency errors: %w", err)
+	}


I think you should pass this failedArtifacts. You're not relying on WasFixed which is the only piece of meta info that failedIngredients has that failedArtifacts doesn't.

That way you can call this earlier, before failedIngredients is set, and you're not waiting on remotely pulling in all relevant ingredients before you error out on an unrelated error.

I see this is adding context to the failedIngredients, but you could always have this return a map that you then use to associate it to ingredients further down.

Naatan · 2025-07-18T19:09:21Z

internal/runners/mcp/projecterrors/projecterrors.go

+	var wg sync.WaitGroup
+	var mu sync.Mutex


Ah, now you're getting to the good stuff! ;)

Naatan · 2025-07-18T19:11:05Z

internal/runners/mcp/projecterrors/projecterrors.go

+				return
+			}
+			ingredient.IsDependencyError = depError
+		}(&(*failedIngredients)[i])


Not sure I follow why this is creating a new pointer? Could conceivably just be

Suggested change

}(&(*failedIngredients)[i])

}(failedIngredients[i])

I get "invalid operation: cannot index failedArtifacts (variable of type *[]FailedArtifact)"

failedArtifacts is a pointer, and this is why I can't index it. I need to dereference using *, but I can do that before "range artifacts" and use &artifacts[i] here

Naatan · 2025-07-18T19:14:41Z

internal/runners/mcp/projecterrors/projecterrors.go

+var MissingDependencyKeywords = map[string][]string{
+	"language/php": {
+		"require_once(): Failed opening required",
+		"include(): Failed opening",
+		"require(): Failed opening required",
+		"include_once(): Failed opening",
+		"Fatal error: Uncaught Error: Class",
+		"Class not found",
+	},
+	"language/perl": {
+		"Can't locate",
+		"in @INC",
+		"BEGIN failed--compilation aborted",
+	},
+	"language/python": {
+		"ModuleNotFoundError",
+		"No module named",
+		"ImportError",
+	},
+	"language/tcl": {
+		"can't find package",
+		"couldn't load library",
+		"package require",
+	},
+	"language/ruby": {
+		"LoadError",
+		"cannot load such file",
+		"no such file to load",
+		"in `require'",
+	},
+	"language/c-sharp": {
+		"CS0246",
+		"are you missing a using directive",
+		"could not be found",
+		"The type or namespace",
+	},
+	"shared": {
+		"unresolved import", "no external crate", "E0432", "E0463", "can't find crate", // RUST
+		"No such file or directory", "fatal error:", "undefined reference to", // C/CPP
+	},
+}


Many of these are far too vague to directly map to missing dependency errors.

I'd strongly prefer if this tool tried to find the most relevant parts of the log, rather than make any assertions about what the log is saying. The LLM will be MUCH better suited for this task than us keyword matching a string.

I rolled it back to check for ModuleNotFoundError as in its Python implementation.
For now, it's known to only work for Python. I added comments saying that we need to improve it to consider other languages and use a more robust mechanism

…mitID.String())

Naatan

Thanks for sticking with me, nice work!

Naatan · 2025-07-23T16:53:56Z

internal/runners/mcp/projecterrors/projecterrors.go

+	type ArtifactOutput struct {
+		Name                  string `json:"name"`
+		Version               string `json:"version"`
+		Namespace             string `json:"namespace"`
+		IsBuildtimeDependency bool   `json:"isBuildtimeDependency"`
+		IsRuntimeDependency   bool   `json:"isRuntimeDependency"`
+		LogURL                string `json:"logURL"`
+		SourceURI             string `json:"sourceURI"`
+		WasFixed              bool   `json:"wasFixed"`
+		IsDependencyError     bool   `json:"isDependencyError"`
+	}


Nit: Define types at the top level (outside of the function) so they can be referenced outside of the function, and to keep the function low on boilerplate.

Naatan · 2025-07-23T16:55:57Z

internal/runners/mcp/projecterrors/projecterrors.go

+	for _, artifact := range failedArtifacts {
+		jsonBytes, err := json.Marshal(ArtifactOutput{
+			Name:                  artifact.Name(),
+			Version:               artifact.Version(),
+			Namespace:             artifact.Ingredients[0].Namespace,
+			IsBuildtimeDependency: artifact.IsBuildtimeDependency,
+			IsRuntimeDependency:   artifact.IsRuntimeDependency,
+			LogURL:                artifact.LogURL,
+			SourceURI:             artifact.Ingredients[0].IngredientSource.Url.String(),
+			WasFixed:              wasFixed[artifact.ArtifactID],
+			IsDependencyError:     isDependencyError[artifact.ArtifactID],
+		})
+		if err != nil {
+			return fmt.Errorf("error marshaling results: %w", err)
+		}
+		runner.output.Print(string(jsonBytes))


Not asking for changes, just noting; Our output package handles structured output like this. But it's based on us running in either JSON or "plain" mode. ie. it affects ALL output. Not sure what this means for MCP, cause with MCP it's kind of a case by case.

Just noting it for now for awareness. Probably an area of improvement for later.

feat: CP-955 migrate ProjectErrorsTool (shallow query)

40d8a52

samueld-activator requested a review from Naatan July 17, 2025 16:47

samueld-activator self-assigned this Jul 17, 2025

Naatan requested changes Jul 17, 2025

View reviewed changes

samueld-activator added 2 commits July 17, 2025 17:56

feat: extend ProjectErrorsTool to check error's cause and check if it…

f22c9b7

… was fixed

chore: reuse existing code instead of calling graphql directly

ce0cd30

samueld-activator requested a review from Naatan July 18, 2025 18:46

Naatan requested changes Jul 18, 2025

View reviewed changes

samueld-activator added 2 commits July 21, 2025 16:31

feat: Add SourceURI to each artifact and refactor some code

6667e8c

chore: dereference *branch.CommitID instead of strfmt.UUID(branch.Com…

a030608

…mitID.String())

samueld-activator requested a review from Naatan July 21, 2025 19:36

fix: artifact.Name -> artifact.Name() is failing CI

3a07e24

Naatan approved these changes Jul 23, 2025

View reviewed changes

samueld-activator merged commit 1b6ba39 into master Jul 23, 2025
6 of 7 checks passed

samueld-activator deleted the samueld/CP-955 branch July 23, 2025 17:13

	return mcp.NewToolResultError(fmt.Errorf("invalid namespace format. Use 'owner/project' format: %w", err).Error()), nil
	return mcp.NewToolResultError(fmt.Sprintf("Could not parse namespace: %s", errs.JoinMessage(err))), nil

feat: CP-955 migrate ProjectErrorsTool #3688

feat: CP-955 migrate ProjectErrorsTool #3688

Uh oh!

Conversation

samueld-activator commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Naatan left a comment

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!

Uh oh!

Uh oh!