From 3ba8fbb8fba5785fd64b08d5e3673cd502f4ef9f Mon Sep 17 00:00:00 2001 From: Snider Date: Fri, 20 Feb 2026 05:42:02 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20Phase=202=20=E2=80=94=20error=20wrapping?= =?UTF-8?q?,=20context=20audit,=20rate=20limiting=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Standardise all fmt.Errorf calls to "package.Func: context" pattern across jobrunner/journal.go, agentci/config.go, agentci/security.go, handlers/dispatch.go, and forge/labels.go (15 bare errors fixed) - Add SecureSSHCommandContext with context propagation for cancellable SSH operations; update dispatch handler to use it - Add CheckGitHubRateLimitCtx for context-aware rate limit checking - Document Forgejo/Gitea SDK v2 context limitation in FINDINGS.md (SDKs do not accept context.Context — adding ctx to 66 wrappers would be ceremony without real propagation) - Review and document rate limiter: handles all edge cases, adaptive throttling at 75% GitHub usage, SDK header parsing not feasible Co-Authored-By: Virgil --- FINDINGS.md | 52 ++++++++++++++++++++++++++++++++ agentci/config.go | 6 ++-- agentci/security.go | 14 +++++++-- collect/ratelimit.go | 10 +++++- forge/labels.go | 2 +- jobrunner/coverage_boost_test.go | 2 +- jobrunner/handlers/dispatch.go | 8 ++--- jobrunner/journal.go | 30 +++++++++--------- 8 files changed, 96 insertions(+), 28 deletions(-) diff --git a/FINDINGS.md b/FINDINGS.md index 1ad73a0..421cf1d 100644 --- a/FINDINGS.md +++ b/FINDINGS.md @@ -91,3 +91,55 @@ Both forge/ and gitea/ follow the same priority order: 4. Default URL when nothing configured (`http://localhost:4000` for forge, `https://gitea.snider.dev` for gitea) Tests must use `t.Setenv("HOME", t.TempDir())` to isolate from the real config file on the development machine. + +--- + +## 2026-02-20: Phase 2 Hardening (Virgil) + +### Error Wrapping Audit + +All `fmt.Errorf` calls now follow the `"package.Func: context: %w"` pattern (for wrapped errors) or `"package.Func: message"` pattern (for sentinel/validation errors). Files fixed: + +| File | Bare errors | Pattern applied | +|------|------------|-----------------| +| `jobrunner/journal.go` | 8 | `journal.NewJournal:`, `journal.sanitizePathComponent:`, `journal.Append:` | +| `agentci/config.go` | 3 | `agentci.LoadAgents:`, `agentci.RemoveAgent:` | +| `agentci/security.go` | 2 | `agentci.SanitizePath:` | +| `jobrunner/handlers/dispatch.go` | 1 | `handlers.Dispatch.Execute:` | +| `forge/labels.go` | 1 | `forge.GetLabelByName:` | + +Existing `log.E()` and `core.E()` calls already followed the pattern — no changes needed for those. + +### Context Propagation Audit + +**SDK limitation (verified):** The Forgejo SDK v2 (`codeberg.org/mvdkleijn/forgejo-sdk/forgejo/v2`) does NOT accept `context.Context` parameters on any API method. All SDK calls are synchronous and blocking. Adding `ctx` to the 66 forge/ and gitea/ wrapper signatures would be ceremony without real propagation — the context would be accepted but never passed to the SDK. The same applies to the Gitea SDK (`code.gitea.io/sdk/gitea`). + +**Where context IS propagated:** + +1. **git/** — All git operations use `exec.CommandContext(ctx, ...)` via `gitCommand()` and `gitInteractive()`. Context cancellation works correctly for all git operations (Status, Push, Pull, PushMultiple). Verified in existing tests. + +2. **collect/** — All HTTP requests use `http.NewRequestWithContext(ctx, ...)` for BitcoinTalk, CoinGecko, IACR, and arXiv collectors. The `RateLimiter.Wait()` method respects context cancellation. The `Excavator.Run()` checks `ctx.Err()` between collectors. Added `CheckGitHubRateLimitCtx(ctx)` for context-aware rate limit checking via `exec.CommandContext`. + +3. **agentci/security.go** — Added `SecureSSHCommandContext(ctx, host, cmd)` which uses `exec.CommandContext` for cancellable SSH operations. The original `SecureSSHCommand` is preserved (deprecated) and delegates to the context-aware version. + +4. **jobrunner/handlers/dispatch.go** — Updated `secureTransfer()`, `runRemote()`, and `ticketExists()` to pass their existing `ctx` parameter through to `SecureSSHCommandContext()`. Previously these methods accepted context but did not propagate it. + +**Recommendation:** When the Forgejo SDK v3 adds context support, a follow-up task should thread `ctx` through all forge/ and gitea/ wrapper signatures. + +### Rate Limiting Review + +**Current implementation (`collect/ratelimit.go`):** + +- Per-source delay tracking with configurable durations per source +- Default delays: GitHub 500ms, BitcoinTalk 2s, CoinGecko 1.5s, IACR 1s, arXiv 1s +- Unknown sources default to 500ms +- `Wait(ctx, source)` blocks until the rate limit allows, respects context cancellation +- `CheckGitHubRateLimit()` uses `gh api rate_limit` CLI to check GitHub API usage +- Auto-pauses at 75% usage by increasing GitHub delay to 5s + +**Assessment:** + +1. The rate limiter handles all current edge cases: context cancellation during wait, unknown sources, concurrent access (mutex-protected), and adaptive throttling. +2. GitHub rate limiting relies on the `gh` CLI rather than parsing HTTP response headers. This is appropriate because GitHub collection also uses the `gh` CLI (not direct HTTP). The `gh` CLI handles its own authentication and rate limit headers internally. +3. Forgejo/Gitea API calls go through their respective SDKs, which do not expose rate limit response headers. The SDKs handle HTTP internally. Adding header parsing would require forking or wrapping the SDK HTTP transport — not worth the complexity for the current usage pattern. +4. The `CheckGitHubRateLimitCtx` variant was added for context-aware cancellation of the `gh api` subprocess. diff --git a/agentci/config.go b/agentci/config.go index f2297c8..333faa3 100644 --- a/agentci/config.go +++ b/agentci/config.go @@ -42,7 +42,7 @@ func LoadAgents(cfg *config.Config) (map[string]AgentConfig, error) { continue } if ac.Host == "" { - return nil, fmt.Errorf("agent %q: host is required", name) + return nil, fmt.Errorf("agentci.LoadAgents: agent %q: host is required", name) } if ac.QueueDir == "" { ac.QueueDir = "/home/claude/ai-work/queue" @@ -125,10 +125,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 fmt.Errorf("agentci.RemoveAgent: no agents configured") } if _, ok := agents[name]; !ok { - return fmt.Errorf("agent %q not found", name) + return fmt.Errorf("agentci.RemoveAgent: agent %q not found", name) } delete(agents, name) return cfg.Set("agentci.agents", agents) diff --git a/agentci/security.go b/agentci/security.go index f917b3f..52b433f 100644 --- a/agentci/security.go +++ b/agentci/security.go @@ -1,6 +1,7 @@ package agentci import ( + "context" "fmt" "os/exec" "path/filepath" @@ -15,10 +16,10 @@ var safeNameRegex = regexp.MustCompile(`^[a-zA-Z0-9\-\_\.]+$`) func SanitizePath(input string) (string, error) { base := filepath.Base(input) if !safeNameRegex.MatchString(base) { - return "", fmt.Errorf("invalid characters in path element: %s", input) + return "", fmt.Errorf("agentci.SanitizePath: invalid characters in path element: %s", input) } if base == "." || base == ".." || base == "/" { - return "", fmt.Errorf("invalid path element: %s", base) + return "", fmt.Errorf("agentci.SanitizePath: invalid path element: %s", base) } return base, nil } @@ -30,8 +31,15 @@ func EscapeShellArg(arg string) string { } // SecureSSHCommand creates an SSH exec.Cmd with strict host key checking and batch mode. +// Deprecated: Use SecureSSHCommandContext for context-aware cancellation. func SecureSSHCommand(host string, remoteCmd string) *exec.Cmd { - return exec.Command("ssh", + return SecureSSHCommandContext(context.Background(), host, remoteCmd) +} + +// SecureSSHCommandContext creates an SSH exec.Cmd with context support for cancellation, +// strict host key checking, and batch mode. +func SecureSSHCommandContext(ctx context.Context, host string, remoteCmd string) *exec.Cmd { + return exec.CommandContext(ctx, "ssh", "-o", "StrictHostKeyChecking=yes", "-o", "BatchMode=yes", "-o", "ConnectTimeout=10", diff --git a/collect/ratelimit.go b/collect/ratelimit.go index 469d493..3068a0f 100644 --- a/collect/ratelimit.go +++ b/collect/ratelimit.go @@ -95,8 +95,16 @@ func (r *RateLimiter) GetDelay(source string) time.Duration { // CheckGitHubRateLimit checks GitHub API rate limit status via gh api. // Returns used and limit counts. Auto-pauses at 75% usage by increasing // the GitHub rate limit delay. +// Deprecated: Use CheckGitHubRateLimitCtx for context-aware cancellation. func (r *RateLimiter) CheckGitHubRateLimit() (used, limit int, err error) { - cmd := exec.Command("gh", "api", "rate_limit", "--jq", ".rate | \"\\(.used) \\(.limit)\"") + return r.CheckGitHubRateLimitCtx(context.Background()) +} + +// CheckGitHubRateLimitCtx checks GitHub API rate limit status via gh api with context support. +// Returns used and limit counts. Auto-pauses at 75% usage by increasing +// the GitHub rate limit delay. +func (r *RateLimiter) CheckGitHubRateLimitCtx(ctx context.Context) (used, limit int, err error) { + cmd := exec.CommandContext(ctx, "gh", "api", "rate_limit", "--jq", ".rate | \"\\(.used) \\(.limit)\"") out, err := cmd.Output() if err != nil { return 0, 0, core.E("collect.RateLimiter.CheckGitHubRateLimit", "failed to check rate limit", err) diff --git a/forge/labels.go b/forge/labels.go index 1418d49..4c0ea98 100644 --- a/forge/labels.go +++ b/forge/labels.go @@ -75,7 +75,7 @@ func (c *Client) GetLabelByName(owner, repo, name string) (*forgejo.Label, error } } - return nil, fmt.Errorf("label %s not found in %s/%s", name, owner, repo) + return nil, fmt.Errorf("forge.GetLabelByName: label %s not found in %s/%s", name, owner, repo) } // EnsureLabel checks if a label exists, and creates it if it doesn't. diff --git a/jobrunner/coverage_boost_test.go b/jobrunner/coverage_boost_test.go index a58c7cf..a1ea6dd 100644 --- a/jobrunner/coverage_boost_test.go +++ b/jobrunner/coverage_boost_test.go @@ -18,7 +18,7 @@ import ( func TestNewJournal_Bad_EmptyBaseDir(t *testing.T) { _, err := NewJournal("") require.Error(t, err) - assert.Contains(t, err.Error(), "journal base directory is required") + assert.Contains(t, err.Error(), "base directory is required") } func TestNewJournal_Good(t *testing.T) { diff --git a/jobrunner/handlers/dispatch.go b/jobrunner/handlers/dispatch.go index 845b242..4579b93 100644 --- a/jobrunner/handlers/dispatch.go +++ b/jobrunner/handlers/dispatch.go @@ -83,7 +83,7 @@ func (h *DispatchHandler) Execute(ctx context.Context, signal *jobrunner.Pipelin agentName, agent, ok := h.spinner.FindByForgejoUser(signal.Assignee) if !ok { - return nil, fmt.Errorf("unknown agent: %s", signal.Assignee) + return nil, fmt.Errorf("handlers.Dispatch.Execute: unknown agent: %s", signal.Assignee) } // Sanitize inputs to prevent path traversal. @@ -258,7 +258,7 @@ func (h *DispatchHandler) secureTransfer(ctx context.Context, agent agentci.Agen safeRemotePath := agentci.EscapeShellArg(remotePath) remoteCmd := fmt.Sprintf("cat > %s && chmod %o %s", safeRemotePath, mode, safeRemotePath) - cmd := agentci.SecureSSHCommand(agent.Host, remoteCmd) + cmd := agentci.SecureSSHCommandContext(ctx, agent.Host, remoteCmd) cmd.Stdin = bytes.NewReader(data) output, err := cmd.CombinedOutput() @@ -270,7 +270,7 @@ func (h *DispatchHandler) secureTransfer(ctx context.Context, agent agentci.Agen // runRemote executes a command on the agent via SSH. func (h *DispatchHandler) runRemote(ctx context.Context, agent agentci.AgentConfig, cmdStr string) error { - cmd := agentci.SecureSSHCommand(agent.Host, cmdStr) + cmd := agentci.SecureSSHCommandContext(ctx, agent.Host, cmdStr) return cmd.Run() } @@ -285,6 +285,6 @@ func (h *DispatchHandler) ticketExists(ctx context.Context, agent agentci.AgentC "test -f %s/%s || test -f %s/../active/%s || test -f %s/../done/%s", qDir, safeTicket, qDir, safeTicket, qDir, safeTicket, ) - cmd := agentci.SecureSSHCommand(agent.Host, checkCmd) + cmd := agentci.SecureSSHCommandContext(ctx, agent.Host, checkCmd) return cmd.Run() == nil } diff --git a/jobrunner/journal.go b/jobrunner/journal.go index c09ffcf..99eaf94 100644 --- a/jobrunner/journal.go +++ b/jobrunner/journal.go @@ -52,7 +52,7 @@ type Journal struct { // NewJournal creates a new Journal rooted at baseDir. func NewJournal(baseDir string) (*Journal, error) { if baseDir == "" { - return nil, fmt.Errorf("journal base directory is required") + return nil, fmt.Errorf("journal.NewJournal: base directory is required") } return &Journal{baseDir: baseDir}, nil } @@ -63,12 +63,12 @@ func NewJournal(baseDir string) (*Journal, error) { func sanitizePathComponent(name string) (string, error) { // Reject empty or whitespace-only values. if name == "" || strings.TrimSpace(name) == "" { - return "", fmt.Errorf("invalid path component: %q", name) + return "", fmt.Errorf("journal.sanitizePathComponent: invalid path component: %q", name) } // Reject inputs containing path separators (directory traversal attempt). if strings.ContainsAny(name, `/\`) { - return "", fmt.Errorf("path component contains directory separator: %q", name) + return "", fmt.Errorf("journal.sanitizePathComponent: path component contains directory separator: %q", name) } // Use filepath.Clean to normalize (e.g., collapse redundant dots). @@ -76,12 +76,12 @@ func sanitizePathComponent(name string) (string, error) { // Reject traversal components. if clean == "." || clean == ".." { - return "", fmt.Errorf("invalid path component: %q", name) + return "", fmt.Errorf("journal.sanitizePathComponent: invalid path component: %q", name) } // Validate against the safe character set. if !validPathComponent.MatchString(clean) { - return "", fmt.Errorf("path component contains invalid characters: %q", name) + return "", fmt.Errorf("journal.sanitizePathComponent: path component contains invalid characters: %q", name) } return clean, nil @@ -90,10 +90,10 @@ func sanitizePathComponent(name string) (string, error) { // Append writes a journal entry for the given signal and result. func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error { if signal == nil { - return fmt.Errorf("signal is required") + return fmt.Errorf("journal.Append: signal is required") } if result == nil { - return fmt.Errorf("result is required") + return fmt.Errorf("journal.Append: result is required") } entry := JournalEntry{ @@ -121,18 +121,18 @@ func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error { data, err := json.Marshal(entry) if err != nil { - return fmt.Errorf("marshal journal entry: %w", err) + return fmt.Errorf("journal.Append: marshal entry: %w", err) } data = append(data, '\n') // Sanitize path components to prevent path traversal (CVE: issue #46). owner, err := sanitizePathComponent(signal.RepoOwner) if err != nil { - return fmt.Errorf("invalid repo owner: %w", err) + return fmt.Errorf("journal.Append: invalid repo owner: %w", err) } repo, err := sanitizePathComponent(signal.RepoName) if err != nil { - return fmt.Errorf("invalid repo name: %w", err) + return fmt.Errorf("journal.Append: invalid repo name: %w", err) } date := result.Timestamp.UTC().Format("2006-01-02") @@ -141,27 +141,27 @@ func (j *Journal) Append(signal *PipelineSignal, result *ActionResult) error { // Resolve to absolute path and verify it stays within baseDir. absBase, err := filepath.Abs(j.baseDir) if err != nil { - return fmt.Errorf("resolve base directory: %w", err) + return fmt.Errorf("journal.Append: resolve base directory: %w", err) } absDir, err := filepath.Abs(dir) if err != nil { - return fmt.Errorf("resolve journal directory: %w", err) + return fmt.Errorf("journal.Append: resolve journal directory: %w", err) } if !strings.HasPrefix(absDir, absBase+string(filepath.Separator)) { - return fmt.Errorf("journal path %q escapes base directory %q", absDir, absBase) + return fmt.Errorf("journal.Append: path %q escapes base directory %q", absDir, absBase) } j.mu.Lock() defer j.mu.Unlock() if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("create journal directory: %w", err) + return fmt.Errorf("journal.Append: create directory: %w", err) } path := filepath.Join(dir, date+".jsonl") f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) if err != nil { - return fmt.Errorf("open journal file: %w", err) + return fmt.Errorf("journal.Append: open file: %w", err) } defer func() { _ = f.Close() }()