Skip to content

chore: replace mcp-go with nanobot #980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 13, 2025

Conversation

thedadams
Copy link
Contributor

No description provided.

@@ -49,7 +49,7 @@ serve-docs:

# This will initialize the node_modules needed to run the docs dev server. Run this before running serve-docs
init-docs:
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster yarn install
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster npm install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this OK? It looked like we are using npm everywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine I think

@thedadams thedadams requested a review from drpebcak June 12, 2025 01:49
@thedadams
Copy link
Contributor Author

Looks like there's an issue here. Moving to draft while I figure it out.

g-linville
g-linville previously approved these changes Jun 12, 2025
Copy link
Member

@g-linville g-linville left a comment

Choose a reason for hiding this comment

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

Nice, this is a lot cleaner

@@ -49,7 +49,7 @@ serve-docs:

# This will initialize the node_modules needed to run the docs dev server. Run this before running serve-docs
init-docs:
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster yarn install
docker run --rm --workdir=/docs -v $${PWD}/docs:/docs node:18-buster npm install
Copy link
Member

Choose a reason for hiding this comment

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

Should be fine I think

@thedadams thedadams marked this pull request as draft June 12, 2025 02:18
@thedadams thedadams force-pushed the replace-mcp-go-with-nanobot branch from be9260e to cd80db5 Compare June 12, 2025 15:34
@thedadams thedadams marked this pull request as ready for review June 12, 2025 16:53
@thedadams thedadams requested a review from g-linville June 12, 2025 16:53
g-linville
g-linville previously approved these changes Jun 12, 2025
if err := daemon.SysDaemon(); err != nil {
log.Debugf("failed running daemon: %v", err)
if os.Args[1] == "_exec" {
if err := supervise.Daemon(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we print the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

StrongMonkey
StrongMonkey previously approved these changes Jun 12, 2025
njhale
njhale previously approved these changes Jun 12, 2025
Comment on lines +17 to 32
if os.Args[1] == "sys.daemon" {
if os.Getenv("GPTSCRIPT_DEBUG") == "true" {
mvl.SetDebug()
}
if err := daemon.SysDaemon(); err != nil {
log.Debugf("failed running daemon: %v", err)
}
os.Exit(0)
}
if err := daemon.SysDaemon(); err != nil {
log.Debugf("failed running daemon: %v", err)
if os.Args[1] == "_exec" {
if err := supervise.Daemon(); err != nil {
_, _ = fmt.Fprintf(os.Stderr, "failed running _exec: %v\n", err)
os.Exit(1)
}
os.Exit(0)
}
Copy link
Member

Choose a reason for hiding this comment

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

nano-nit: could use a switch statement here.

@thedadams thedadams dismissed stale reviews from njhale and StrongMonkey via bcc5b87 June 12, 2025 21:13
@thedadams thedadams force-pushed the replace-mcp-go-with-nanobot branch from bcc5b87 to afb3675 Compare June 13, 2025 00:20
@thedadams
Copy link
Contributor Author

The final pushes were removing the replace directive and rebasing. So, merging.

@thedadams thedadams merged commit 118e3ae into gptscript-ai:main Jun 13, 2025
10 checks passed
@thedadams thedadams deleted the replace-mcp-go-with-nanobot branch June 13, 2025 00:30
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.

4 participants