fp-go-pr-review

name

fp-go-pr-review

description

Use this skill when reviewing pull requests for fp-go code (github.com/IBM/fp-go/v2). Trigger on mentions of PR review, code review, pull request validation, fp-go best practices validation, functional programming review, or when the user asks to review changes on a PR branch. This skill validates that changes follow fp-go conventions including data-last composition, point-free style, proper monad usage, lens patterns, and idiomatic functional patterns.

fp-go PR Review

Overview

This skill assists with reviewing pull requests that use the fp-go library (github.com/IBM/fp-go/v2). It validates that code changes follow fp-go best practices and functional programming conventions. Requires Go 1.24+ for generic type alias support.

When to Use This Skill

• Reviewing pull requests with fp-go code
• Validating that changes follow fp-go best practices
• Checking for common fp-go anti-patterns
• Ensuring proper functional composition patterns
• Verifying correct monad usage and error handling

Review Checklist

1. Import Path Validation

Rule: All imports MUST use github.com/IBM/fp-go/v2/..., never github.com/IBM/fp-go/... (v1).

Check for:

// ❌ WRONG - v1 import
import "github.com/IBM/fp-go/option"

// ✅ CORRECT - v2 import
import O "github.com/IBM/fp-go/v2/option"

Severity: Critical — v1 and v2 are incompatible

2. Data-Last Principle

Rule: All fp-go operations use data-last. The data being transformed is always the last argument.

Check for:

// ❌ WRONG - data-first
option.Map(myOption, transformFunc)

// ✅ CORRECT - data-last
option.Map(transformFunc)(myOption)

// ✅ CORRECT - in pipeline
F.Pipe2(
    myOption,
    O.Map(transformFunc),
    O.GetOrElse(F.Constant("default")),
)

Severity: High — breaks composition

3. Point-Free Style

Rule: Prefer composing named functions with Flow and Pipe over inline anonymous functions.

Check for:

// ❌ AVOID - unnecessary lambda wrapping
pipeline := F.Flow2(
    func(s string) O.Option[string] { return O.FromPredicate(S.IsNonEmpty)(s) },
    func(o O.Option[string]) string { return O.GetOrElse(func() string { return "" })(o) },
)

// ✅ CORRECT - point-free composition
pipeline := F.Flow2(
    O.FromPredicate(S.IsNonEmpty),   // string -> Option[string]
    O.GetOrElse(LZ.Of("")),          // Option[string] -> string; LZ = v2/lazy
)

// ❌ AVOID - inline comparison
A.Filter(func(x int) bool { return x > 18 })

// ✅ CORRECT - use numeric combinator
A.Filter(N.MoreThan(18))

Severity: Medium — impacts readability and maintainability

4. Prefer Result Over Either

Rule: Use Result[A] (which is Either[error, A]) when the error type is Go's error. Reserve Either for custom error types.

Check for:

// ❌ AVOID - Either with error
func fetchData() E.Either[error, Data] { ... }

// ✅ CORRECT - use Result
func fetchData() R.Result[Data] { ... }

// ✅ CORRECT - Either with custom error type
func validate() E.Either[ValidationError, Data] { ... }

Severity: Medium — Result is more idiomatic for Go errors

5. IO Laziness

Rule: IO values are lazy (IO[A] is func() A). They must be called with () to execute.

Check for:

// ❌ WRONG - forgot to execute
result := readConfig("config.json")  // returns IO[Config], not Config

// ✅ CORRECT - execute with ()
result := readConfig("config.json")()

// ❌ WRONG - in ReaderIOResult, forgot inner ()
res := pipeline(ctx)         // returns func() Result[A], nothing has run yet

// ✅ CORRECT - execute both context and IO
res := pipeline(ctx)()       // Result[A] — ONE value

// ❌ WRONG - Result[A] is a single value, not a (value, error) tuple
value, err := pipeline(ctx)()

// ✅ CORRECT - unwrap to idiomatic Go at the boundary
value, err := R.Unwrap(pipeline(ctx)())

Severity: Critical — code won't execute

6. Monad Selection

Rule: Use the simplest monad that covers your needs. Escalate only when necessary.

Check for:

// ❌ AVOID - using ReaderIOResult for pure computation
// (also note: in context/readerioresult the context is baked in —
//  it is RIO.Of[A] and RIO.Map[A, B], with no environment type parameter)
func processUsers(users []User) RIO.ReaderIOResult[string] {
    return F.Pipe1(
        RIO.Of(users),
        RIO.Map(pureTransform),
    )
}

// ✅ CORRECT - pure computation, no monad needed
func processUsers() func([]User) string {
    return F.Flow2(
        A.FilterMap(toAdultName()),
        A.Intercalate(S.Monoid)(","),
    )
}

Severity: Medium — unnecessary complexity

Escalation path: Option → Result → IOResult → ReaderIOResult → Effect

7. Effect vs ReaderIOResult

Rule: Use Effect[C, A] for services with typed dependencies. Use ReaderIOResult only when you truly only need context.Context.

Check for:

// ❌ AVOID - stuffing deps into context.Context
func fetchUser(id int) RIO.ReaderIOResult[User] {
    return func(ctx context.Context) func() R.Result[User] {
        db := ctx.Value("db").(DBClient)  // runtime type assertion
        // ...
    }
}

// ✅ CORRECT - typed dependencies with Effect
type Deps struct {
    DB     DBClient
    Logger Logger
}

// Effect[Deps, User] IS func(Deps) ReaderIOResult[User] — return the closure directly.
// EF.Asks is only for pure projections func(Deps) A; feeding it a ReaderIOResult
// silently produces the nested Effect[Deps, ReaderIOResult[User]].
func fetchUser(id int) EF.Effect[Deps, User] {
    return func(deps Deps) EF.ReaderIOResult[User] {
        // deps.DB is compile-time checked
        return queryUser(deps.DB, id)
    }
}

Also flag EF.Map(f) and EF.Provide(deps)(eff) without annotations — Map[C, A, B] usually cannot infer C, and Provide[A, C] cannot infer A through the function it returns. Write EF.Map[Deps](f) and EF.Provide[string](deps).

Severity: High — type safety and testability

8. Lifting Go Functions

Rule: Use Eitherize1..EitherizeN to lift Go functions returning (T, error) into Result.

Check for:

// ❌ AVOID - manual error handling
func parseNumber(s string) R.Result[int] {
    n, err := strconv.Atoi(s)
    if err != nil {
        return R.Left[int](err)   // Left[A] — A is the success type
    }
    return R.Of(n)                // NOT R.Right[error](n); Right[A any](v A)
}

// ✅ CORRECT - use Eitherize
var parseNumber = R.Eitherize1(strconv.Atoi)

// ✅ CORRECT - in pipeline
pipeline := F.Flow2(
    R.Eitherize1(strconv.Atoi),
    R.Map(N.Mul(2)),
)

Severity: Medium — reduces boilerplate

9. Do-Notation with Lenses

Rule: Use lenses with Bind/ApS instead of manual setter functions.

Check for:

// ❌ AVOID - manual setter functions
func setUser(u User) func(State) State {
    return func(s State) State { s.User = u; return s }
}

pipeline := F.Pipe2(
    RIO.Do(State{}),
    RIO.Bind(setUser, fetchUser),
)

// ✅ CORRECT - use lens
var userLens = L.MakeLens(
    func(s State) User { return s.User },
    func(s State, u User) State { s.User = u; return s },
)

pipeline := F.Pipe2(
    RIO.Do(State{}),
    RIO.Bind(userLens.Set, fetchUser),
)

// ✅ EVEN BETTER - use code generation
//go:generate go run github.com/IBM/fp-go/v2/main lens --dir . --filename gen_lens.go

// fp-go:Lens
type State struct {
    User User
}

// Then use generated lens
lenses := MakeStateLenses()
pipeline := F.Pipe2(
    RIO.Do(State{}),
    RIO.Bind(lenses.User.Set, fetchUser),
)

Severity: Medium — maintainability and consistency

10. Bind vs ApS

Rule: Use Bind when the step depends on accumulated state; use ApS when steps are independent.

Check for:

// ❌ WRONG - using Bind when steps are independent
pipeline := F.Pipe2(
    RIO.Do(Summary{}),
    RIO.Bind(userLens.Set, func(_ Summary) RIO.ReaderIOResult[User] {
        return fetchUser(42)  // doesn't use state
    }),
    RIO.Bind(weatherLens.Set, func(_ Summary) RIO.ReaderIOResult[Weather] {
        return fetchWeather("NYC")  // doesn't use state
    }),
)

// ✅ CORRECT - use ApS for independent steps
pipeline := F.Pipe2(
    RIO.Do(Summary{}),
    RIO.ApS(userLens.Set, fetchUser(42)),
    RIO.ApS(weatherLens.Set, fetchWeather("NYC")),
)

// ✅ CORRECT - use Bind when dependent
pipeline := F.Pipe2(
    RIO.Do(Pipeline{}),
    RIO.Bind(userLens.Set, func(_ Pipeline) RIO.ReaderIOResult[User] {
        return fetchUser(42)
    }),
    RIO.Bind(configLens.Set, F.Flow2(userLens.Get, fetchConfigForUser)),
)

Severity: Medium — semantic clarity

11. TraverseArray Usage

Rule: Use TraverseArray to process slices monadically, not manual loops with error accumulation.

Check for:

// ❌ AVOID - manual loop with error handling
func fetchAll(ids []int) RIO.ReaderIOResult[[]User] {
    return func(ctx context.Context) func() R.Result[[]User] {
        return func() R.Result[[]User] {
            users := make([]User, 0, len(ids))
            for _, id := range ids {
                user, err := R.Unwrap(fetchUser(id)(ctx)())
                if err != nil {
                    return R.Left[[]User](err)
                }
                users = append(users, user)
            }
            return R.Of(users)
        }
    }
}

// ✅ CORRECT - use TraverseArray
func fetchAll(ids []int) RIO.ReaderIOResult[[]User] {
    return RIO.TraverseArray(fetchUser)(ids)
}

Severity: High — idiomatic functional pattern

12. Logging Side Effects

Rule: Use ChainFirstIOK with IO.Logf for logging without breaking the pipeline.

Check for:

// ❌ AVOID - breaking the pipeline for logging
pipeline := F.Pipe2(
    fetchUser(42),
    RIO.Chain(func(user User) RIO.ReaderIOResult[User] {
        log.Printf("Fetched user: %v", user)
        return RIO.Of(user)
    }),
)

// ✅ CORRECT - use ChainFirstIOK
pipeline := F.Pipe2(
    fetchUser(42),
    RIO.ChainFirstIOK(IO.Logf[User]("Fetched user: %v")),
)

// ✅ CORRECT - structured logging with TapSLog
pipeline := F.Pipe2(
    fetchUser(42),
    RIO.TapSLog[User]("User fetched"),
)

Severity: Low — code quality

13. Prefer Functions Over Variables

Rule: Wrap pipeline results in functions, not package-level vars.

Check for:

// ❌ WRONG - var is allocated even if never called
var processUser = F.Flow2(getName, strings.ToUpper)

// ✅ CORRECT - zero cost until called
func processUser() func(User) string {
    return F.Flow2(getName, strings.ToUpper)
}

Severity: Low — performance and dead code elimination

14. Type Parameter Order

Rule: Non-inferrable type parameters come first, so an explicit annotation only ever needs the leading prefix.

Which params are non-inferrable differs per package — check the signature rather than assuming:

// option / result / ioresult: Map[A, B](f func(A) B) — BOTH inferable from f
O.Map(toLength)              // ✅ preferred, no annotation at all
O.Map[string, int](toLength) // ✅ legal but redundant (note the order: A then B)

// Ap[B, A](fa M[A]) — B is not recoverable from fa, so it leads
O.Ap[int](fa)                // ✅

// either / reader / readerio*: the error or environment type leads
E.Map[error](f)              // ✅ either.Map[E, A, B]
RD.Map[context.Context](f)   // ✅ reader.Map[R, A, B]

// effect: C leads and is often not inferable
EF.Map[Deps](f)              // ✅
EF.Provide[string](deps)     // ✅ Provide[A, C] cannot infer A through its result

Flag an annotation that is in the wrong order (it will not compile) or one that restates what the compiler already infers.

Severity: Low — compilation errors or verbosity

15. Lens Composition

Rule: Use Compose/ComposeRef for nested struct access, not manual chaining.

Check for:

// ❌ AVOID - manual nested access
func getStreetName(p Person) string {
    if p.Address != nil && p.Address.Street != nil {
        return p.Address.Street.Name
    }
    return ""
}

// ✅ CORRECT - compose lenses
streetNameInPerson := F.Pipe2(
    personAddressLens,
    LO.Compose[Person, *Street](defaultAddress)(addressStreetLens),
    LO.ComposeOption[Person, string](defaultStreet)(streetNameLens),
)

name := streetNameInPerson.Get(person)  // Option[string]

Severity: Medium — immutability and composability

16. Immutability / No Hidden Mutation

Rule: Functions passed to Map, Chain, Filter, etc. must be pure — they must not mutate variables captured from an outer scope, and lens setters must not mutate shared slice/map fields in place.

Check for:

// ❌ WRONG - closure mutates a captured slice
var acc []string
A.Map(func(u User) User {
    acc = append(acc, u.Name)  // hidden side effect
    return u
})

// ✅ CORRECT - derive a new value, no captured mutation
names := F.Pipe1(users, A.Map(getName))

// ❌ WRONG - lens setter mutates a shared slice in place
//   append may reuse the original backing array (shallow struct copy)
func(u User, t []string) User { u.Tags = append(u.Tags, t...); return u }

// ✅ CORRECT - assign a freshly built value
func(u User, t []string) User { u.Tags = t; return u }

Severity: High — a mutating closure silently defeats fp-go's guarantees and breaks under TraverseArray/concurrency.

Review Process

Step 1: Obtain Git Diff

Get the changes on the PR branch relative to main:

git diff main...HEAD

To list only changed file paths:

git diff --name-only main...HEAD

For a GitHub PR, fetch it first:

gh pr checkout <PR-number>
git diff main...HEAD

Step 2: Analyze Changes

First, confirm the branch compiles: run go build ./... and go vet ./... on the checked-out branch. Report any build or vet failure as a Critical finding — there is no point reviewing composition style on code that does not compile, and most fp-go-specific mistakes (wrong leading type parameter, data-first vs data-last argument order, missing trailing ()) surface here.

Then, for each modified file:

1. Check import paths (v2 requirement)
2. Validate data-last usage
3. Check for point-free style opportunities
4. Verify monad selection appropriateness
5. Check IO execution (trailing ())
6. Validate error handling patterns
7. Check lens usage in do-notation
8. Verify Bind vs ApS usage
9. Look for TraverseArray opportunities
10. Check logging patterns

Step 3: Submit Findings

Post a review comment on the GitHub PR:

gh pr review <PR-number> --comment -b "$(cat <<'EOF'
## fp-go Review

**Overall**: Needs Changes

### Critical
- ❌ ...

### High
- ⚠️ ...

### Recommendations
1. ...
EOF
)"

For inline annotations on specific lines, use:

gh api repos/{owner}/{repo}/pulls/<PR-number>/comments \
  -f body="Replace inline lambda with point-free: \`F.Flow2(O.FromPredicate(S.IsNonEmpty), O.GetOrElse(LZ.Of(\"\")))\`" \
  -f commit_id="$(git rev-parse HEAD)" \
  -f path="src/user/handler.go" \
  -F line=42 \
  -f side=RIGHT

Alternatively, use the /code-review --comment skill to post inline PR annotations automatically.

Common Issue Categories

Category	Type	Example
maintainability	dry-principle-violation	Inline lambdas instead of point-free
maintainability	naming-intent-review	Non-descriptive variable names
functionality	error-handling-review	Missing error propagation
performance	inefficient-algorithm	Manual loops instead of TraverseArray
style	style-consistency-check	Inconsistent import aliases
security	sensitive-data-logging	Logging sensitive information

Severity Guidelines

• Critical: Code won't compile or execute (wrong import path, missing ())
• High: Type safety issues, incorrect monad usage, breaks composition
• Medium: Readability, maintainability, non-idiomatic patterns
• Low: Style preferences, minor optimizations

Example Review Comments

Import Path Issue

Severity: Critical Issue: Using v1 import path

The import github.com/IBM/fp-go/option is the v1 path. All imports must use v2: github.com/IBM/fp-go/v2/option

v1 and v2 are incompatible. This will cause compilation errors or runtime issues.

Point-Free Style

Severity: Medium Issue: Unnecessary lambda wrapping

This inline lambda can be replaced with point-free composition:

Current:

option.Filter(func(s string) bool { return s != "" })

Suggested:

option.Filter(S.IsNonEmpty)

Point-free style is more readable and idiomatic in fp-go.

Monad Selection

Severity: Medium Issue: Unnecessary monad for pure computation

This function uses ReaderIOResult but performs only pure transformations without IO or context:

func processUsers(users []User) RIO.ReaderIOResult[string] {
    return F.Pipe1(
        RIO.Of(users),
        RIO.Map(pureTransform),
    )
}

Suggested:

func processUsers() func([]User) string {
    return F.Flow2(
        A.FilterMap(toAdultName()),
        A.Intercalate(S.Monoid)(","),
    )
}

Use the simplest abstraction that covers your needs.

Integration with Other Skills

This skill can reference and include:

• fp-go — Core fp-go patterns and best practices
• fp-go-pipe-flow — Pipe/Flow composition patterns
• fp-go-http — HTTP request patterns
• fp-go-logging — Logging patterns
• fp-go-lens — Lens and optics patterns

Automated Checks

When reviewing, automatically check for:

1. ✅ All imports use v2 path
2. ✅ No data-first function calls
3. ✅ IO values are executed with ()
4. ✅ Result used instead of Either[error, A]
5. ✅ Point-free style where applicable
6. ✅ Appropriate monad selection
7. ✅ Lenses used in do-notation
8. ✅ Bind vs ApS used correctly
9. ✅ TraverseArray for slice processing
10. ✅ ChainFirstIOK for logging
11. ✅ No hidden mutation in Map/Chain closures or lens setters
12. ✅ Branch compiles (go build ./...) and passes go vet ./...

Output Format

Provide a summary with:

1. Overall Assessment: Pass/Needs Changes/Blocked
2. Critical Issues: Count and list
3. High Priority Issues: Count and list
4. Medium Priority Issues: Count and list
5. Low Priority Issues: Count and list
6. Positive Observations: What was done well
7. Recommendations: Suggested improvements

Example Summary

## PR Review Summary

**Overall Assessment**: Needs Changes

### Critical Issues (1)
- ❌ Using v1 import path in `user/handler.go:5`

### High Priority Issues (2)
- ⚠️ Missing IO execution in `config/loader.go:42`
- ⚠️ Manual error handling instead of Eitherize in `api/client.go:78`

### Medium Priority Issues (3)
- 💡 Inline lambda instead of point-free in `user/service.go:23`
- 💡 Using ReaderIOResult for pure computation in `utils/format.go:15`
- 💡 Manual setter instead of lens in `state/pipeline.go:56`

### Low Priority Issues (1)
- 📝 Inconsistent import alias in `handler/http.go:8`

### Positive Observations
- ✅ Excellent use of TraverseArray for parallel requests
- ✅ Proper Effect usage with typed dependencies
- ✅ Good lens composition for nested struct access

### Recommendations
1. Update all imports to v2 path
2. Add trailing `()` to execute IO values
3. Consider using `R.Eitherize1` for Go function lifting
4. Refactor pure computations to use Flow instead of ReaderIOResult

References

• fp-go v2 Documentation
• fp-go GitHub Repository
• Functional Programming in Go