Review Microservice

CRITICAL: Scope this review strictly to the microservice directory you are asked to review. Do NOT explore or analyze other microservices.

CRITICAL: Perform this review sequentially in the current context. Do NOT launch subagents to parallelize the steps unless the user explicitly asks to run the review concurrently or asks for a faster review.

Naming convention used in this skill: service.go refers collectively to all hand-written .go implementation files in the microservice directory (excluding intermediate.go, mock.go, and test files). service_test.go refers collectively to all *_test.go files. In most microservices these are single files, but they may be split across multiple files in rare cases. Step 1 identifies the actual files; subsequent steps use these shorthand names.

Workflow

Copy this checklist and track your progress:

Microservice review:
- [ ] Step 1: Read the microservice
- [ ] Step 2: Implementation completeness
- [ ] Step 3: Framework compliance
- [ ] Step 4: Test quality
- [ ] Step 5: Spec consistency
- [ ] Step 6: Code quality
- [ ] Step 7: Security
- [ ] Step 8: Documentation
- [ ] Step 9: API design
- [ ] Step 10: Performance and data access
- [ ] Step 11: Run vet and tests
- [ ] Step 12: Produce the report

Step 1: Read the Microservice

List the directory to identify all files. Read myserviceapi/definition.go (the API spec), manifest.yaml, and the generated intermediate.go, client.go, and mock.go. Read all hand-written .go implementation files (service.go and any others) and all *_test.go files.

Build a feature inventory from the define.* vars in definition.go. This is the ground truth for what the microservice exposes; each var's kind (define.Function, define.Web, define.Config, define.Task, define.Workflow, define.OutboundEvent, define.InboundEvent, define.Metric, define.Ticker) gives the feature type directly.

Step 2: Implementation Completeness

For each feature in the inventory (each define.* var in definition.go), verify it is implemented and tested:

• Functions, webs, tasks, inbound events, tickers - a handler method in service.go, and at least one test case in service_test.go.
• Workflows - a graph builder method in service.go, and a test.
• Config callbacks / observable metrics - an OnChangedFeatureName / OnObserveFeatureName method in service.go when the var sets Callback: true / Observable: true; service_test.go coverage is desirable but optional.
• Outbound events, non-callback configs, non-observable metrics - no service.go handler is expected.

Report any feature whose required handler or test is missing.

The generated files (intermediate.go, client.go, mock.go, mock_test.go, manifest.yaml) are projections of definition.go and are always mutually consistent, so they are not audited feature-by-feature here. Instead, run go run github.com/microbus-io/fabric/cmd/genservice -check <microservice dir> and flag any file it reports as out of date (the generated files were edited by hand or never regenerated).

Step 3: Framework Compliance

Grep the hand-written .go files (same set as Step 1) for violations of Microbus framework conventions:

• http.Client, http.Get, http.Post, http.DefaultClient - outbound HTTP must go through the HTTP egress proxy (httpegressapi)
• Bare goroutines - go func or go followed by a function call must use svc.Go(ctx, func) instead
• fmt.Errorf - must use errors.New instead
• return err without errors.Trace(err) - all non-nil error returns must be traced (allow exceptions for nil checks and errors.Trace already applied in the same line)
• fmt.Print, log.Print, println - structured logging must use svc.LogDebug/Info/Warn/Error
• context.Background() or context.TODO() - handlers must propagate the received context, not create a new root context
• errors.Newc, errors.Newf, errors.Newcf - deprecated constructors; use errors.New instead
• httpx.PathValues - deprecated; use r.PathValue("name") instead
• Resource lifecycle - resources opened or subscribed in OnStartup (connections, file handles, hooks) should have a corresponding release or unsubscribe in OnShutdown. Flag asymmetric lifecycle management.
• Foreman call sites (foremanapi.NewClient) - grep this microservice's hand-written files for foremanapi.NewClient. The foreman is the execution engine, not a downstream dependency, so most call sites are anti-patterns. Flag each of these:
  • a function or web handler that calls foremanapi...Run (or Create+Await) and returns the flow's result - a synchronous endpoint wrapping a workflow, running an open-ended flow on the caller's request budget and coupling an API provider to the foreman (wrong whether the workflow is this service's or another's; fix with a shared Go helper the handler and the task body both call);
  • a define.Task handler that calls foremanapi at all (Run, Create, Await, Snapshot, Cancel, Resume) - a task must never reach the foreman. To invoke another microservice's workflow as a child from inside a task body, use flow.Subgraph(otherflowapi.OtherFlow.URL(), input) or the downstream microservice's generated otherapi.NewSubgraph(flow) client (return nil while yield=true, adopt out on re-entry). The foreman call spawns a detached flow that loses re-entry, cascading cancel, single-flow audit trail, and interrupt propagation. Flag every foremanapi reference inside a task handler;
  • a pass-through endpoint forwarding to foremanapi.Create/Run/Cancel/Await/Snapshot/Resume (Run<Workflow>, <Workflow>Status, ...) - the trigger code should call the foreman directly.
• Legitimate launch site - do not flag, but check duration handling - a foremanapi.Run/Create is correct in code that owns a triggering event this service genuinely holds and does not block a caller on the flow's result: its own ticker, an inbound event sink, or a UI/webhook action that fires the flow and returns a handle or redirect (an acceptable shortcut for a self-contained service; a separate launcher microservice is cleaner). There, verify an Await handles the timeout (poll-later handle, error, compensating cancel). There is no stop-notification event; a follow-up that must happen reliably belongs in the workflow as its own durable task (an orchestrating graph that runs the real work as a subgraph and routes success/failure to separate retryable tasks), not in a caller's Await.

Agentic workflow internals (skip if this microservice has no tasks or workflows). A graph and its tasks live entirely inside this directory, so their internal correctness is reviewed here; cross-service composition (launcher-vs-provider placement, main.go wiring, subgraph boundaries into another microservice's workflow) is a review-architecture concern.

• Reducer correctness at fan-in - for every graph.SetFanIn(node), check that every state field a branch writes is either replace-safe (last write wins is intended) or has an explicit graph.SetReducer(field, reducer). The default is Replace; a branch that writes a messages field expecting accumulation silently loses siblings' contributions without a reducer. Match the reducer to the semantic: Append for ordered deltas, Add for numeric sums, Union for sets, Merge for objects, And/Or for booleans, Concat for strings.
• Subgraph state hygiene - when this graph calls flow.Subgraph and the parent and child use different field names or one must curate state at the boundary, look for a small adapter task (Before<Node> / After<Node>) using flow.Del / flow.Clear. In-task state mutation scattered through the graph to paper over a boundary mismatch is a smell; the fix is one adapter task at the boundary.
• Task idempotency - tasks may run more than once for the same logical step (flow.Retry, foreman lease recovery, subgraph re-entry, manual Retry). A task that fires an external side effect (charging a card, sending an email, mutating a non-transactional store) must carry a dedupe key or check first whether the effect already happened. Pure-state tasks need no guard. Flag any external side effect without an obvious idempotency guard.
• Long-running tasks free the worker - a task that intrinsically exceeds the foreman's TimeBudget ceiling (default 2m, max 15m) must use Interrupt/Resume or Sleep/Retry polling, never block in time.Sleep or a synchronous external wait. Grep tasks for long blocking calls that ignore the return of svc.Sleep, or for polling shapes lacking flow.Retry / flow.Interrupt.
• LLM tool exposure shape - if the workflow uses llmapi.ChatLoop as a subgraph, the prepare task should produce toolURLs []string of canonical endpoint URLs (calculatorapi.Arithmetic.URL() style), not a pre-built []llmapi.Tool. Pre-resolving Tool schemas duplicates what InitChat does internally and bypasses the actor-aware OpenAPI fetch that gates tools by requiredClaims. Flag any prepare task that constructs llmapi.Tool literals.

Step 4: Test Quality

Review all *_test.go files in the microservice directory (commonly just service_test.go, but tests may be split across multiple files):

• Does each integration test function (TestXxx_FeatureName) have at least one assert call? Flag empty test bodies or tests that call the endpoint but make no assertions.
• Does each integration test initialize a proper application.New() + RunInTest(t) harness?
• Are downstream dependencies mocked? Flag tests that would fail if a real downstream service is unavailable.
• Is mock_test.go present and unmodified? It is generated by cmd/genservice and should not be edited by hand. Flag any service that lacks it or whose TestXxx_Mock lives in service_test.go instead.
• Are top-level test functions run with t.Parallel()? A missing t.Parallel() is acceptable if the same line is preceded by a comment of the form // No parallel: {reason} explaining why.
• Mock handler quality - do mock handler functions in MockXxx calls exercise the feature in a meaningful way, or do they return zero values unconditionally? Flag mocks that always return the same constant regardless of inputs, as these may mask bugs in the caller.
• HINT scaffold comments - /* HINT: ... */ or // HINT: ... blocks in test files are intentional placeholders for future work. Do not flag their presence.

Step 5: Spec Consistency

manifest.yaml and the other generated files are projections of definition.go, so review the spec itself:

• Is every feature in definition.go implemented in service.go per Step 2, and is every service.go handler backed by a define.* var (no orphan handlers)?
• Does every define.* var have a meaningful godoc (it becomes the description), not placeholder text?
• Config defaults in range - for each define.Config with both a Default and a range Validation, verify the default falls within the declared range.
• Dead metrics - for each define.Metric, check that the generated svc.IncrementXxx / svc.RecordXxx recorder is actually called somewhere in the hand-written code. Flag metrics that are declared but never recorded.
• Metrics coverage - are this microservice's key operations (payments, user actions, external API calls, expensive computations) instrumented with a counter, gauge, or histogram? Flag important operations that carry no metric.
• Histogram buckets - for each histogram metric, are the Buckets appropriate for the expected value range? Flag buckets that cannot cover the real distribution (e.g. latency buckets maxing out at 10ms for an operation that routinely takes seconds).

Step 6: Code Quality

• Route conventions - are routes in kebab-case? Routes typically follow the handler name (e.g. MyHandler → /my-handler) but deviations are acceptable when intentional (e.g. REST-style paths, versioned routes). Flag routes that appear inconsistent without obvious reason.
• Port conventions - conventional port assignments are :443 for standard endpoints, :444 for internal-only, :888 for management, :417 for events, :428 for tasks, but these are defaults not enforced rules. Flag deviations only when they appear unintentional or inconsistent with the endpoint's stated purpose.
• Error strings - do errors.New strings start with a lowercase letter and not end with punctuation?
• HTTP status code on errors - errors.New and errors.Trace accept an unpaired int argument as the HTTP status code. Errors caused by user input (validation failures, missing fields, unauthorized state) should attach a 4xx code (e.g. http.StatusBadRequest); errors caused by internal failures default to 500. Flag user-caused errors that return a generic 500.
• Godoc - do all exported functions and methods in service.go and *api/ have godoc comments?
• Description consistency - the description lives once, as the godoc on the define.* var in definition.go, and genservice propagates it everywhere. Flag a handler in service.go whose godoc meaningfully contradicts its var's godoc (not just minor wording differences).
• Magic HTTP arguments - are httpRequestBody, httpResponseBody, and httpStatusCode used correctly in function signatures?
• Struct tags - do JSON struct tags on the In/Out and domain structs in *api/definition.go (and sibling type files) use omitzero (not omitempty)?
• jsonschema tags - do structs used in functional endpoint signatures have jsonschema_description:"..." tags on their fields?
• Concurrent map access - are maps that are read or written from multiple goroutines (e.g. from ticker handlers, svc.Go calls, or OnStartup vs. request handlers) protected by a mutex or replaced with sync.Map? Flag unprotected shared maps.
• Ticker concurrency - if a ticker handler performs slow operations (DB queries, downstream calls), could overlapping ticker fires cause contention or double-processing? Flag tickers without a guard (mutex, flag, or atomic) when the body's duration might exceed the interval.
• Actor context in tickers - tickers run on a service-derived context with no inherited request frame or actor. If a ticker calls a downstream endpoint that requires requiredClaims or relies on frame.Of(ctx).ParseActor/IfActor for tenant scoping, it must establish its actor context explicitly. Flag tickers that call authorized endpoints without setting up an actor.
• Goroutine leaks in svc.Go - callbacks passed to svc.Go that contain for { ... } loops or unbuffered channel reads must include a case <-ctx.Done(): arm or another ctx check, otherwise they leak across service shutdown. Flag long-lived svc.Go callbacks with no ctx-cancellation path.
• svc.Parallel opportunities - look for sequential blocks of two or more independent downstream calls (each result unused by the next call before all complete). Flag these as candidates for svc.Parallel to reduce latency.
• Context cancellation in long loops - flag loops that iterate over large or unbounded collections without checking ctx.Err(). A cancelled request should not continue doing expensive work.
• Time budget awareness - do long-running operations respect the context deadline? Flag downstream calls in a loop that never check ctx.Err(), and operations that could exceed the default 20-second time budget without declaring a sub.TimeBudget or using pub.Timeout.
• Failure isolation and graceful degradation - when a downstream call fails, is the error handled or does it propagate unchecked and take down the whole operation? Are there fallback paths for non-critical downstream failures? For multicast calls (iterating over NewMulticastClient responses), is the zero-response case handled?
• Blocking operations - are there synchronous operations in handlers that can block for extended periods without respecting cancellation (file I/O, network calls without timeout, channel operations without a select on ctx.Done())?
• Resource exhaustion - are there maps, slices, or channels on the Service struct that grow without bound? DistribCache and lru.Cache have built-in limits, but hand-rolled maps and slices do not. Flag handlers that accumulate state with no eviction or size limit.
• Ticker intervals - is each ticker's Interval appropriate for its purpose? Flag intervals that run far too frequently (sub-second) or too infrequently for the ticker's stated goal.
• Externalized strings - are user-facing strings (error messages shown to end users, UI labels) externalized to resources/text.yaml and loaded via svc.LoadResString, rather than hardcoded? Internal messages for logging or developer consumption need not be externalized.
• Distributed cache initialization - [Info] if svc.DistribCache is used, is it initialized in OnStartup with SetMaxAge and/or SetMaxMemory? Reasonable defaults exist, but explicit initialization avoids surprises under load. The cache is for data that tolerates loss and staleness; flag any use of DistribCache to share authoritative state between peers, which the framework explicitly warns against.
• Cache invalidation - when handlers mutate data that is also cached in svc.DistribCache, the corresponding cache entry must be invalidated (Delete) or overwritten (Set). Flag mutating handlers that change the canonical store without updating the cache, since subsequent reads will return stale data until the entry expires.
• Cache stampede protection - for svc.DistribCache miss paths that perform expensive work (DB query, downstream call, heavy computation), prefer GetOrCompute/LoadOrCompute over the Get+miss+compute+Set pattern. The compute methods deduplicate concurrent callers in the same process via singleflight, bounding backend load to one call per key per process instead of one per request. Flag heavy miss paths that use the read-then-write pattern.