From b72ac616983f37fed21f242aa00d892532adf92d Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 9 Feb 2026 11:15:00 +0000 Subject: [PATCH] fix(agentci): use log.E() error pattern, add Charm SSH TODOs Replace fmt.Errorf() with structured log.E() errors in agentci, forge, jobrunner packages. Update PipelineSignal comment to reflect dispatch fields. Add TODO markers for charmbracelet/ssh migration across all exec ssh call sites. Co-Authored-By: Virgil --- internal/cmd/ai/cmd_agent.go | 4 ++++ pkg/agentci/config.go | 7 ++++--- pkg/forge/prs.go | 8 ++++---- pkg/jobrunner/forgejo/source.go | 6 +++--- pkg/jobrunner/handlers/dispatch.go | 5 ++++- pkg/jobrunner/types.go | 2 +- scripts/agent-runner.sh | 1 + 7 files changed, 21 insertions(+), 12 deletions(-) diff --git a/internal/cmd/ai/cmd_agent.go b/internal/cmd/ai/cmd_agent.go index 8d325ae7..2c99a4a1 100644 --- a/internal/cmd/ai/cmd_agent.go +++ b/internal/cmd/ai/cmd_agent.go @@ -52,6 +52,7 @@ func agentAddCmd() *cli.Command { } // Test SSH connectivity. + // TODO: Replace exec ssh with charmbracelet/ssh native Go client + keygen. fmt.Printf("Testing SSH to %s... ", host) out, err := exec.Command("ssh", "-o", "StrictHostKeyChecking=accept-new", @@ -115,6 +116,7 @@ func agentListCmd() *cli.Command { } // Quick SSH check for queue depth. + // TODO: Replace exec ssh with charmbracelet/ssh native Go client. queue := dimStyle.Render("-") out, err := exec.Command("ssh", "-o", "StrictHostKeyChecking=accept-new", @@ -180,6 +182,7 @@ func agentStatusCmd() *cli.Command { fi ` + // TODO: Replace exec ssh with charmbracelet/ssh native Go client. sshCmd := exec.Command("ssh", "-o", "StrictHostKeyChecking=accept-new", "-o", "ConnectTimeout=10", @@ -215,6 +218,7 @@ func agentLogsCmd() *cli.Command { return fmt.Errorf("agent %q not found", name) } + // TODO: Replace exec ssh with charmbracelet/ssh native Go client. tailArgs := []string{ "-o", "StrictHostKeyChecking=accept-new", "-o", "ConnectTimeout=10", diff --git a/pkg/agentci/config.go b/pkg/agentci/config.go index 27c654fe..f0d39c29 100644 --- a/pkg/agentci/config.go +++ b/pkg/agentci/config.go @@ -6,6 +6,7 @@ import ( "github.com/host-uk/core/pkg/config" "github.com/host-uk/core/pkg/jobrunner/handlers" + "github.com/host-uk/core/pkg/log" ) // AgentConfig represents a single agent machine in the config file. @@ -33,7 +34,7 @@ func LoadAgents(cfg *config.Config) (map[string]handlers.AgentTarget, error) { continue } if ac.Host == "" { - return nil, fmt.Errorf("agent %q: host is required", name) + return nil, log.E("agentci.LoadAgents", fmt.Sprintf("agent %q: host is required", name), nil) } queueDir := ac.QueueDir if queueDir == "" { @@ -80,10 +81,10 @@ func SaveAgent(cfg *config.Config, name string, ac AgentConfig) error { func RemoveAgent(cfg *config.Config, name string) error { var agents map[string]AgentConfig if err := cfg.Get("agentci.agents", &agents); err != nil { - return fmt.Errorf("no agents configured") + return log.E("agentci.RemoveAgent", "no agents configured", err) } if _, ok := agents[name]; !ok { - return fmt.Errorf("agent %q not found", name) + return log.E("agentci.RemoveAgent", fmt.Sprintf("agent %q not found", name), nil) } delete(agents, name) return cfg.Set("agentci.agents", agents) diff --git a/pkg/forge/prs.go b/pkg/forge/prs.go index dfa7552d..5c010b18 100644 --- a/pkg/forge/prs.go +++ b/pkg/forge/prs.go @@ -29,7 +29,7 @@ func (c *Client) MergePullRequest(owner, repo string, index int64, method string return log.E("forge.MergePullRequest", "failed to merge pull request", err) } if !merged { - return fmt.Errorf("forge.MergePullRequest: merge returned false for %s/%s#%d", owner, repo, index) + return log.E("forge.MergePullRequest", fmt.Sprintf("merge returned false for %s/%s#%d", owner, repo, index), nil) } return nil } @@ -41,13 +41,13 @@ func (c *Client) SetPRDraft(owner, repo string, index int64, draft bool) error { payload := map[string]bool{"draft": draft} body, err := json.Marshal(payload) if err != nil { - return fmt.Errorf("forge.SetPRDraft: marshal: %w", err) + return log.E("forge.SetPRDraft", "marshal payload", err) } url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.url, owner, repo, index) req, err := http.NewRequest(http.MethodPatch, url, bytes.NewReader(body)) if err != nil { - return fmt.Errorf("forge.SetPRDraft: create request: %w", err) + return log.E("forge.SetPRDraft", "create request", err) } req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", "token "+c.token) @@ -59,7 +59,7 @@ func (c *Client) SetPRDraft(owner, repo string, index int64, draft bool) error { defer func() { _ = resp.Body.Close() }() if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("forge.SetPRDraft: unexpected status %d", resp.StatusCode) + return log.E("forge.SetPRDraft", fmt.Sprintf("unexpected status %d", resp.StatusCode), nil) } return nil } diff --git a/pkg/jobrunner/forgejo/source.go b/pkg/jobrunner/forgejo/source.go index 0df0f13a..38b41b48 100644 --- a/pkg/jobrunner/forgejo/source.go +++ b/pkg/jobrunner/forgejo/source.go @@ -82,7 +82,7 @@ func (s *ForgejoSource) pollRepo(_ context.Context, owner, repo string) ([]*jobr // Fetch epic issues (label=epic, state=open). issues, err := s.forge.ListIssues(owner, repo, forge.ListIssuesOpts{State: "open"}) if err != nil { - return nil, fmt.Errorf("fetch issues: %w", err) + return nil, log.E("forgejo.pollRepo", "fetch issues", err) } // Filter to epics only. @@ -106,7 +106,7 @@ func (s *ForgejoSource) pollRepo(_ context.Context, owner, repo string) ([]*jobr // Fetch all open PRs (and also merged/closed to catch MERGED state). prs, err := s.forge.ListPullRequests(owner, repo, "all") if err != nil { - return nil, fmt.Errorf("fetch PRs: %w", err) + return nil, log.E("forgejo.pollRepo", "fetch PRs", err) } var signals []*jobrunner.PipelineSignal @@ -167,7 +167,7 @@ type epicInfo struct { func splitRepo(full string) (string, string, error) { parts := strings.SplitN(full, "/", 2) if len(parts) != 2 || parts[0] == "" || parts[1] == "" { - return "", "", fmt.Errorf("expected owner/repo format, got %q", full) + return "", "", log.E("forgejo.splitRepo", fmt.Sprintf("expected owner/repo format, got %q", full), nil) } return parts[0], parts[1], nil } diff --git a/pkg/jobrunner/handlers/dispatch.go b/pkg/jobrunner/handlers/dispatch.go index 116e01cd..886f96c2 100644 --- a/pkg/jobrunner/handlers/dispatch.go +++ b/pkg/jobrunner/handlers/dispatch.go @@ -79,7 +79,7 @@ func (h *DispatchHandler) Execute(ctx context.Context, signal *jobrunner.Pipelin agent, ok := h.agents[signal.Assignee] if !ok { - return nil, fmt.Errorf("unknown agent: %s", signal.Assignee) + return nil, log.E("dispatch.Execute", fmt.Sprintf("unknown agent: %s", signal.Assignee), nil) } // Determine target branch (default to repo default). @@ -166,8 +166,10 @@ func (h *DispatchHandler) Execute(ctx context.Context, signal *jobrunner.Pipelin } // scpTicket writes ticket data to a remote path via SSH. +// TODO: Replace exec ssh+cat with charmbracelet/ssh for native Go SSH. func (h *DispatchHandler) scpTicket(ctx context.Context, host, remotePath string, data []byte) error { // Use ssh + cat instead of scp for piping stdin. + // TODO: Use charmbracelet/keygen for key management, native Go SSH client for transport. cmd := exec.CommandContext(ctx, "ssh", "-o", "StrictHostKeyChecking=accept-new", "-o", "ConnectTimeout=10", @@ -184,6 +186,7 @@ func (h *DispatchHandler) scpTicket(ctx context.Context, host, remotePath string } // ticketExists checks if a ticket file already exists in queue, active, or done. +// TODO: Replace exec ssh with native Go SSH client (charmbracelet/ssh). func (h *DispatchHandler) ticketExists(agent AgentTarget, ticketName string) bool { cmd := exec.Command("ssh", "-o", "StrictHostKeyChecking=accept-new", diff --git a/pkg/jobrunner/types.go b/pkg/jobrunner/types.go index 79cf6b5b..e8d0bd2d 100644 --- a/pkg/jobrunner/types.go +++ b/pkg/jobrunner/types.go @@ -6,7 +6,7 @@ import ( ) // PipelineSignal is the structural snapshot of a child issue/PR. -// Never contains comment bodies or free text — structural signals only. +// Carries structural state plus issue title/body for dispatch prompts. type PipelineSignal struct { EpicNumber int ChildNumber int diff --git a/scripts/agent-runner.sh b/scripts/agent-runner.sh index 85f41086..06c99bc7 100755 --- a/scripts/agent-runner.sh +++ b/scripts/agent-runner.sh @@ -75,6 +75,7 @@ FORGEJO_USER=$(jq -r '.forgejo_user // empty' "$TICKET_FILE") if [ -z "$FORGEJO_USER" ]; then FORGEJO_USER="$(hostname -s)-$(whoami)" fi +# TODO: Replace token-in-URL with git credential helper or SSH clone via charmbracelet/keygen. CLONE_URL="https://${FORGEJO_USER}:${FORGE_TOKEN}@${FORGE_URL#https://}/${REPO_OWNER}/${REPO_NAME}.git" if [ -d "$REPO_DIR/.git" ]; then