feat(sam-node): add skill install, --daemonize, and reset --all - #268
Conversation
Getting an AI agent onto the mesh took a page of manual steps: paste the skill in by hand, invent an API token, keep a terminal pinned to a foreground node, and copy MCP config out of the docs. - `sam-node skill [install|list|show]` installs the embedded SAM agent skill into the directories Claude Code and Antigravity scan, so an agent knows when and how to drive the mesh on its own. SKILL.md now also covers bootstrapping a node, and reaching mesh inference through the OpenAI-compatible facade rather than call_remote_tool. - `sam-node run --daemonize` starts the node detached and returns once its local API answers, generating an API token in the data directory when none is configured. A failed startup is reported with the child's log, and the command is idempotent. - `sam-node reset --all` deletes every file the node keeps, so a broken or half-configured setup can be started over instead of silently inheriting old state. It confirms first, and needs --yes where there is no terminal to confirm on. internal/node: NewStore wraps its file-lock timeout in an ErrStoreLocked sentinel, which is how the CLI tells "a node is already running" apart from a genuine store failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces background daemonization for sam-node via the --daemonize flag, adds a reset --all command to completely purge local node state, and implements a skill command to install and manage the SAM agent skill for local AI coding assistants. The review feedback highlights three key areas for improvement: killing the background child process if startup verification fails to prevent process leaks, robustly handling empty or corrupt token files during automatic token generation, and ensuring the E2E tests are portable across Linux and macOS by avoiding GNU-specific stat flags.
| if err := waitForDaemon(probe, exited); err != nil { | ||
| return fmt.Errorf("%w\nLast lines of %s:\n%s", err, logPath, tailFile(logPath, 8192, 10)) | ||
| } |
There was a problem hiding this comment.
If waitForDaemon times out or fails, the child process might still be running in an unresponsive or stuck state. Since the parent process exits with an error, this child process is leaked in the background, which will keep the database locked and prevent subsequent startup attempts.
Explicitly killing the child process on startup failure ensures clean resource cleanup.
| if err := waitForDaemon(probe, exited); err != nil { | |
| return fmt.Errorf("%w\nLast lines of %s:\n%s", err, logPath, tailFile(logPath, 8192, 10)) | |
| } | |
| if err := waitForDaemon(probe, exited); err != nil { | |
| _ = child.Process.Kill() | |
| return fmt.Errorf("%w\nLast lines of %s:\n%s", err, logPath, tailFile(logPath, 8192, 10)) | |
| } |
| if _, err := os.Stat(tokenPath); err == nil { | ||
| return []string{"--api-token-path", tokenPath}, tokenPath, nil | ||
| } | ||
| buf := make([]byte, 32) | ||
| if _, err := rand.Read(buf); err != nil { | ||
| return nil, "", fmt.Errorf("generating an API token: %w", err) | ||
| } | ||
| f, err := os.OpenFile(tokenPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) |
There was a problem hiding this comment.
If tokenPath already exists but is empty (e.g., due to an interrupted write or crash in a previous run), os.Stat will succeed, and the daemon will be started with an empty token file, which might bypass authentication or cause unexpected behavior.
Furthermore, if we only check for non-empty files but keep os.O_EXCL, the subsequent os.OpenFile call will fail because the file already exists.
We should ensure the file is non-empty, and use os.O_TRUNC instead of os.O_EXCL so we can safely overwrite an empty or corrupt token file.
| if _, err := os.Stat(tokenPath); err == nil { | |
| return []string{"--api-token-path", tokenPath}, tokenPath, nil | |
| } | |
| buf := make([]byte, 32) | |
| if _, err := rand.Read(buf); err != nil { | |
| return nil, "", fmt.Errorf("generating an API token: %w", err) | |
| } | |
| f, err := os.OpenFile(tokenPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) | |
| if info, err := os.Stat(tokenPath); err == nil && info.Size() > 0 { | |
| return []string{"--api-token-path", tokenPath}, tokenPath, nil | |
| } | |
| buf := make([]byte, 32) | |
| if _, err := rand.Read(buf); err != nil { | |
| return nil, "", fmt.Errorf("generating an API token: %w", err) | |
| } | |
| f, err := os.OpenFile(tokenPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0600) |
| # The child ran detached with a generated token and its own log. | ||
| [[ -f "$XDG_CONFIG_HOME/sam-mesh/api-token" ]] | ||
| [[ -s "$XDG_CONFIG_HOME/sam-mesh/sam-node.log" ]] | ||
| [[ "$(stat -c %a "$XDG_CONFIG_HOME/sam-mesh/api-token")" == "600" ]] |
There was a problem hiding this comment.
The stat -c %a command is specific to GNU stat (Linux) and will fail on macOS/BSD systems where stat -f %Lp is used instead. This will break E2E tests for developers running on macOS.
Using a portable fallback ensures the tests run seamlessly across both Linux and macOS environments.
[[ "$(stat -c %a "$XDG_CONFIG_HOME/sam-mesh/api-token" 2>/dev/null || stat -f %Lp "$XDG_CONFIG_HOME/sam-mesh/api-token")" == "600" ]]
1 participant
Getting an AI agent onto the mesh took a page of manual steps: paste the skill in by hand, invent an API token, keep a terminal pinned to a foreground node, and copy MCP config out of the docs.
sam-node skill [install|list|show]installs the embedded SAM agent skill into the directories Claude Code and Antigravity scan, so an agent knows when and how to drive the mesh on its own. SKILL.md now also covers bootstrapping a node, and reaching mesh inference through the OpenAI-compatible facade rather than call_remote_tool.sam-node run --daemonizestarts the node detached and returns once its local API answers, generating an API token in the data directory when none is configured. A failed startup is reported with the child's log, and the command is idempotent.sam-node reset --alldeletes every file the node keeps, so a broken or half-configured setup can be started over instead of silently inheriting old state. It confirms first, and needs --yes where there is no terminal to confirm on.internal/node: NewStore wraps its file-lock timeout in an ErrStoreLocked sentinel, which is how the CLI tells "a node is already running" apart from a genuine store failure.