From 4145f7a50b308754624dc89e683ba8b1c2b37d40 Mon Sep 17 00:00:00 2001 From: thuanle Date: Tue, 28 Apr 2026 05:18:18 +0700 Subject: [PATCH] fix: remove out-of-scope plan files and gofmt test file Address review feedback on PR #13: - Remove docs/superpowers/plans/ files not related to issue #4 - gofmt internal/mail/resolve_thread_test.go Co-Authored-By: Claude Opus 4.7 --- .../plans/2026-04-27-callback-base-url.md | 172 -------------- .../plans/2026-04-27-imap-proxy-support.md | 224 ------------------ internal/mail/resolve_thread_test.go | 2 +- 3 files changed, 1 insertion(+), 397 deletions(-) delete mode 100644 docs/superpowers/plans/2026-04-27-callback-base-url.md delete mode 100644 docs/superpowers/plans/2026-04-27-imap-proxy-support.md diff --git a/docs/superpowers/plans/2026-04-27-callback-base-url.md b/docs/superpowers/plans/2026-04-27-callback-base-url.md deleted file mode 100644 index 537a4af..0000000 --- a/docs/superpowers/plans/2026-04-27-callback-base-url.md +++ /dev/null @@ -1,172 +0,0 @@ -# Callback Base URL Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Separate server bind address from callback URL generation and default callback base to localhost when unset. - -**Architecture:** Keep `LISTEN_ADDR` only for HTTP server binding. Add `CALLBACK_BASE_URL` in config, default to `http://localhost` when empty, and build dispatch callback URL from this base plus `/callback`. - -**Tech Stack:** Go, stdlib `net/url` + `strings`, existing project packages. - ---- - -### Task 1: Add callback base URL config with localhost fallback - -**Files:** -- Modify: `internal/config/config.go` -- Test: `internal/config/config_test.go` (create) - -- [ ] **Step 1: Write the failing tests** - -```go -func TestLoad_DefaultCallbackBaseURL_WhenUnset(t *testing.T) { - t.Setenv("CALLBACK_BASE_URL", "") - setRequiredEnv(t) - - cfg, err := Load() - require.NoError(t, err) - require.Equal(t, "http://localhost", cfg.CallbackBaseURL) -} - -func TestLoad_UsesCallbackBaseURL_WhenSet(t *testing.T) { - t.Setenv("CALLBACK_BASE_URL", "https://bridge.example.com") - setRequiredEnv(t) - - cfg, err := Load() - require.NoError(t, err) - require.Equal(t, "https://bridge.example.com", cfg.CallbackBaseURL) -} -``` - -- [ ] **Step 2: Run tests to verify failure** - -Run: `go test ./internal/config -run CallbackBaseURL -v` -Expected: FAIL because `Config` has no `CallbackBaseURL` field yet. - -- [ ] **Step 3: Write minimal implementation** - -```go -type Config struct { - // ...existing fields... - CallbackBaseURL string -} - -cfg := &Config{ - // ...existing env reads... - CallbackBaseURL: os.Getenv("CALLBACK_BASE_URL"), -} - -if cfg.CallbackBaseURL == "" { - cfg.CallbackBaseURL = "http://localhost" -} -``` - -- [ ] **Step 4: Run tests to verify pass** - -Run: `go test ./internal/config -run CallbackBaseURL -v` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add internal/config/config.go internal/config/config_test.go -git commit -m "feat(config): add callback base URL with localhost default" -``` - -### Task 2: Build callback URL from callback base URL - -**Files:** -- Modify: `internal/ai_client/client.go` -- Test: `internal/ai_client/client_test.go` (create) - -- [ ] **Step 1: Write the failing tests** - -```go -func TestBuildCallbackURL_DefaultLocalhostBase(t *testing.T) { - got := buildCallbackURL("http://localhost") - require.Equal(t, "http://localhost/callback", got) -} - -func TestBuildCallbackURL_TrimsTrailingSlash(t *testing.T) { - got := buildCallbackURL("https://bridge.example.com/") - require.Equal(t, "https://bridge.example.com/callback", got) -} -``` - -- [ ] **Step 2: Run tests to verify failure** - -Run: `go test ./internal/ai_client -run BuildCallbackURL -v` -Expected: FAIL because helper function does not exist. - -- [ ] **Step 3: Write minimal implementation** - -```go -func buildCallbackURL(base string) string { - base = strings.TrimRight(base, "/") - return base + "/callback" -} - -// in Dispatch: -callbackURL := buildCallbackURL(d.cfg.CallbackBaseURL) -``` - -- [ ] **Step 4: Run tests to verify pass** - -Run: `go test ./internal/ai_client -run BuildCallbackURL -v` -Expected: PASS. - -- [ ] **Step 5: Commit** - -```bash -git add internal/ai_client/client.go internal/ai_client/client_test.go -git commit -m "fix(ai-client): use callback base URL for callback endpoint" -``` - -### Task 3: Update environment docs/examples - -**Files:** -- Modify: `.env.example` -- Modify: `README.md` - -- [ ] **Step 1: Add config key to `.env.example`** - -```dotenv -LISTEN_ADDR=:8080 -CALLBACK_BASE_URL=http://localhost -``` - -- [ ] **Step 2: Document purpose in README** - -```md -- `LISTEN_ADDR`: local bind address for HTTP server (e.g. `:8080`) -- `CALLBACK_BASE_URL`: public base URL used to build callback endpoint; defaults to `http://localhost` when unset -``` - -- [ ] **Step 3: Run full test suite** - -Run: `go test ./...` -Expected: PASS. - -- [ ] **Step 4: Commit** - -```bash -git add .env.example README.md -git commit -m "docs: document callback base URL configuration" -``` - -### Task 4: Prepare PR - -**Files:** -- No code changes expected - -- [ ] **Step 1: Push branch** - -Run: `git push -u origin ` -Expected: branch published. - -- [ ] **Step 2: Open PR with summary and test evidence** - -Include: -- Problem: callback URL incorrectly built from listen addr -- Fix: separate `CALLBACK_BASE_URL` with localhost fallback -- Tests: `go test ./internal/config -run CallbackBaseURL -v`, `go test ./internal/ai_client -run BuildCallbackURL -v`, `go test ./...` diff --git a/docs/superpowers/plans/2026-04-27-imap-proxy-support.md b/docs/superpowers/plans/2026-04-27-imap-proxy-support.md deleted file mode 100644 index 30879bf..0000000 --- a/docs/superpowers/plans/2026-04-27-imap-proxy-support.md +++ /dev/null @@ -1,224 +0,0 @@ -# IMAP Proxy Support Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make IMAP connections route through `IMAP_PROXY_URL` when configured, while keeping current direct dial behavior when unset. - -**Architecture:** Introduce a small dial abstraction in IMAP watcher so connection setup is testable and can branch between direct TLS dial and SOCKS5 proxy dial. Keep authentication/fetch logic unchanged; only connection creation path changes. Validate proxy URL format early and fail clearly when invalid. - -**Tech Stack:** Go, `github.com/emersion/go-imap/v2/imapclient`, `golang.org/x/net/proxy`, `crypto/tls`. - ---- - -### Task 1: Add failing tests for dial-path behavior - -**Files:** -- Modify: `internal/mail/imap_test.go` (create) -- Modify: `internal/mail/imap.go` (for injectable dial function used by tests) - -- [ ] **Step 1: Write failing test for direct dial when proxy unset** - -```go -func TestConnectAndWatch_UsesDirectDial_WhenProxyUnset(t *testing.T) { - watcher := &IMAPWatcher{cfg: &config.Config{IMAPHost: "imap.example.com", IMAPPort: "993", IMAPUser: "u", IMAPPass: "p"}} - - var directCalled bool - watcher.dialIMAP = func(addr string, _ *imapclient.Options) (*imapclient.Client, error) { - directCalled = true - return nil, errors.New("stop") - } - - _ = watcher.connectAndWatch(context.Background()) - - if !directCalled { - t.Fatal("expected direct dial path") - } -} -``` - -- [ ] **Step 2: Write failing test for proxy dial when proxy set** - -```go -func TestConnectAndWatch_UsesProxyDial_WhenProxySet(t *testing.T) { - watcher := &IMAPWatcher{cfg: &config.Config{IMAPHost: "imap.example.com", IMAPPort: "993", IMAPUser: "u", IMAPPass: "p", IMAPProxyURL: "socks5://127.0.0.1:1080"}} - - var proxyCalled bool - watcher.dialIMAPViaProxy = func(addr, proxyURL string, _ *imapclient.Options) (*imapclient.Client, error) { - proxyCalled = true - return nil, errors.New("stop") - } - - _ = watcher.connectAndWatch(context.Background()) - - if !proxyCalled { - t.Fatal("expected proxy dial path") - } -} -``` - -- [ ] **Step 3: Run tests to verify RED** - -Run: `go test ./internal/mail -run Uses -v` -Expected: FAIL because `dialIMAP` / `dialIMAPViaProxy` hooks do not exist yet. - -- [ ] **Step 4: Commit test scaffold after it compiles with new hooks (still failing behavior checks if needed)** - -```bash -git add internal/mail/imap_test.go internal/mail/imap.go -git commit -m "test(mail): add dial path tests for IMAP proxy" -``` - -### Task 2: Implement proxy-aware dialing with minimal changes - -**Files:** -- Modify: `internal/mail/imap.go` - -- [ ] **Step 1: Add dial hooks to watcher for testability** - -```go -type IMAPWatcher struct { - cfg *config.Config - db *gorm.DB - client *imapclient.Client - - dialIMAP func(addr string, options *imapclient.Options) (*imapclient.Client, error) - dialIMAPViaProxy func(addr, proxyURL string, options *imapclient.Options) (*imapclient.Client, error) - - OnReceived func(task *database.Task) -} -``` - -- [ ] **Step 2: Initialize default dial hooks in constructor** - -```go -func NewIMAPWatcher(cfg *config.Config, db *gorm.DB) *IMAPWatcher { - return &IMAPWatcher{ - cfg: cfg, - db: db, - dialIMAP: imapclient.DialTLS, - dialIMAPViaProxy: dialTLSViaSOCKS5, - } -} -``` - -- [ ] **Step 3: Branch connection setup by `IMAPProxyURL`** - -```go -if strings.TrimSpace(w.cfg.IMAPProxyURL) == "" { - c, err = w.dialIMAP(addr, options) -} else { - c, err = w.dialIMAPViaProxy(addr, w.cfg.IMAPProxyURL, options) -} -if err != nil { - return fmt.Errorf("dial IMAP: %w", err) -} -``` - -- [ ] **Step 4: Implement SOCKS5 TLS dial helper** - -```go -func dialTLSViaSOCKS5(addr, proxyURL string, options *imapclient.Options) (*imapclient.Client, error) { - u, err := url.Parse(proxyURL) - if err != nil { - return nil, fmt.Errorf("parse proxy url: %w", err) - } - if u.Scheme != "socks5" { - return nil, fmt.Errorf("unsupported proxy scheme: %s", u.Scheme) - } - - var auth *proxy.Auth - if u.User != nil { - pw, _ := u.User.Password() - auth = &proxy.Auth{User: u.User.Username(), Password: pw} - } - - d, err := proxy.SOCKS5("tcp", u.Host, auth, proxy.Direct) - if err != nil { - return nil, fmt.Errorf("create socks5 dialer: %w", err) - } - - conn, err := d.Dial("tcp", addr) - if err != nil { - return nil, fmt.Errorf("proxy dial: %w", err) - } - - host, _, _ := net.SplitHostPort(addr) - tlsConn := tls.Client(conn, &tls.Config{ServerName: host}) - if err := tlsConn.Handshake(); err != nil { - _ = tlsConn.Close() - return nil, fmt.Errorf("tls handshake: %w", err) - } - - return imapclient.New(tlsConn, options), nil -} -``` - -- [ ] **Step 5: Run focused tests to verify GREEN** - -Run: `go test ./internal/mail -run Uses -v` -Expected: PASS. - -- [ ] **Step 6: Commit implementation** - -```bash -git add internal/mail/imap.go internal/mail/imap_test.go -git commit -m "feat(mail): support IMAP SOCKS5 proxy dialing" -``` - -### Task 3: Add URL validation coverage and docs consistency - -**Files:** -- Modify: `internal/mail/imap_test.go` -- Modify: `README.md` (only if wording needs tighter scheme requirement) - -- [ ] **Step 1: Write test for invalid proxy scheme** - -```go -func TestDialTLSViaSOCKS5_RejectsNonSocks5Scheme(t *testing.T) { - _, err := dialTLSViaSOCKS5("imap.example.com:993", "http://proxy:8080", nil) - if err == nil || !strings.Contains(err.Error(), "unsupported proxy scheme") { - t.Fatalf("expected unsupported scheme error, got %v", err) - } -} -``` - -- [ ] **Step 2: Run targeted test** - -Run: `go test ./internal/mail -run RejectsNonSocks5Scheme -v` -Expected: PASS. - -- [ ] **Step 3: Ensure README explicitly says socks5 scheme (already present, keep if unchanged)** - -```md -IMAP_PROXY_URL: socks5://user:pass@host:port -``` - -- [ ] **Step 4: Commit doc/test alignment** - -```bash -git add internal/mail/imap_test.go README.md -git commit -m "test/docs: validate IMAP proxy URL scheme" -``` - -### Task 4: Final verification - -**Files:** -- No new files expected - -- [ ] **Step 1: Run package tests for mail** - -Run: `go test ./internal/mail -v` -Expected: PASS. - -- [ ] **Step 2: Run full suite** - -Run: `go test ./...` -Expected: PASS. - -- [ ] **Step 3: Push and update issue/PR with evidence** - -```bash -git push -u origin -``` - -Include commands and pass results in issue/PR comment. diff --git a/internal/mail/resolve_thread_test.go b/internal/mail/resolve_thread_test.go index 9e52251..054c489 100644 --- a/internal/mail/resolve_thread_test.go +++ b/internal/mail/resolve_thread_test.go @@ -83,4 +83,4 @@ func TestResolveThreadID_LookupError_ReturnsMessageID(t *testing.T) { } } -var nilLookup = func(string) (*database.Task, error) { return nil, nil } \ No newline at end of file +var nilLookup = func(string) (*database.Task, error) { return nil, nil }