Fix CI pipeline issues in pull request

[tonyalaribe]

• Add weeder job to PR workflow that checks for unused code
• Generate HIE files during build for weeder analysis
• Upload HIE files and .conf files as artifacts for weeder job
• Fix weeder.toml config: use module names instead of file paths
• Keep action versions consistent with master (v6/v5)

Closes #

How to test

Checklist

• Make sure you have described your changes and added all relevant screenshots or data.
• Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests).
• Make sure to add/update documentation regarding your changes (or request one from the team).
• You are NOT deprecating/removing a feature.

[claude]

Code Review

Summary

This PR adds weeder integration to detect unused code in the CI pipeline. Overall good addition for code quality, but there are several areas for improvement.

---

Issues & Concerns

1. CI Inefficiency - Redundant HIE File Generation

.github/workflows/pullrequest.yml:72

The build step now generates HIE files with custom GHC options, but this conflicts with the configure step:

• Line 66 configures with --ghc-options="-O0 -j8"
• Line 72 builds with additional --ghc-options="-fwrite-ide-info -hiedir=.hiefiles"

Problem: These options don't compose well. The second invocation may trigger partial rebuilds.

Fix: Move HIE generation flags to the configure step:

- name: Configure project
  run: cabal configure --enable-tests --enable-benchmarks --ghc-options="-O0 -j8 -fwrite-ide-info -hiedir=.hiefiles"

- name: Build project
  run: cabal build all -j4

This ensures consistent build artifacts and avoids potential rebuild overhead.

---

2. Artifact Upload Pattern Too Broad

.github/workflows/pullrequest.yml:80

The dist-newstyle/**/*.conf glob may upload unnecessary files. Weeder typically only needs the package .conf files from installed packages. Be more specific or verify what weeder actually requires.

---

3. Missing Error Handling

.github/workflows/pullrequest.yml:233-239

The weeder job checks for .hiefiles directory but doesn't verify its contents. Add validation to check that HIE files were actually generated.

---

4. weeder.toml Configuration

config/weeder.toml:3

Good fix changing hie_directory to hie-files. Verify weeder 2.9.0 accepts this format.

---

Performance Considerations

1. Artifact Retention: 1 day retention is appropriate
2. Build Time: HIE generation adds ~5-10% overhead (acceptable trade-off)
3. weeder Caching: Good use of cache for the weeder binary

---

Security

No security concerns.

---

Recommendations

1. HIGH: Fix GHC options in configure step (prevents rebuild)
2. MEDIUM: Verify HIE files are actually generated with file count check
3. LOW: Consider continue-on-error: true initially to establish baseline

---

Questions

1. Has this been tested with actual unused code to verify weeder catches it?
2. Does the current codebase pass weeder checks?
3. Should weeder be non-blocking initially?

---

Overall: Good addition. Main concern is GHC options handling which could cause inefficient rebuilds.

[claude]

PR Review: CI Pipeline - Weeder Integration

Overview

This PR adds weeder (unused code detection) to the CI pipeline. The implementation is solid but has several opportunities for improvement.

Critical Issue: Build Configuration Conflict

Location: .github/workflows/pullrequest.yml:71-72

GHC options are specified twice:

• Line 66: cabal configure --ghc-options="-O0 -j8"
• Line 72: cabal build --ghc-options="-fwrite-ide-info -hiedir=.hiefiles"

The second invocation overrides the first, removing -O0 -j8 optimizations.

Fix: Merge options in build step:
cabal build all -j4 --ghc-options="-O0 -j8 -fwrite-ide-info -hiedir=.hiefiles"

Code Succinctness Issues

1. Unnecessary checkout (line 207): weeder job only needs artifacts, not source. Remove checkout, include config/weeder.toml in artifacts
2. Redundant directory check (lines 236-239): weeder fails clearly without HIE files, check is unnecessary
3. Config file handling: include weeder.toml in artifacts for self-contained job

Positives

• Correct hie-files vs hie_directory fix
• Correct module name regex patterns
• Correct artifact extraction with path: .
• Consistent action versions

Summary

Must Fix: Merge GHC options
Should Fix: Remove checkout, remove HIE check, include config in artifacts
Line Reduction: -7 lines possible
Rating: 6/10 → 8.5/10 with fixes

[claude]

Code Review - PR #261

Summary

Good addition of weeder to catch unused code early. The implementation has the right idea but can be optimized for efficiency and reliability.

Issues & Improvements

1. Artifact Upload Inefficiency (.github/workflows/pullrequest.yml:78-86)

Current: Uploads HIE files, all .conf files, and weeder binary (~100s of MB potentially)
Issue: Unnecessary artifact size; weeder binary may have library dependencies that break across jobs
Fix: Only upload HIE files; install weeder fresh in the weeder job

      - name: Upload HIE files
        uses: actions/upload-artifact@v4
        with:
          name: hie-files
          path: .hiefiles
          retention-days: 1

Remove weeder installation from build job entirely.

2. Weeder Job Setup (.github/workflows/pullrequest.yml:208-228)

Current: Downloads weeder binary from artifact and runs it
Issue: Binary may not work due to missing dynamic libraries
Fix: Install weeder in this job with caching

  weeder:
    name: Check unused code (weeder)
    needs: build
    runs-on: ubuntu-24.04
    steps:
      - uses: actions/checkout@v6

      - name: Download HIE files
        uses: actions/download-artifact@v4
        with:
          name: hie-files
          path: .hiefiles

      - name: Setup Haskell
        uses: haskell-actions/setup@v2
        with:
          ghc-version: '9.12.2'

      - name: Cache weeder
        uses: actions/cache@v4
        with:
          path: ~/.local/bin/weeder
          key: weeder-2.9.0-${{ runner.os }}

      - name: Install weeder
        run: |
          if [ ! -f ~/.local/bin/weeder ]; then
            cabal install weeder-2.9.0 --install-method=copy --installdir=$HOME/.local/bin --overwrite-policy=always
          fi

      - name: Run weeder
        run: |
          [ -d ".hiefiles" ] || { echo "Error: HIE files not found"; exit 1; }
          ~/.local/bin/weeder --config config/weeder.toml

3. Action Version Consistency

Verify if upload-artifact@v4 and download-artifact@v4 match master. PR description mentions v6/v5 consistency.

4. Build Job Optimization (.github/workflows/pullrequest.yml:71-72)

Current: Builds with HIE files inline
Better: Make HIE generation configurable via cabal.project.local

Consider adding to the build job before build:

echo "write-ghc-environment-files: always" >> cabal.project.local
echo "ghc-options: -fwrite-ide-info -hiedir=.hiefiles" >> cabal.project.local
cabal build all -j4

This keeps the run command cleaner and is more maintainable.

Code Quality Summary

• ✅ Correct weeder.toml fixes (module names, not paths)
• ✅ Good use of artifact retention (1 day)
• ⚠️ Artifact size can be reduced by ~90%
• ⚠️ Binary portability issue needs addressing
• ✅ Error checking for HIE files is good

Performance Impact

• Current: ~100-200MB artifact upload/download
• Optimized: ~10-20MB artifact (HIE files only)
• Time savings: ~30-60 seconds per PR

Test Coverage

No test changes needed - this is CI infrastructure.

Security

• ✅ Pinned weeder version (2.9.0)
• Consider: Add checksum verification for weeder binary

Priority

Medium - Works but inefficient. Should optimize before merge to avoid wasted CI time/storage on every PR.