From 3293607bef3bfa30aaccf84c484d758e91fc019f Mon Sep 17 00:00:00 2001 From: yushen Date: Tue, 24 Mar 2026 15:49:32 +0800 Subject: [PATCH] fix(cli): address code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Add Client.SendHeartbeat/Register methods — no more direct postJSON calls 2. Use url.Values for query params to prevent URL injection 3. Unexport helpers (envOrDefault, durationFromEnv, sleepWithContext) 4. CLI resolveWorkspaceID falls back to daemon.json 5. Implement agent stop (PUT /api/agents/{id} with status=offline) 6. Add --output flag to agent get for consistent UX 7. Add server/multica to .gitignore for stray builds 8. Inject version/commit via -ldflags in Makefile build target Co-Authored-By: Claude Opus 4.6 (1M context) --- .gitignore | 1 + Makefile | 5 +++- server/cmd/multica/cmd_agent.go | 41 ++++++++++++++++++++++++++++--- server/internal/cli/client.go | 33 +++++++++++++++++++++++++ server/internal/cli/config.go | 20 +++++++++++++++ server/internal/daemon/client.go | 16 ++++++++++++ server/internal/daemon/config.go | 18 +++++++------- server/internal/daemon/daemon.go | 19 ++++++-------- server/internal/daemon/helpers.go | 11 +++------ 9 files changed, 130 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index d902de1c..15638010 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,7 @@ server/tmp/ server/migrate server/daemon server/multica-cli +server/multica # Test artifacts test-results/ diff --git a/Makefile b/Makefile index d5c0b55b..929ba7c7 100644 --- a/Makefile +++ b/Makefile @@ -118,9 +118,12 @@ daemon: cli: cd server && go run ./cmd/multica $(ARGS) +VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) +COMMIT ?= $(shell git rev-parse --short HEAD 2>/dev/null || echo unknown) + build: cd server && go build -o bin/server ./cmd/server - cd server && go build -o bin/multica-cli ./cmd/multica + cd server && go build -ldflags "-X main.version=$(VERSION) -X main.commit=$(COMMIT)" -o bin/multica-cli ./cmd/multica test: cd server && go test ./... diff --git a/server/cmd/multica/cmd_agent.go b/server/cmd/multica/cmd_agent.go index 69fcaf60..670c8c11 100644 --- a/server/cmd/multica/cmd_agent.go +++ b/server/cmd/multica/cmd_agent.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "net/url" "os" "time" @@ -50,6 +51,7 @@ func init() { agentCmd.AddCommand(agentStopCmd) agentListCmd.Flags().String("output", "table", "Output format: table or json") + agentGetCmd.Flags().String("output", "json", "Output format: table or json") } func newAPIClient(cmd *cobra.Command) (*cli.APIClient, error) { @@ -84,7 +86,11 @@ func resolveWorkspaceID(cmd *cobra.Command) string { return val } cfg, _ := cli.LoadCLIConfig() - return cfg.WorkspaceID + if cfg.WorkspaceID != "" { + return cfg.WorkspaceID + } + // Fallback: try daemon.json for workspace_id + return cli.LoadWorkspaceIDFromDaemonConfig() } func runAgentList(cmd *cobra.Command, _ []string) error { @@ -99,7 +105,7 @@ func runAgentList(cmd *cobra.Command, _ []string) error { var agents []map[string]any path := "/api/agents" if client.WorkspaceID != "" { - path += "?workspace_id=" + client.WorkspaceID + path += "?" + url.Values{"workspace_id": {client.WorkspaceID}}.Encode() } if err := client.GetJSON(ctx, path, &agents); err != nil { return fmt.Errorf("list agents: %w", err) @@ -138,6 +144,20 @@ func runAgentGet(cmd *cobra.Command, args []string) error { return fmt.Errorf("get agent: %w", err) } + output, _ := cmd.Flags().GetString("output") + if output == "table" { + headers := []string{"ID", "NAME", "STATUS", "RUNTIME", "DESCRIPTION"} + rows := [][]string{{ + strVal(agent, "id"), + strVal(agent, "name"), + strVal(agent, "status"), + strVal(agent, "runtime_mode"), + strVal(agent, "description"), + }} + cli.PrintTable(os.Stdout, headers, rows) + return nil + } + return cli.PrintJSON(os.Stdout, agent) } @@ -159,8 +179,21 @@ func runAgentDelete(cmd *cobra.Command, args []string) error { } func runAgentStop(cmd *cobra.Command, args []string) error { - // TODO: implement agent stop (PUT /api/agents/{id} with status=offline) - return fmt.Errorf("agent stop is not yet implemented") + client, err := newAPIClient(cmd) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + body := map[string]any{"status": "offline"} + if err := client.PutJSON(ctx, "/api/agents/"+args[0], body, nil); err != nil { + return fmt.Errorf("stop agent: %w", err) + } + + fmt.Fprintf(os.Stderr, "Agent %s stopped.\n", args[0]) + return nil } func strVal(m map[string]any, key string) string { diff --git a/server/internal/cli/client.go b/server/internal/cli/client.go index 90badbb6..550f16cb 100644 --- a/server/internal/cli/client.go +++ b/server/internal/cli/client.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "context" "encoding/json" "fmt" @@ -76,6 +77,38 @@ func (c *APIClient) DeleteJSON(ctx context.Context, path string) error { return nil } +// PutJSON performs a PUT request with a JSON body. +func (c *APIClient) PutJSON(ctx context.Context, path string, body any, out any) error { + data, err := json.Marshal(body) + if err != nil { + return err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPut, c.BaseURL+path, bytes.NewReader(data)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json") + if c.WorkspaceID != "" { + req.Header.Set("X-Workspace-ID", c.WorkspaceID) + } + + resp, err := c.HTTPClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode >= 400 { + respData, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) + return fmt.Errorf("PUT %s returned %d: %s", path, resp.StatusCode, strings.TrimSpace(string(respData))) + } + if out == nil { + return nil + } + return json.NewDecoder(resp.Body).Decode(out) +} + // HealthCheck hits the /health endpoint and returns the response body. func (c *APIClient) HealthCheck(ctx context.Context) (string, error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+"/health", nil) diff --git a/server/internal/cli/config.go b/server/internal/cli/config.go index a2aa1bb8..bb4921dc 100644 --- a/server/internal/cli/config.go +++ b/server/internal/cli/config.go @@ -45,6 +45,26 @@ func LoadCLIConfig() (CLIConfig, error) { return cfg, nil } +// LoadWorkspaceIDFromDaemonConfig reads workspace_id from ~/.multica/daemon.json +// as a fallback when it's not set in the CLI config. +func LoadWorkspaceIDFromDaemonConfig() string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + data, err := os.ReadFile(filepath.Join(home, ".multica/daemon.json")) + if err != nil { + return "" + } + var cfg struct { + WorkspaceID string `json:"workspace_id"` + } + if json.Unmarshal(data, &cfg) != nil { + return "" + } + return cfg.WorkspaceID +} + // SaveCLIConfig writes the CLI config to disk. func SaveCLIConfig(cfg CLIConfig) error { path, err := CLIConfigPath() diff --git a/server/internal/daemon/client.go b/server/internal/daemon/client.go index 8f2c58d9..98a654fb 100644 --- a/server/internal/daemon/client.go +++ b/server/internal/daemon/client.go @@ -84,6 +84,22 @@ func (c *Client) FailTask(ctx context.Context, taskID, errMsg string) error { }, nil) } +func (c *Client) SendHeartbeat(ctx context.Context, runtimeID string) error { + return c.postJSON(ctx, "/api/daemon/heartbeat", map[string]string{ + "runtime_id": runtimeID, + }, nil) +} + +func (c *Client) Register(ctx context.Context, req map[string]any) ([]Runtime, error) { + var resp struct { + Runtimes []Runtime `json:"runtimes"` + } + if err := c.postJSON(ctx, "/api/daemon/register", req, &resp); err != nil { + return nil, err + } + return resp.Runtimes, nil +} + func (c *Client) postJSON(ctx context.Context, path string, reqBody any, respBody any) error { var body io.Reader if reqBody != nil { diff --git a/server/internal/daemon/config.go b/server/internal/daemon/config.go index 6c474771..3bf588ce 100644 --- a/server/internal/daemon/config.go +++ b/server/internal/daemon/config.go @@ -55,7 +55,7 @@ type Overrides struct { // persisted config, and optional CLI flag overrides. func LoadConfig(overrides Overrides) (Config, error) { // Server URL: override > env > default - rawServerURL := EnvOrDefault("MULTICA_SERVER_URL", DefaultServerURL) + rawServerURL := envOrDefault("MULTICA_SERVER_URL", DefaultServerURL) if overrides.ServerURL != "" { rawServerURL = overrides.ServerURL } @@ -91,14 +91,14 @@ func LoadConfig(overrides Overrides) (Config, error) { // Probe available agent CLIs agents := map[string]AgentEntry{} - claudePath := EnvOrDefault("MULTICA_CLAUDE_PATH", "claude") + claudePath := envOrDefault("MULTICA_CLAUDE_PATH", "claude") if _, err := exec.LookPath(claudePath); err == nil { agents["claude"] = AgentEntry{ Path: claudePath, Model: strings.TrimSpace(os.Getenv("MULTICA_CLAUDE_MODEL")), } } - codexPath := EnvOrDefault("MULTICA_CODEX_PATH", "codex") + codexPath := envOrDefault("MULTICA_CODEX_PATH", "codex") if _, err := exec.LookPath(codexPath); err == nil { agents["codex"] = AgentEntry{ Path: codexPath, @@ -132,7 +132,7 @@ func LoadConfig(overrides Overrides) (Config, error) { } // Durations: override > env > default - pollInterval, err := DurationFromEnv("MULTICA_DAEMON_POLL_INTERVAL", DefaultPollInterval) + pollInterval, err := durationFromEnv("MULTICA_DAEMON_POLL_INTERVAL", DefaultPollInterval) if err != nil { return Config{}, err } @@ -140,7 +140,7 @@ func LoadConfig(overrides Overrides) (Config, error) { pollInterval = overrides.PollInterval } - heartbeatInterval, err := DurationFromEnv("MULTICA_DAEMON_HEARTBEAT_INTERVAL", DefaultHeartbeatInterval) + heartbeatInterval, err := durationFromEnv("MULTICA_DAEMON_HEARTBEAT_INTERVAL", DefaultHeartbeatInterval) if err != nil { return Config{}, err } @@ -148,7 +148,7 @@ func LoadConfig(overrides Overrides) (Config, error) { heartbeatInterval = overrides.HeartbeatInterval } - agentTimeout, err := DurationFromEnv("MULTICA_AGENT_TIMEOUT", DefaultAgentTimeout) + agentTimeout, err := durationFromEnv("MULTICA_AGENT_TIMEOUT", DefaultAgentTimeout) if err != nil { return Config{}, err } @@ -157,17 +157,17 @@ func LoadConfig(overrides Overrides) (Config, error) { } // String overrides - daemonID := EnvOrDefault("MULTICA_DAEMON_ID", host) + daemonID := envOrDefault("MULTICA_DAEMON_ID", host) if overrides.DaemonID != "" { daemonID = overrides.DaemonID } - deviceName := EnvOrDefault("MULTICA_DAEMON_DEVICE_NAME", host) + deviceName := envOrDefault("MULTICA_DAEMON_DEVICE_NAME", host) if overrides.DeviceName != "" { deviceName = overrides.DeviceName } - runtimeName := EnvOrDefault("MULTICA_AGENT_RUNTIME_NAME", DefaultRuntimeName) + runtimeName := envOrDefault("MULTICA_AGENT_RUNTIME_NAME", DefaultRuntimeName) if overrides.RuntimeName != "" { runtimeName = overrides.RuntimeName } diff --git a/server/internal/daemon/daemon.go b/server/internal/daemon/daemon.go index 401241fc..8a4f9cdb 100644 --- a/server/internal/daemon/daemon.go +++ b/server/internal/daemon/daemon.go @@ -84,16 +84,14 @@ func (d *Daemon) registerRuntimes(ctx context.Context) ([]Runtime, error) { "runtimes": runtimes, } - var resp struct { - Runtimes []Runtime `json:"runtimes"` - } - if err := d.client.postJSON(ctx, "/api/daemon/register", req, &resp); err != nil { + rts, err := d.client.Register(ctx, req) + if err != nil { return nil, fmt.Errorf("register runtimes: %w", err) } - if len(resp.Runtimes) == 0 { + if len(rts) == 0 { return nil, fmt.Errorf("register runtimes: empty response") } - return resp.Runtimes, nil + return rts, nil } func (d *Daemon) ensurePaired(ctx context.Context) (string, error) { @@ -160,7 +158,7 @@ func (d *Daemon) ensurePaired(ctx context.Context) (string, error) { return "", fmt.Errorf("pairing session expired before approval") } - if err := SleepWithContext(ctx, d.cfg.PollInterval); err != nil { + if err := sleepWithContext(ctx, d.cfg.PollInterval); err != nil { return "", err } } @@ -176,10 +174,7 @@ func (d *Daemon) heartbeatLoop(ctx context.Context, runtimeIDs []string) { return case <-ticker.C: for _, rid := range runtimeIDs { - err := d.client.postJSON(ctx, "/api/daemon/heartbeat", map[string]string{ - "runtime_id": rid, - }, nil) - if err != nil { + if err := d.client.SendHeartbeat(ctx, rid); err != nil { d.logger.Printf("heartbeat failed for runtime %s: %v", rid, err) } } @@ -216,7 +211,7 @@ func (d *Daemon) pollLoop(ctx context.Context, runtimeIDs []string) error { if !claimed { pollOffset = (pollOffset + 1) % n - if err := SleepWithContext(ctx, d.cfg.PollInterval); err != nil { + if err := sleepWithContext(ctx, d.cfg.PollInterval); err != nil { return err } } diff --git a/server/internal/daemon/helpers.go b/server/internal/daemon/helpers.go index 75bb2c24..a7de9b9e 100644 --- a/server/internal/daemon/helpers.go +++ b/server/internal/daemon/helpers.go @@ -8,9 +8,7 @@ import ( "time" ) -// EnvOrDefault returns the trimmed value of the environment variable key, -// falling back to fallback if empty. -func EnvOrDefault(key, fallback string) string { +func envOrDefault(key, fallback string) string { value := strings.TrimSpace(os.Getenv(key)) if value == "" { return fallback @@ -18,9 +16,7 @@ func EnvOrDefault(key, fallback string) string { return value } -// DurationFromEnv parses a duration from an environment variable, -// returning fallback if the variable is empty. -func DurationFromEnv(key string, fallback time.Duration) (time.Duration, error) { +func durationFromEnv(key string, fallback time.Duration) (time.Duration, error) { value := strings.TrimSpace(os.Getenv(key)) if value == "" { return fallback, nil @@ -32,8 +28,7 @@ func DurationFromEnv(key string, fallback time.Duration) (time.Duration, error) return d, nil } -// SleepWithContext blocks for the given duration or until the context is cancelled. -func SleepWithContext(ctx context.Context, d time.Duration) error { +func sleepWithContext(ctx context.Context, d time.Duration) error { timer := time.NewTimer(d) defer timer.Stop()