reliant-labs/forge/code-hygiene-review

Code Hygiene Review

Focus Areas

Correctness

Off-by-One & Boundary Cases

• Empty inputs: does code handle empty arrays, strings, maps?
• Single element: edge case often broken
• Last index: fencepost errors, inclusive vs exclusive ranges
• Boundary values: max int, zero, negative numbers
• Unicode: multi-byte characters, string length vs byte length

Nil/Null Handling

• Unexpected nil pointers: dereferencing without nil checks
• Optional fields: assuming presence without verification
• Absent map keys: reading from maps without checking existence
• Nil slices vs empty slices: behavioral differences
• Chained access: a.b.c where any could be nil

Error Handling

• Ignored errors: _ = mayFail() or no error check
• Swallowed exceptions: catch-all that logs and continues
• Error wrapping loses context: wrapped errors missing original cause
• Retries without bounds: infinite retry loops
• Partial cleanup on error: resources not released in error path

Invariants & State

• Data structure invariants: does the change preserve them?
• State machine violations: invalid state transitions
• Precondition/postcondition: are assumptions documented and checked?
• Defensive copying: mutable state shared unexpectedly

Idempotency

• Repeated calls safe?: handlers, jobs, retries
• Side effects on retry: duplicate emails, double charges
• Idempotency keys: are they used where needed?

Time & Clock Issues

• Timezones: assuming local time, missing UTC conversion
• Monotonic vs wall clock: for measuring duration vs display
• Truncation/rounding: time precision issues
• "now" inside loops: time.Now() called repeatedly when it should be captured once
• Daylight saving: edge cases around DST transitions

Tests & Observability

Test Coverage

• New branches covered?: if/else, switch cases, error paths
• Failure paths tested?: what happens when things go wrong
• Edge cases: empty, nil, boundary values
• Concurrency cases: race conditions have test coverage?

Test Quality

• Fake tests: tests that don't actually test production code
• Meaningless assertions: assert(true), testing mocks
• Flaky tests: sleeps, timing assumptions, shared global state
• Test isolation: tests affecting each other's state
• Mocking too much: testing the mocks, not the code

Logging & Observability

• Actionable logs: can you debug with these logs?
• Log spam: excessive logging, logging in hot paths
• Useful fields: request ID, user ID, operation context
• Missing error logs: silent failures
• Debug logging left in: temporary logging not removed

Debuggability

• Error context: errors include enough info to debug
• "Unknown error occurred": useless error messages
• Stack traces: preserved where useful, hidden from users
• Correlation IDs: can you trace a request across services?

LLM Code Antipatterns

Fallbacks That Mask Bugs

• Swallowed errors: catch { return default }
• Silent defaults: defaulting instead of failing on bad input
• "Log and continue": on critical paths where failure should propagate
• Retries hiding root cause: masking persistent failures

Stringly-Typed Code

• Accessing fields by string: obj["fieldName"] instead of typed access
• Magic strings: repeated string literals that should be constants
• String parsing: parsing strings that should be structured types
• Type assertions without checks: unsafe casts

Assumptions Without Verification

• Files exist: assuming paths are valid without checking
• Services running: assuming dependencies are available
• Migrations applied: assuming schema is up to date
• Env vars set: using env vars without validation

Code Fragmentation

• Unnecessary helper sprawl: tiny functions that obscure rather than clarify
• Wrapper over wrapper: layers of indirection
• Parallel codepaths: similar code that should be unified
• Abstraction without simplification: more indirection, same complexity

Migration & Rollout

• Schema changes safe?: backwards compatible, nullable new columns
• Backfills needed?: data migration for existing records
• Feature flags: gradual rollout, kill switch
• Roll-forward plan: if this ships broken, can a follow-up release fix it from the state it leaves behind (no down migrations, no rollback)?
• Config changes documented: new config values, changed defaults

Output Format

Hygiene Review Summary

• Cleanliness rating: CLEAN / MOSTLY CLEAN / NEEDS CLEANUP / MESSY
• Top concerns (1-3 bullets)

Findings (ordered by category)

For each finding:

• Category: (e.g., "Error Handling", "Test Quality", "LLM Antipattern")
• Severity: HIGH / MEDIUM / LOW / NIT
• Location: file:line or function
• Issue: What's wrong
• Fix: Specific recommendation

Correctness Checklist

• [ ] Nil/null handling: OK / CONCERNS
• [ ] Error handling: OK / CONCERNS
• [ ] Boundary cases: OK / CONCERNS
• [ ] Idempotency: OK / CONCERNS / N/A

Test Assessment

• Coverage of new code: GOOD / PARTIAL / MISSING
• Test quality: GOOD / CONCERNS
• Specific test gaps to address

Cleanup Items

• Debug logging to remove
• Dead code to delete
• TODOs to address or ticket

Review Guidelines

• Be thorough about error handling
• Treat "it probably won't happen" as "it will happen in production"
• Check for LLM-generated code smells (verbose, over-defensive, stringly-typed)
• Verify tests actually test the production code, not mocks
• Flag anything that would make debugging production issues harder
• Keep suggestions proportional to the change. For trivial changes (CSS, copy, config), focus only on obvious issues in the changed code. Do not review surrounding unchanged code.