diff --git a/analyze/prompts/_base.txt b/analyze/prompts/_base.txt index 4fe692c..8a7f8bf 100644 --- a/analyze/prompts/_base.txt +++ b/analyze/prompts/_base.txt @@ -3,11 +3,19 @@ ## Functions (mutually recursive group) {{range .Functions}} +### {{.Name}} +{{if .Godoc}}{{.Godoc}} +{{end}} ```go {{.Body}} ``` {{end -}} {{- else}} +{{- if .Godoc}} + +## Documentation +{{.Godoc}} +{{end -}} ```go {{.Body}} ``` @@ -77,7 +85,9 @@ Signature: {{.Signature}} {{- define "issues-format"}} -Respond with JSON. The "line" field must be the line number. The "code" field must contain the exact line of code where the issue occurs, copied verbatim from the code block above. +Severity scale: "critical" = exploitable security flaw or data corruption; "high" = incorrect behavior in normal use; "medium" = incorrect in edge cases or degraded reliability; "low" = poor practice with high probability of causing future bugs; "info" = minor concern. + +Respond with JSON. The "line" field must be the line number within the function code block shown above, counting from 1 at the first line of the function — not the absolute file line number. The "code" field must contain the exact line of code where the issue occurs, copied verbatim from the code block above. Example: {"issues": [{"function": "ParseConfig", "line": 42, "code": " query := \"SELECT * FROM users WHERE id=\" + id", "severity": "critical", "message": "SQL injection via string concatenation", "suggestion": "Use parameterized query"}]} diff --git a/analyze/prompts/baseline.txt b/analyze/prompts/baseline.txt index 7f36bdb..6728d6d 100644 --- a/analyze/prompts/baseline.txt +++ b/analyze/prompts/baseline.txt @@ -1,6 +1,25 @@ -You are reviewing Go code for common issues and mistakes. +You are reviewing Go code for logical correctness. {{template "function-context" .}} +{{- template "summary-context" .}} -Is this function correct? +Does this function correctly implement what it claims to do? Look for: + +Implementation vs. Intent: +- Behavior that contradicts the declared purpose, invariants, or documented error conditions +- A code path that returns success when it should return an error, or vice versa +- A precondition stated in the summary that the implementation does not actually enforce + +Logic Errors: +- Inverted boolean conditions (== where != is needed, < where > is needed) +- Wrong logical operator (&& vs ||) that produces incorrect short-circuit behavior +- Off-by-one errors in loop bounds, index expressions, or size calculations +- Unreachable code following an unconditional return, panic, or continue +- A switch or if-else chain that silently omits a case that must be handled +- Integer overflow or underflow in arithmetic that is not guarded + +Control Flow: +- A loop that cannot terminate under reachable inputs +- Early return that skips required cleanup or postcondition logic +- Error path that falls through to success-path code {{- template "issues-format" .}} diff --git a/analyze/prompts/concurrency.txt b/analyze/prompts/concurrency.txt index f1342f0..274c544 100644 --- a/analyze/prompts/concurrency.txt +++ b/analyze/prompts/concurrency.txt @@ -5,16 +5,33 @@ You are reviewing Go code for concurrency issues. {{- template "external-funcs-context" .}} ## Concurrency Review Checklist -Look for: + +Safety: - Race conditions on shared state without synchronization - Goroutine leaks (goroutines that never terminate) - Missing mutex locks or unlocks - Deadlock potential from lock ordering - Channel misuse (send on closed channel, nil channel operations) -- Missing select default cases causing blocking +- select without a default case where non-blocking dispatch was clearly intended - Improper use of sync.WaitGroup (Add after Wait, wrong count) -- Context cancellation not checked in long operations -- Shared state modified in goroutines without protection +- Context cancellation not checked in long-running operations - sync.Mutex copied by value +- sync.RWMutex used but reads acquire a full write lock instead of RLock — serializes all reads unnecessarily + +Design: +- Multiple goroutines writing to the same external resource — designate one owner goroutine; + all others produce immutable values and send them to the owner via a channel +- Mutex reached for as the first response to a shared-state problem when an immutable-value- + and-channel design would eliminate the shared state entirely +- errgroup or fan-in channel not used to collect results from parallel goroutines — instead, + results accumulated via a shared slice with a mutex, or goroutines write directly to an + external resource rather than sending immutable values to an owner + +Time: +- time.Now() compared against another time.Now() across goroutines to determine ordering — + wall clocks drift and NTP can move time backward; use a monotonic measurement (time.Since, + time.Now().Add) for elapsed time and timeouts; use wall-clock time only for display +- time.Now(), rand, or os.Getenv called inside concurrent logic instead of injected at the + boundary — hidden global state causes flaky tests and makes concurrent behavior non-deterministic {{- template "issues-format" .}} diff --git a/analyze/prompts/context-auth.txt b/analyze/prompts/context-auth.txt new file mode 100644 index 0000000..a531b48 --- /dev/null +++ b/analyze/prompts/context-auth.txt @@ -0,0 +1,36 @@ +You are reviewing Go code for context and authorization pattern violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} + +## Context Patterns + +- context.Context stored as a struct field — it must be passed as an explicit function parameter +- context.Value used to retrieve a typed domain value with an untyped key (string, int + literal) or an exported key type — keys must be unexported typed constants + (type contextKey int; const userContextKey contextKey = iota) so different packages cannot + collide; provide package-level accessors (UserFromContext) rather than exposing the key +- context.Value used to pass business data, feature flags, or configuration that should be + explicit function parameters — Value is for request-scoped cross-cutting concerns only + (trace IDs, auth identity) +- Context not propagated to a downstream call — DB methods (QueryContext, ExecContext), + outbound HTTP (http.NewRequestWithContext), and gRPC calls must receive the caller's ctx + +## Authorization + +Layer responsibility: the transport layer (HTTP handler or middleware) authenticates the caller +and stores identity on the context. Service and data layers extract identity from context and +enforce authorization. Look for violations of this boundary: + +- Service or repository method that calls context.WithValue to attach user identity — only + the transport layer should set auth context; downstream layers only read it +- Authorization enforced by fetching all rows and then filtering in application code — the + constraint must be embedded in the SQL WHERE clause so the database enforces it, not + application code after the fact +- Service method that accepts userID as an explicit parameter and passes it as an optional + filter rather than extracting it from context and treating it as a mandatory constraint +- Authorization decision made exclusively in middleware with no enforcement in the service or + data layer — callers that bypass the middleware (background jobs, CLI, tests) get no enforcement + +Only report actual violations. +{{- template "issues-format" .}} diff --git a/analyze/prompts/correctness.txt b/analyze/prompts/correctness.txt index 5108a70..ad1aa75 100644 --- a/analyze/prompts/correctness.txt +++ b/analyze/prompts/correctness.txt @@ -8,7 +8,7 @@ You are reviewing Go code for correctness issues. Error Handling: - Ignored errors (assigned to _ or not checked) -- Missing error wrapping (use fmt.Errorf with %w) +- Errors propagated without any wrapping or context (use fmt.Errorf with %w, or Op-based domain wrapping if a domain Error type is in use) - Panic for recoverable errors - Error shadowing in inner scopes @@ -23,7 +23,25 @@ Resource Management: - Missing defer for cleanup - defer in loops (accumulates until function returns) - Resources not released on error paths -- Context cancellation not checked in long operations - Missing timeouts on network operations +Error Type Discipline: +- Lookup-by-ID returning (nil, nil) instead of an ENOTFOUND error +- External errors (sql.ErrNoRows, os.ErrNotExist) escaping the implementation boundary untranslated +- Leaf errors carrying both Code and Err (leaf: Code+Message only; wrapping: Op+Err only) +- Missing Op wrap on significant function boundaries +- Type-asserting *Error directly at the call site instead of calling ErrorCode() and + ErrorMessage() — direct type assertions couple callers to the concrete Error struct and + break when the error is wrapped + +SQL / Data Access: +- defer tx.Rollback() not called immediately after a successful BeginTx — without it a failed + commit or a panic will leave the transaction open and hold database locks +- rows.Close() not deferred immediately after a successful QueryContext call +- Result slice initialized with var (encodes as JSON null) instead of make([]*T, 0) +- rows.Err() not checked after the iteration loop +- N+1 query pattern: one query issued per record inside a loop when a JOIN or batch fetch would retrieve the same data in one round trip +- Transaction held open while waiting for user input or an external HTTP response — transactions must be short; never span a user interaction +- Transaction exposed to service callers — *Tx or *sql.Tx accepted as a parameter or returned from a service method; transactions are an implementation detail and must not leak across the service boundary + {{- template "issues-format" .}} diff --git a/analyze/prompts/distributed.txt b/analyze/prompts/distributed.txt new file mode 100644 index 0000000..5f11719 --- /dev/null +++ b/analyze/prompts/distributed.txt @@ -0,0 +1,84 @@ +You are reviewing Go code for distributed systems reliability violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} +{{- template "external-funcs-context" .}} + +## Timeouts + +- Outbound HTTP call made with http.DefaultClient or a client with no Timeout set — a slow + upstream will hold the goroutine indefinitely, eventually exhausting the pool +- Database call made with a context that has no deadline — long-running queries will not be + cancelled when the caller gives up or the request is aborted +- Context passed to a network call is context.Background() or context.TODO() rather than the + request-scoped context received from the caller + +## Retries + +- Retry loop uses time.Sleep with a fixed interval — use exponential backoff with random jitter + (base * 2^n + jitter) to avoid coordinated retry storms when many clients fail simultaneously +- Retry loop does not classify errors as retryable before retrying — 400, 404, and validation + errors must not be retried; only transient errors (timeouts, 503, connection reset) should be +- Retry loop has no maximum attempt count or deadline — will retry indefinitely under sustained failure + +## Idempotency + +- Mutating operation (create entity, charge, send notification, enqueue job) that does not + check or assign a client-supplied idempotency key before executing — duplicate requests + produce duplicate side effects +- Idempotency key stored only in an in-memory map or cache — must be persisted durably so + deduplication survives process restarts; in-memory deduplication is lost on crash + +## Fan-out and Isolation + +- Independent downstream calls issued sequentially when they could run concurrently — use + errgroup and issue them in parallel; sequential fan-out multiplies latency unnecessarily +- No per-upstream goroutine limit or semaphore — a single slow dependency can exhaust all + available goroutines and starve unrelated request handling + +## Dual Writes + +- Two separate systems (database + cache, database + search index, two databases) written to + in sequence without a compensating mechanism — if the second write fails the systems are + permanently inconsistent; treat the event log or primary database as the source of truth and + let derived systems subscribe via CDC + +## Consistency + +- Read-modify-write cycle (SELECT then UPDATE) without SELECT FOR UPDATE or a compare-and- + swap on a version/ETag column — concurrent requests will silently overwrite each other +- SELECT FOR UPDATE issued without a context that has a deadline — if the lock holder stalls, + the waiting transaction will block indefinitely +- Counter or balance incremented with a read-modify-write cycle instead of an atomic + UPDATE counter = counter + ? — the read value may be stale by the time the write executes +- Last-write-wins (LWW) conflict resolution used where data loss is unacceptable — LWW + silently discards concurrent writes; use a version column, vector clock, or merge function +- Distributed lock used without a fencing token — if the lock holder pauses (GC, network + stall) after acquiring the lock, another node will acquire it; both nodes then believe they + hold the lock; every write to the shared resource must include the monotonic token and the + resource must reject writes with a stale token +- Database table polled in a tight loop to detect new work — polling causes table-scan load + and lock contention; use a purpose-built message queue (Kafka, RabbitMQ, SQS) or + LISTEN/NOTIFY instead + +## Caching + +- Cache used as the sole copy of data with no independent source of truth — when the cache is + empty the system must be able to repopulate it; if it cannot, the cache is actually a database + and should be treated as one +- Cached value written without an explicit TTL or invalidation strategy — unbounded TTLs cause + stale data to persist indefinitely across deployments and schema changes +- Cache miss under high load not protected against stampede — when a popular key expires many + requests miss simultaneously and all rebuild concurrently; use probabilistic early expiration, + request coalescing, or a background refresh pattern + +## Encoding + +- json.Marshal / json.Encode used to serialize data crossing an internal service boundary — + use binary encoding (Protocol Buffers, Thrift, Avro) for internal inter-service communication; + JSON is acceptable only for external-facing APIs +- Protocol Buffers field removed from a message definition without marking it reserved — + old consumers will re-use the field number for a different type, silently corrupting data + +Only report actual violations. +{{- template "issues-format" .}} diff --git a/analyze/prompts/domain-types.txt b/analyze/prompts/domain-types.txt new file mode 100644 index 0000000..1f7df2d --- /dev/null +++ b/analyze/prompts/domain-types.txt @@ -0,0 +1,47 @@ +You are reviewing Go code for domain type and CRUD convention violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} + +## CRUD Conventions + +FindByID / FindOne: +- Returns (nil, nil) when the entity does not exist — must return ENOTFOUND +- Does not wrap the callee's error with an Op before returning + +FindMany / List: +- Result slice declared with var (encodes as JSON null) — initialize with make([]*T, 0) +- Returns ENOTFOUND for an empty result set — an empty list is not an error +- Second return value omitted — FindMany must return ([]*T, int, error) where int is the + total count for pagination regardless of Limit/Offset + +Create: +- Does not mutate the input pointer to set ID, CreatedAt, or UpdatedAt on success +- Returns the new entity as a separate value rather than populating the caller's struct + +Update: +- Does not return the updated object on error — callers need it to replay a form +- Accepts individual fields rather than an *Update struct with pointer fields +- Uses a non-pointer field type in an Update struct — nil cannot express "do not change this field" + +Delete: +- Accepts more than the primary key as parameters — authorization and ownership checks belong + inside the implementation, not in the signature +- Checks authorization outside the implementation (in the handler) rather than enforcing it + inside the data layer using UserIDFromContext + +Filter structs: +- Filter field is a non-pointer type — nil cannot express "not filtered on" +- Accepts a raw offset integer for pagination — use a cursor (last-seen ID) to avoid skipped + or duplicated rows under concurrent writes + +Interface contracts: +- Exported service interface method missing a godoc comment that names the error codes it + can return — callers cannot know which errors to handle without this contract +- Service interface method whose first parameter is not context.Context — all service methods + must accept context.Context as their first argument for cancellation and auth propagation +- Caching or decoration wrapper that re-implements domain logic rather than delegating to the + wrapped service — wrappers must satisfy the same interface and add one concern only + +Only report actual violations of these conventions. +{{- template "issues-format" .}} diff --git a/analyze/prompts/functional-core.txt b/analyze/prompts/functional-core.txt new file mode 100644 index 0000000..7a1d39f --- /dev/null +++ b/analyze/prompts/functional-core.txt @@ -0,0 +1,50 @@ +You are reviewing Go code for functional core / imperative shell violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} +{{- template "external-funcs-context" .}} + +## I/O Mixed into Domain Logic + +A domain or business logic function should accept values and return new values without +performing I/O. A function that contains computation (sorting, filtering, ranking, validation, +pricing, scheduling) is difficult to test and reason about when it also performs I/O. Look for +functions that mix both: + +- Direct calls into database/sql, sqlx, pgx, or any ORM package alongside business logic +- Outbound HTTP calls (http.Get, http.Post, http.Client.Do) or gRPC client calls alongside + computation — move the I/O to the caller and pass the result in as a parameter +- time.Now() — inject the current time as a parameter; a hidden time.Now() call makes the + function non-deterministic and causes intermittent test failures +- rand.Float64(), rand.Intn(), or any global random call — inject the rand.Source or the + already-generated value; global random state cannot be controlled in tests +- os.Getenv, os.ReadFile, os.Open, or direct filesystem access — read at the boundary and + pass values in; domain functions should not know where configuration comes from +- Dependency accepted as a concrete struct type rather than an interface — concrete types + make the function untestable without the real implementation; accept the narrowest interface + that provides the methods the function actually calls + +## Mutation + +- Method mutates a field on its receiver when it could return a new value — domain types + should be transformed (accept value, return new value), not mutated in place; receiver + mutation makes behavior invisible to callers that hold the original value +- Function modifies a slice or map passed as a parameter — return a new collection so callers + are not surprised by hidden side effects on their own data structures + +## Shell Code Absorbing Logic + +The imperative shell (service methods, HTTP handlers, background workers) should be a flat +sequence of I/O calls and value assignments with minimal branching. When the shell accumulates +logic it can no longer be understood without reading it all, and that logic cannot be tested +without triggering I/O. Look for: + +- Sorting, filtering, ranking, or business rule evaluation done inline in a service method or + handler body rather than delegated to a pure function — extract the computation; call it + with real values in the shell +- Shell code with three or more conditional branches where the branch condition depends only + on values already in scope (no I/O needed to resolve it) — this logic belongs in a pure function + +Do not flag I/O in functions whose documented purpose is I/O (repository methods, HTTP +handlers, background pollers, main wiring code). +{{- template "issues-format" .}} diff --git a/analyze/prompts/http.txt b/analyze/prompts/http.txt new file mode 100644 index 0000000..28c7788 --- /dev/null +++ b/analyze/prompts/http.txt @@ -0,0 +1,65 @@ +You are reviewing Go code for HTTP service pattern violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} +{{- template "external-funcs-context" .}} + +## Entry Point and Wiring + +- os.Getenv called directly inside a handler, service, or any non-main function — environment + should be read once in run() and injected as a getenv parameter or explicit value +- flag package global functions (flag.String, flag.Parse, flag.Bool) used instead of + flag.NewFlagSet inside run() +- os.Exit called outside of main — return an error from run() instead +- signal.NotifyContext called in main() instead of inside run() — cancel will not be deferred correctly +- Fallible setup (database connection, config parsing) performed inside addRoutes — resolve + errors in run() before calling addRoutes +- addRoutes returns an error — it must not; all fallible setup belongs in run() + +## Handler Structure (maker func pattern) + +- Handler implemented as a method on a struct rather than returned from a maker function — + maker functions make one-time setup visible and keep per-handler state explicit +- Return type declared as http.HandlerFunc instead of http.Handler — third-party middleware + and router wrappers expect http.Handler +- Per-request work (regexp compilation, template parsing, prepared statement creation) done + inside the returned closure — it must be in the outer maker function scope so it runs once + at registration time, not on every request +- Durable application state accumulated in closure variables — use a database or service layer; + closure state does not survive restarts and does not replicate across instances +- sync.Once used for deferred setup with the error checked inside the Do function — the error + must be checked outside Do so it surfaces on every request after the first failure, not only on + the first attempt +- Middleware function type given a named type alias — func(http.Handler) http.Handler is + self-documenting; a named alias (type Middleware func(http.Handler) http.Handler) obscures + what the type is and complicates composition with third-party routers +- Mux has no explicit handler registered for "/" — always register http.NotFoundHandler() so + unmatched routes return a controlled 404 rather than the default ServeMux behavior + +## JSON Encoding + +- json.NewEncoder or json.NewDecoder called inline in a handler — all JSON encoding must go + through central encode/decode helpers so Content-Type is set consistently and error handling + is uniform +- Content-Type header set per-handler rather than once in the encode helper +- Validation (required fields, value ranges) performed inline in the handler body rather than + in a Valid(ctx context.Context) method on the request type + +## Error Translation + +- Domain error returned directly to the client without mapping — switch on ErrorCode(err) to + produce the right HTTP status (ENOTFOUND→404, EINVALID→400, EUNAUTHORIZED→401, + ECONFLICT→409, default→500) +- Internal error detail (Op chain, raw database error message) written to the response body — + send only the public ErrorMessage; log the internal detail server-side + +## Graceful Shutdown + +- httpServer.ListenAndServe error not compared against http.ErrServerClosed — the normal + shutdown path will be treated as an unexpected error +- httpServer.Shutdown called without a timeout context — a hung in-flight request can prevent + the process from ever exiting +- Process exits before wg.Wait() — in-flight requests may be abandoned mid-response + +Only report actual violations of these patterns. +{{- template "issues-format" .}} diff --git a/analyze/prompts/layout.txt b/analyze/prompts/layout.txt new file mode 100644 index 0000000..b8171c6 --- /dev/null +++ b/analyze/prompts/layout.txt @@ -0,0 +1,46 @@ +You are reviewing Go code for application layout violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} +{{- template "external-funcs-context" .}} + +## init() Functions + +init() must not perform side effects. If this is an init() function, look for: + +- Network connection opened (http.Get, sql.Open, grpc.Dial, redis.NewClient, etc.) — defer + this to an explicit Open() or Connect() constructor so errors are returned and callers control + the lifecycle +- File opened or read (os.Open, os.ReadFile, ioutil.ReadFile) — same; errors from init() cannot + be handled or retried +- Global variable assigned a value that depends on I/O or external state — any global var + requiring initialization that can fail belongs in an explicit constructor +- Mutation of a package-level variable shared by tests — init() runs once per binary; tests + cannot reset it; use constructor injection instead +- Flag registered with the global flag package (flag.String, flag.Bool) — global flags prevent + flag.NewFlagSet-based testing; register in run() instead + +## main() Functions + +main() must do nothing except call run() and handle the error. If this is a main() function, +look for: + +- Business logic (conditionals, loops, data transformation) — belongs in run() or a domain function +- Flag parsing (flag.Parse()) — belongs inside run() using flag.NewFlagSet(args[0], ...) +- Dependency construction (sql.Open, http.NewServeMux, constructors) — belongs inside run() +- os.Getenv called directly — belongs inside run() via the getenv parameter +- Anything beyond the pattern: `if err := run(...); err != nil { os.Exit(1) }` + +## Global State in Application Functions + +Dependencies must be injected explicitly; package-level variables must not carry application +state. In any non-init, non-main function, look for: + +- A DB connection, HTTP client, cache client, or config struct read from a package-level variable + rather than received as a parameter or struct field — hidden globals make functions impossible + to test in isolation and couple all callers to the same instance +- A singleton initialized via a package-level var declaration with a composite literal rather + than a constructor — callers share state they cannot control or replace + +Only report actual violations. +{{- template "issues-format" .}} diff --git a/analyze/prompts/maintainability.txt b/analyze/prompts/maintainability.txt index 918961c..6c1f26d 100644 --- a/analyze/prompts/maintainability.txt +++ b/analyze/prompts/maintainability.txt @@ -5,7 +5,7 @@ You are reviewing Go code for maintainability issues. ## Maintainability Review Checklist Complexity: -- Functions longer than 50 lines +- Functions over 80 lines that aren't justified by a single, clearly focused purpose - Deep nesting (more than 3 levels) - Functions with more than 5 parameters - Complex boolean expressions @@ -13,15 +13,40 @@ Complexity: Readability: - Magic numbers without named constants -- Unclear variable or function names - Complex logic without explanatory comments - Inconsistent naming conventions +- Named type not used for a pair or group of values that always travel together — a struct + with generic field names (Field1/Value, First/Second) conveys nothing at the call site +- Behavior that violates common reader expectations without prominent documentation: + a constructor that spawns background goroutines, a method that modifies a caller-supplied + parameter, a function named Get that has observable side effects API Design (exported functions only): -- Exported functions without documentation - Too many parameters (consider options struct) -- Boolean parameters that reduce readability +- Boolean parameters that reduce readability at the call site +- Pass-through method whose entire body is a single forwarding call with the same signature +- Shallow module: the interface is nearly as complex as the implementation — no meaningful + complexity is hidden, so callers gain nothing from the abstraction +- Configuration parameter exported when the module itself could provide a sensible default + and callers are unlikely to know a better value — pull the decision downward +- Domain constraint enforced by scattered if-checks at every call site rather than encoded + once in a type with a validated constructor — the compiler should enforce what it can +- Repetition: the same nontrivial logic (multi-step transformation, validation sequence, + query-building pattern) appears in two or more places without extraction into a shared helper +- Conjoined methods: two functions that cannot be understood independently because one silently + assumes state or invariants that only the other establishes — a caller who reads one without + the other will use it incorrectly + +Naming and Comments: +- Names too vague to guess meaning at the call site without documentation +- Boolean variable or field names that are not predicates (e.g. blinkStatus instead of cursorVisible) +- Comments that restate what the code already shows rather than explaining why +- Exported types, variables, or methods missing godoc comments +- Non-trivial exported function missing a contract comment specifying its preconditions and + postconditions — format: // Requires: ... (entry conditions) / // Ensures: ... (guarantees); + if Requires needs more than one sentence the function has too many entry conditions; if Ensures + requires enumerating many cases the function does too much Only report significant maintainability issues. Ignore minor style preferences. -Use severity "minor" for suggestions, "important" for real maintainability problems. +Use severity "low" for style suggestions, "medium" for real maintainability problems. {{- template "issues-format" .}} diff --git a/analyze/prompts/observability.txt b/analyze/prompts/observability.txt new file mode 100644 index 0000000..5b62c7c --- /dev/null +++ b/analyze/prompts/observability.txt @@ -0,0 +1,44 @@ +You are reviewing Go code for observability violations. +{{template "function-context" .}} +{{- template "summary-context" .}} +{{- template "callees-context" .}} +{{- template "external-funcs-context" .}} + +## Logging + +- fmt.Println, fmt.Printf, fmt.Fprintf(os.Stderr, ...), or log.Println used for application + logging — use a structured logger (slog, zap, zerolog) so log lines can be parsed, queried, + and correlated across services +- Log call in a request handler or service method missing a correlation ID or trace ID field — + every log line emitted during a request must carry trace context so lines can be joined + across service boundaries +- Error logged at more than one layer of the call stack — log once at the outermost boundary + that can take action; lower layers must return the error, not log it, or the same failure + produces duplicate log lines with conflicting context +- Log message describes what the code is doing rather than what happened or went wrong + ("calling database", "entering function") — log operational events and errors with enough + context to diagnose the problem without reading the source code +- Sensitive values (passwords, tokens, session IDs, PII) included as structured log field values + +## Metrics + +- HTTP handler does not record request duration — instrument with a histogram so SLO + percentile latency (p50/p95/p99) is computable; averages hide tail latency +- Error paths not counted separately from success paths — error rate requires a distinct + counter or label; the absence of a success increment is not equivalent to an error increment + +## Health Endpoints + +- Liveness and readiness checks implemented in the same endpoint — they answer different + questions: liveness (is the process alive?) vs readiness (are dependencies ready?); combining + them causes orchestrators to restart healthy but temporarily-unready instances + +## Tracing + +- Outbound HTTP call or database call made without propagating the trace context from the + incoming request's context — the downstream span will appear as a disconnected root in traces +- New root span created inside a handler or service method rather than a child span derived + from the incoming context — the request trace will be split across multiple unlinked roots + +Only report actual violations. +{{- template "issues-format" .}} diff --git a/analyze/prompts/security.txt b/analyze/prompts/security.txt index 8724e23..64c43e0 100644 --- a/analyze/prompts/security.txt +++ b/analyze/prompts/security.txt @@ -11,11 +11,16 @@ Look for these vulnerability categories: - Path Traversal: File operations with user-controlled paths, missing sanitization - XSS: Unescaped output in HTML contexts - SSRF: User-controlled URLs in HTTP requests -- Insecure Deserialization: Untrusted data in unmarshal calls +- Insecure Deserialization: Decoding user-controlled input without size limits (missing io.LimitReader on the body), or decoding into interface{}/any where attacker-supplied type data can violate caller assumptions - Hardcoded Secrets: API keys, passwords, tokens in code - Weak Cryptography: Insecure algorithms, weak random sources -- Race Conditions: Unsynchronized access to shared state - Information Disclosure: Sensitive data in logs or errors +- Insecure Direct Object Reference (IDOR): Resource accessed or modified by caller-supplied ID without verifying the caller has permission for that specific resource +- SQL Column Injection: Sort or filter columns interpolated from caller-supplied strings instead of mapping a fixed set of named values to SQL fragments +- Unsafe Deserialization: Language-native serialization (Python pickle, Java Serializable, Go gob with interface{}) used across process or service boundaries +- Integer Overflow in Allocation: caller-supplied length or count used in make(), new(), or a + size arithmetic expression without an upper-bound check — an attacker-controlled large value + can cause an enormous allocation, OOM, or wrap to a negative length Only report actual security issues. {{- template "issues-format" .}} diff --git a/analyze/prompts/summary.txt b/analyze/prompts/summary.txt index e5eb6ac..3583604 100644 --- a/analyze/prompts/summary.txt +++ b/analyze/prompts/summary.txt @@ -1,6 +1,7 @@ You are analyzing a Go function to create a summary of its behavior. {{template "function-context" .}} {{- template "callees-context" .}} +{{- template "external-funcs-context" .}} Analyze this function and respond with JSON in this exact format: { @@ -10,4 +11,8 @@ Analyze this function and respond with JSON in this exact format: "security": ["security-relevant properties for callers to know"] } -Be precise and concise. Focus on what callers need to know. +Field guidance: +- purpose: one sentence from the caller's perspective — what the function does, not how; do not restate the function name +- behavior: edge cases, side effects, non-obvious return value semantics, and error conditions; do not repeat the happy path already captured in purpose +- invariants: observable contracts — preconditions the caller must satisfy (e.g. "input must be non-empty"), postconditions guaranteed on success (e.g. "returned pointer is never nil"), and invariants that always hold; use an empty array if none apply +- security: include only if security-relevant: whether inputs are validated or sanitized, whether authentication or authorization is enforced, whether sensitive data is handled, whether output is escaped; use an empty array if nothing security-relevant applies diff --git a/analyze/prompts/testing.txt b/analyze/prompts/testing.txt new file mode 100644 index 0000000..cec9d32 --- /dev/null +++ b/analyze/prompts/testing.txt @@ -0,0 +1,75 @@ +You are reviewing Go test code for test mechanics violations. +{{template "function-context" .}} +{{- template "summary-context" .}} + +## Test Helpers + +- Test helper function missing t.Helper() — failure output will point inside the helper + rather than the call site that triggered it +- Test helper returns an error instead of calling t.Fatalf internally — callers must not + need to check a return value from a helper +- Cleanup not registered with t.Cleanup or returned as a func() — resources (temp files, + DB connections, servers) will leak across test cases + +## Assertions and Frameworks + +- Third-party assertion or mock library imported (testify/assert, testify/require, gomock, + gocheck, ginkgo, gomega) — use the stdlib testing package only; define assert, ok, and + equals helpers inline if needed +- Generated mock code from mockgen or similar tool — hand-write mocks in a mock package + with one Fn func field and one Invoked bool field per method +- Test that asserts only what the compiler already enforces — if the test passes solely + because the type system prevents any other outcome (e.g. asserting a typed function returns + a string), the test adds no information and should be deleted +- Mock used to test a pure domain function — pure functions accept values and return values; + they need no test doubles; if a mock is necessary the function is not pure and should be split + at the I/O boundary +- net.Conn mocked in a network test — make real loopback connections with net.Listen on + 127.0.0.1:0 (OS assigns the port); mocking net.Conn produces tests that cannot catch real + protocol or framing bugs + +## Timing + +- time.Sleep called with a fixed duration — use select with time.After and a package-level + timeMultiplier variable so CI environments can scale the timeout without code changes +- Async condition polled in a loop with time.Sleep — signal completion via a channel or use + a context with deadline and select + +## Parallelism and Isolation + +- t.SetEnv used — it disables t.Parallel() for the entire test binary; inject getenv as a + parameter to the function under test instead +- t.Parallel() called in a test that reads or writes package-level variables or global state +- t.Parallel() absent in a test that constructs a fully isolated environment — own server, + own in-memory DB, injected dependencies, OS-assigned port (127.0.0.1:0) + +## Structure + +- Sub-cases not wrapped in t.Run — cases cannot be targeted individually with -run and + defer does not scope correctly within each case +- Table-driven test cases defined as a slice ([]struct{...}) rather than a map + (map[string]struct{...}) — slice indices appear in failure output ("case[2]") rather than + names ("case basic"), making failures ambiguous without counting into the slice +- Test of an unexported function as a primary strategy — test the exported API; if the + exported API behaves correctly, the unexported internals are correct by definition; if the + internals can only be verified by reaching around the public interface, the boundary is wrong +- runtime.Caller or os.Getwd used to build fixture file paths — go test sets the working + directory to the package directory; use relative paths (filepath.Join("testdata", "...")) +- init() used to set global state that tests cannot override — use constructor injection +- sync.Once used to initialize a global singleton that tests need to reinitialize — once + initialized state cannot be reset between tests; use constructor injection +- Complex expected output hardcoded as a string literal inline in the test — use a golden + file with an -update flag so the expected output is reviewable as a diff and updatable + without editing the test source +- Integration or acceptance test not guarded by a flag or build tag — expensive tests must + opt in: var flagAcceptance = flag.Bool("acceptance", false, "run acceptance tests") + +## Interface Width + +- Interface defined in the implementing package and exported for callers to use — define + the interface at the point of use (in the test or the consuming package) +- Interface accepted by the function under test is wider than the methods actually called — + narrow it so test doubles require less scaffolding and the contract is more explicit + +Only report actual violations. +{{- template "issues-format" .}}