Added state-mcp #3671

Naatan · 2025-04-10T16:05:05Z

https://activestatef.atlassian.net/browse/CP-945

Version 0.47.0-RC2

Version 0.47.0-RC3

Version 0.47.1-RC1

…shell doesnt support it

# Conflicts: # go.mod

Naatan · 2025-07-11T23:00:09Z

@samueld-activator flagged you for review mainly for the high level architecture, much of the actual business logic I suggest you defer to Mitchell for now.

Of main interest to you will be:

https://github.com/ActiveState/cli/pull/3671/files#diff-e49aff4bd82f3519a62eed2b59080de67be9fabe342a5d1962512dba2437a388

@mitchell-as This is introducing the state-mcp executable with basic handling for tool calling. I'm not entirely happy with how primer and the output package are being treated, but addressing that properly would be a significantly bigger refactor that I don't want to get into now.

mitchell-as

There are failing unit tests that need to be addressed: https://github.com/ActiveState/cli/actions/runs/16231227928/job/45834291877?pr=3671#step:14:1021.

I've reviewed everything except the 'state-mcp' folder. I will do so after either Samuel weighs in on the architecture, or my other comments are addressed.

mitchell-as · 2025-07-14T14:43:42Z

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

+func Categories() ToolCategories {
+	return ToolCategories{
+		ToolCategoryDebug,
+	}


nit: Usually we like to return pointers to avoid copies each time this function is called.

I've gotten into the habit of only using pointers when I intend to track the underlying type across different areas of responsibility. I don't believe we ever had any rule around this.

I'd have preferred to make this a constant, but Go is a little weird about what things can be constants.

mitchell-as · 2025-07-14T14:54:15Z

internal/runners/cve/cve.go

+	defer func() {
+		if rc := recover(); rc != nil {
+			logging.Error("Recovered from panic: %v", rc)
+			fmt.Printf("Recovered from panic: %v\n", rc)
+		}
+	}()


None of the other runners have this. This seems like a quick, cheap fix that hides a more serious problem. Ideally we would utilize defensive programming to avoid this scenario.

Perhaps this is leftover from a debug session to identify the problem? In that case, please remove it.

Thanks for catching. I don't recall why I did this, probably was trying to debug.

mitchell-as · 2025-07-14T14:59:26Z

pkg/projectfile/projectfile.go

 	projectRegex := regexp.MustCompile(fmt.Sprintf("(?m:(^project:\\s*%s))", ProjectURLRe))
 	lockString := fmt.Sprintf("%s@%s", branch, version)
-	lockUpdate := []byte(fmt.Sprintf("${1}\nlock: %s", lockString))
+	lockUpdate := []byte(fmt.Sprintf(`${1}\nlock: %s`, lockString))


I don't see why this was changed. Now there will be no newline in the string, but instead a literal '' followed by an 'n'.

Sorry I don't recall. Some of these changes go back months. Probably had a previous change that I reverted.

mitchell-as · 2025-07-14T15:04:21Z

internal/runners/hello/hello_example.go

 // expressed in its own file.
 package hello

 import (


I don't understand why there are changes in this file. It's supposed to be a pretty complete example for a runner. These changes are stripping it down and minimizing its usefulness.

Please don't tell me you had an AI produce a good chunk of this PR and I'm the first one reviewing it.

Because it required me to satisfy project dependencies that are inappropriate in an MCP context. I could have went the extra mile and mocked them anyway, but in reviewing this I didn't feel like it added any real value for the purposes of showing an example runner. I don't mind bringing it back and mocking it if you disagree.

No I did not have AI generate any significant parts. I use it mainly as a glorified auto completer. I will say I've found it useful to generate larger bits of logic when working with technologies I'm unfamiliar with, but that's obviously not the case here.

mitchell-as · 2025-07-14T15:08:01Z

internal/output/plain.go

+	w := NewWriteProxy(f.cfg.OutWriter, func(p []byte) {
+		f.history.Print = append(f.history.Print, string(p))
+	})


I wish there was a way to not have to construct this every single time this function is called. I can't think of one though...

Yeah I went through a couple of different ideas and this one felt the least awkward, but definitely still awkward.

This goes back to my earlier comment; I'd like to address this better but I don't think I can reasonably do so without blowing up the scope of the mcp story significantly, so for now it'll have to be an annoyance that we get to later (which is starting to become a bit of a list wrt the output package..).

mitchell-as · 2025-07-14T15:15:16Z

internal/osutils/user/user_test.go

 	_, err = HomeDir()
-	assert.Error(t, err)
-	assert.Contains(t, err.Error(), "HOME environment variable is unset")
+	assert.NoError(t, err)


I feel like this change negates the value of the test since you've changed init() to check for environment variables.

Please consider either removing the test, or changing it to be more useful.

This is testing that we get our home dir with no env vars set, I don't think it makes sense to remove it.

Note the error tells you to set your HOME dir as it's the one way a user could address this issue. That doesn't mean that the HOME dir not being set is the cause of the issue.

mitchell-as · 2025-07-14T15:17:05Z

internal/osutils/user/user.go

+	u, err := user.Current()
+	if err == nil {
+		return u.HomeDir, nil
+	}


What is the rationale for this change? This could potentially be very significant, as it affects every installation. I'm not very comfortable seeing this.

This addresses running state tool or state-mcp without any environment variables. In the case of mcp it seems common that servers are ran without any env vars, and so relying on HOME will break them. user.Current() uses low level system bindings to retrieve the user info, rather than env vars. This is what all the refactoring in user and storage is about, and why configdir was removed.

This will be required for state-mcp, and I recall occasionally seeing rollbar errors about HOME not being set, so it will help there too.

I'm not too concerned with these changes, either way we rely on the system to tell us the home dir of the current user, it's just through a different channel. And configdir is a very dumb library, I looked into fixing its issues and found that it's so simplistic I was better off just copying the paths it used.

mitchell-as · 2025-07-14T15:18:05Z

internal/installation/storage/storage.go

 	"github.com/ActiveState/cli/internal/constants"
 	"github.com/ActiveState/cli/internal/osutils/user"
 	"github.com/google/uuid"
-	"github.com/shibukawa/configdir"


May I ask why we are no longer using this dependency and instead creating our own version?

Not sure why this is suddenly breaking. The cast library was updated, but the code paths for the errors I'm seeing were not..

mitchell-as

Looks good to me!

mitchell-as · 2025-07-16T16:06:30Z

cmd/state-mcp/server_test.go

+func TestServerHello(t *testing.T) {
+	t.Skip(`
+Fails due to state-svc not being detected when run as regular test, 
+works when running with debugger. Problem for another day.


Do we have a ticket to track this? Otherwise we might lose track of it.

I'm going to leave it alone for now as this feels like a growing pain. This really isn't even a unit test if it relies on state-svc. It's just that we currently don't have a way to end to end test MCP. That'll be its own thing down the line I'm sure but it's not a burning need at the moment.

mitchell-as · 2025-07-16T16:07:50Z

cmd/state/donotshipme/donotshipme.go

@@ -0,0 +1,17 @@
+package donotshipme


I don't understand why this is here or what it's supposed to do or prevent. Would you mind enlightening me? The panic still doesn't shed much light on it for me.

HAH! Thanks for catching! Seems I already dropped the import of this so it shouldn't bite, but obviously forgot to clean this up.

This was from when it started out as a proof of concept. I was paranoid about this bleeding into our codebase somehow.

This reverts commit 169471b, reversing changes made to 897a3a9.

Naatan added 22 commits January 14, 2025 11:46

Merge pull request #3629 from ActiveState/version/0-47-0-RC2

1e30204

Version 0.47.0-RC2

Merge pull request #3639 from ActiveState/version/0-47-0-RC3

44f50ff

Version 0.47.0-RC3

Merge pull request #3642 from ActiveState/version/0-47-0-RC3

abb5937

Version 0.47.0-RC3

Merge pull request #3645 from ActiveState/version/0-47-1-RC1

cde656c

Version 0.47.1-RC1

Merge branch 'version/0-47-1-RC1' into beta

5a667f6

Merge remote-tracking branch 'origin/master' into mcp

e5d7281

Captain: fix SetArgs, sort of

1271598

Don't include root command (state) in NameRecursive

e8d1d28

Added BaseCommand and AllChildren to Captain

94c8042

Remove dependency on HOME and TEMPDIR env vars

48850be

Support debugging via go-build

7114a62

Fix nil panic

0d405d8

Fix simple output oddly formatted

d614458

Drop useless debug entry

e35097b

Work around vscode syntax parsing issue

3a78c3c

Add state-mcp

ed26dc9

Fix AI deciding to indent something that didn't need it

7f33790

Fix prime being stale

993a88b

Drop unused import

0a062a3

Disable tests

c11c5c4

Add missing comma

c8dc1b3

Update test now that we work without HOME set

af5769a

Naatan added the Test: help label Apr 10, 2025

Naatan added 7 commits April 10, 2025 14:11

Include state-mcp in update

93a5529

Windows specific build target

5e29acd

Always print what we're building

c714877

Debug copy error

5cc036d

Re-org by AI

6286ad5

Fix window target not being used

1d20586

Implemented flawed script runner that doesn't pass back stdout as sub…

0063d6f

…shell doesnt support it

Cleanup state-mcp

121eb84

Naatan changed the title ~~Added highly experimental state-mcp~~ Added state-mcp Jul 11, 2025

Merge branch 'master' into mcp

cac3faf

# Conflicts: # go.mod

Naatan requested review from mitchell-as and samueld-activator July 11, 2025 22:57

mitchell-as requested changes Jul 14, 2025

View reviewed changes

Naatan added 4 commits July 15, 2025 09:47

Handle cast library returning nil values if source is nil

397c278

Not sure why this is suddenly breaking. The cast library was updated, but the code paths for the errors I'm seeing were not..

Drop debugging code

6d41be7

Fix test not constructing outputter properly

38e51d5

Fix tests

c97f5d8

Naatan requested a review from mitchell-as July 15, 2025 22:04

mitchell-as previously approved these changes Jul 16, 2025

View reviewed changes

samueld-activator previously approved these changes Jul 16, 2025

View reviewed changes

Drop donotshipme package; we can in fact ship this now

8ffadb2

Naatan dismissed stale reviews from samueld-activator and mitchell-as via 8ffadb2 July 16, 2025 17:43

samueld-activator approved these changes Jul 16, 2025

View reviewed changes

mitchell-as approved these changes Jul 16, 2025

View reviewed changes

Naatan merged commit 169471b into master Jul 16, 2025
7 of 8 checks passed

Naatan deleted the mcp branch July 16, 2025 17:54

mitchell-as added a commit that referenced this pull request Jul 17, 2025

Revert "Merge pull request #3671 from ActiveState/mcp"

a71ac7b

This reverts commit 169471b, reversing changes made to 897a3a9.

Added state-mcp #3671

Added state-mcp #3671

Uh oh!

Conversation

Naatan commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Naatan commented Jul 11, 2025

Uh oh!

mitchell-as 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!

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!

mitchell-as 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Naatan commented Apr 10, 2025 •

edited

Loading