diff --git a/AGENTS.md b/AGENTS.md index ec3bc7167..e54c311cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,9 @@ Detailed guides for specific subsystems live in `contrib/claude/`: - [`contrib/claude/graphql.md`](contrib/claude/graphql.md) — Frontend Relay client (queries, fragments, mutations, pagination) - [`contrib/claude/commit.md`](contrib/claude/commit.md) — Commit message conventions - [`contrib/claude/license.md`](contrib/claude/license.md) — ISC license header (all file types) +- [`contrib/claude/go-testing.md`](contrib/claude/go-testing.md) — Go test conventions (parallel, require vs assert, naming) +- [`contrib/claude/go-worker.md`](contrib/claude/go-worker.md) — Go worker pattern (poll-based, bounded concurrency, FOR UPDATE SKIP LOCKED) +- [`contrib/claude/go-service.md`](contrib/claude/go-service.md) — Go service orchestration (Run, graceful shutdown, crash propagation) ## API Surface Rules @@ -152,46 +155,6 @@ Sentinel errors in grouped `var ()` blocks. Custom error types implement `Unwrap - Request structs: `*Request` suffix (e.g. `UpdateTrustCenterRequest`) - Unexported types for internal data: lowercase (e.g. `vendorInfo`, `ctxKey`) -### Tests - -Black-box test packages (`package foo_test`). White-box (`package foo`) only when testing unexported functions. Test names follow `TestFunctionName_Scenario`. Use `t.Run` for subtests with lowercase descriptive names. Table-driven tests for parameterized cases. - -**Parallel tests:** Always call `t.Parallel()` at both the top-level test and each subtest. - -**`require` vs `assert`:** -- `require` — stops the test immediately on failure. Use for **preconditions** that would make subsequent assertions meaningless: `require.NoError`, `require.Error`, `require.ErrorAs`, `require.Len`, `require.NotNil`, `require.True` (as a guard). -- `assert` — logs failure but continues the test. Use for the **actual values** being verified: `assert.Equal`, `assert.Contains`, `assert.True`, `assert.False`, `assert.Nil`. - -Rule of thumb: if a failure would cause a nil-pointer panic or make every following assertion nonsensical, use `require`; otherwise use `assert`. - -```go -func TestRun_Handoff(t *testing.T) { - t.Parallel() - - t.Run( - "handoff with custom tool name and description", - func(t *testing.T) { - t.Parallel() - - // ... setup ... - - result, err := triage.Run( - context.Background(), - []llm.Message{ - userMessage("How much is my invoice?"), - }, - ) - - require.NoError(t, err) - assert.Equal(t, "Your invoice is $42.", result.FinalMessage().Text()) - assert.Equal(t, "billing", result.LastAgent.Name()) - }, - ) -} -``` - -**Helpers and mocks:** Define mock types and helper functions (e.g. `stopResponse`, `userMessage`) at the top of the test file, not inline in each test. - ### Functional options and Config structs Use `Config` structs when a constructor has many required parameters. Use functional options (`With*` functions) for optional configuration. @@ -246,152 +209,3 @@ l.InfoCtx(ctx, "HTTP request to trust center custom domain, redirecting to HTTPS ) ``` -### Service `Run()` orchestration - -A top-level `Run` method starts child subsystems (workers, servers) as goroutines via `sync.WaitGroup.Go`. Each child gets its own cancellable context created with `context.WithCancel(context.WithoutCancel(ctx))` so that a parent cancellation does not kill in-flight work — the parent explicitly calls each `stop*` function and then `wg.Wait()` for a controlled shutdown. - -When a child crashes, it calls `cancel(fmt.Errorf("… crashed: %w", err))` to signal the parent. - -```go -func (impl *Implm) Run(ctx context.Context, l *log.Logger) error { - wg := sync.WaitGroup{} - ctx, cancel := context.WithCancelCause(ctx) - defer cancel(context.Canceled) - - // Start a worker - workerCtx, stopWorker := context.WithCancel(context.WithoutCancel(ctx)) - worker := NewFooWorker(pgClient, l.Named("foo-worker")) - wg.Go( - func() { - if err := worker.Run(workerCtx); err != nil { - cancel(fmt.Errorf("foo worker crashed: %w", err)) - } - }, - ) - - // Start a server - serverCtx, stopServer := context.WithCancel(context.WithoutCancel(ctx)) - defer stopServer() - wg.Go( - func() { - if err := impl.runServer(serverCtx, l); err != nil { - cancel(fmt.Errorf("server crashed: %w", err)) - } - }, - ) - - <-ctx.Done() - - stopServer() - stopWorker() - - wg.Wait() - - return context.Cause(ctx) -} -``` - -### Workers - -Background workers follow a poll-based pattern with bounded concurrency. The struct holds a `*pg.Client`, a `*log.Logger`, and tuning knobs (`interval`, `staleAfter`, `maxConcurrency`). Use functional options (`With*` functions) for the tuning knobs with sensible defaults. - -The `Run(ctx context.Context) error` method loops with a `select` on `ctx.Done()` and `time.After(interval)`. On each tick it recovers stale rows, then drains available work via `processNext`. Work items are claimed inside a transaction with `FOR UPDATE SKIP LOCKED`, marked as processing, then handled concurrently in goroutines bounded by a semaphore channel. Use `context.WithoutCancel` for work that must complete even after shutdown, and `sync.WaitGroup` with `defer wg.Wait()` to ensure in-flight goroutines finish before `Run` returns. - -```go -type ( - FooWorker struct { - pg *pg.Client - logger *log.Logger - interval time.Duration - staleAfter time.Duration - maxConcurrency int - } - - FooWorkerOption func(*FooWorker) -) - -func NewFooWorker( - pgClient *pg.Client, - logger *log.Logger, - opts ...FooWorkerOption, -) *FooWorker { - w := &FooWorker{ - pg: pgClient, - logger: logger, - interval: 10 * time.Second, - staleAfter: 5 * time.Minute, - maxConcurrency: 5, - } - for _, opt := range opts { - opt(w) - } - return w -} - -func (w *FooWorker) Run(ctx context.Context) error { - var ( - wg sync.WaitGroup - sem = make(chan struct{}, w.maxConcurrency) - ) - defer wg.Wait() - -LOOP: - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(w.interval): - nonCancelableCtx := context.WithoutCancel(ctx) - w.recoverStaleRows(nonCancelableCtx) - for { - if err := w.processNext(ctx, sem, &wg); err != nil { - if !errors.Is(err, coredata.ErrResourceNotFound) { - w.logger.ErrorCtx(nonCancelableCtx, "cannot claim item", log.Error(err)) - } - break - } - } - goto LOOP - } -} - -func (w *FooWorker) processNext(ctx context.Context, sem chan struct{}, wg *sync.WaitGroup) error { - select { - case sem <- struct{}{}: - case <-ctx.Done(): - return ctx.Err() - } - - var ( - item coredata.FooItem - now = time.Now() - nonCancelableCtx = context.WithoutCancel(ctx) - ) - - if err := w.pg.WithTx( - nonCancelableCtx, - func(tx pg.Conn) error { - if err := item.LoadNextPendingForUpdateSkipLocked(nonCancelableCtx, tx); err != nil { - return err - } - item.Status = coredata.FooStatusProcessing - item.UpdatedAt = now - return item.Update(nonCancelableCtx, tx, coredata.NewNoScope()) - }, - ); err != nil { - <-sem - return err - } - - wg.Add(1) - go func(item coredata.FooItem) { - defer wg.Done() - defer func() { <-sem }() - - if err := w.handle(nonCancelableCtx, &item); err != nil { - w.logger.ErrorCtx(nonCancelableCtx, "cannot process item", log.Error(err)) - } - }(item) - - return nil -} -```