reliant-labs/forge/architecture-review

Architecture Review

Core Principles (in priority order)

1) Correctness & Safety First

• Avoid race conditions, deadlocks, non-determinism.
• Ensure multi-step DB writes are atomic (use transactions where needed).
• Validate error handling, retries, idempotency, and resource cleanup.

2) Simplicity & Elegance

• Minimize code fragmentation. Prefer strong types (use generics if necessary).
• Prefer straightforward control flow over cleverness.
• Extract repeated helper functions.
• Delete code when possible; reduce indirection.

3) Decoupling & Modularity

• Separate concerns (I/O vs domain logic vs orchestration).
• Use clear boundaries and interfaces where they improve testability.
• Avoid "god objects" and tight coupling via globals/singletons.

4) Idiomatic Implementation

• Match the repo's language conventions and patterns.
• Prefer standard library + existing internal utilities over bespoke helpers.

5) Testability by Design

• Structure code so core logic can be tested without heavy integration scaffolding.
• Push side effects to edges; make dependencies injectable.
• Define seams for mocks/fakes only where they add value.

What to Look For

Complexity & Simplification

• Can logic be simplified? Look for convoluted conditionals, unnecessary nesting, over-abstraction
• Remove duplication: DRY violations, copy-paste code that should be unified
• Unify patterns: similar code solving similar problems differently
• Dead code: unreachable branches, unused parameters, vestigial logic
• Premature abstraction: interfaces with one implementation, "framework-itis"

Naming & Clarity

• Misleading names: variables/functions that don't do what the name suggests
• Unclear responsibilities: functions doing too much, god objects
• Hidden side effects: functions that modify state unexpectedly
• Inconsistent naming: mixing conventions (camelCase vs snake_case), abbreviation inconsistency
• Magic values: unexplained numbers/strings that should be named constants

Coupling & Dependencies

• New dependencies: do they add value? are they maintained? security track record?
• Tight coupling: classes that can't be tested in isolation
• Layering violations: UI calling database directly, business logic in controllers
• Circular dependencies: modules that depend on each other
• Global state: singletons, package-level variables that make testing hard

Leaky Abstractions

• Implementation details bleeding through: callers needing to know internals to use an API correctly
• Incomplete abstractions: wrapper that doesn't handle all cases, forcing callers to bypass it
• Wrong abstraction level: interface that's too low-level (exposes mechanics) or too high-level (hides necessary control)
• Inconsistent abstraction: some methods abstract well, others expose raw internals
• Error abstraction failures: internal errors bubbling up unchanged, exposing implementation choice
• Performance characteristics leaking: callers needing to understand internal caching, batching, or connection pooling to use correctly
• Resource lifecycle leaking: abstractions that require callers to manage cleanup, ordering, or initialization that should be encapsulated

Configuration & Defaults

• Magic constants: hardcoded values that should be configurable
• Environment-dependent behavior: code that assumes specific environment
• Missing defaults: required config with no sensible default
• Missing validation: config values not validated at startup
• Feature flags: proper implementation with cleanup plan

API Contracts & Compatibility

• Backward compatibility: breaking changes to public APIs
• Wire format stability: serialization changes that break clients
• Versioning: API version strategy, deprecation notices
• Contract documentation: are API contracts clear and documented?
• Error contracts: consistent error formats, appropriate status codes

Type Safety & Idioms

• Type safety: stringly-typed code, any/interface{} overuse, missing generics
• Idiomatic code: following language conventions and best practices
• Standard library: reinventing what stdlib provides
• Existing utilities: duplicating internal helpers that already exist
• Error handling patterns: consistent with codebase conventions

Transactionality & Concurrency

• Transaction boundaries and data invariants
• Concurrency hazards (shared maps/slices, goroutines, async handlers)
• Error-handling gaps (dropped errors, partial failure states)
• Hidden coupling (shared mutable state, implicit dependencies)

Output Format (strict)

1) Summary Verdict

• Overall assessment: EXCELLENT / GOOD / NEEDS WORK / POOR
• 2–5 bullets: what's solid, what's risky

2) High-Impact Recommendations (ranked)

For each recommendation:

• Title (short)
• Why it matters (1–2 sentences)
• Proposed change (concrete steps)
• Category: (e.g., "Complexity", "Naming", "Coupling", "Safety")
• Scope: S / M / L
• Risk: Low / Med / High

3) Transactionality & Concurrency Review

• DB atomicity: OK/WARNING with specifics
• Race-condition risks: OK/WARNING with specifics

4) Pattern Consistency

• Does this code follow established patterns in the codebase?
• Any pattern violations or drift?

5) Test Plan

• Unit tests to add (table of cases if useful)
• Integration tests (only if necessary)
• Concurrency tests (if applicable)
• What to mock vs what not to mock
• Edge cases / failure modes

Constraints

• Keep suggestions proportional to the change.
• If uncertain (missing context), state assumptions explicitly and suggest the smallest verification step.
• Compare against existing patterns in the codebase.
• Prefer simple over clever.
• Flag both over-engineering AND under-engineering.
• Acknowledge when code is already well-written.