feat: optimize builds for CI and reduce disk usage

[AlexMikhalev]

Build Optimization (Phase 1):

• Add [profile.ci] with strip=true for faster builds
• Add [profile.ci-release] with strip=symbols for smaller binaries
• Add sccache cache layer to ci-main.yml
• Create weekly cleanup workflow (.github/workflows/cleanup-target.yml)
• Create local cleanup script (scripts/cleanup-build.sh)
• Document build optimization strategies in CLAUDE.md

Expected savings: 95-150 GB (addresses 200+ GB problem)

[claude]

Code Review: Build Optimization PR

Thank you for working on build optimization! However, there are several critical issues that must be fixed before merge.

Critical Issues

1. Compilation Errors

• Line 442: Typo e.to().to_string() should be e.to_string()
• Lines 366-368: Type mismatches - blocks don't return Result

2. Breaking Change

• Removes read_url() without proper feature flag
• Breaks AutomataPath::Remote functionality
• No migration guide provided

3. CI Profiles Not Used

• New profiles defined but CI still uses --release
• PR doesn't achieve optimization goal

4. Cleanup Workflow Issue

• Runs on GitHub-hosted runners where it has no effect
• Should use self-hosted or be removed

5. Code Duplication

• Error handling logic duplicated 4 times
• Extract to helper function

Required Actions

Must fix before merge:

1. Fix compilation errors
2. Restore read_url() or document breaking change
3. Update CI to use --profile ci-release
4. Fix cleanup workflow
5. Extract duplicate code

Testing

Run: cargo build && cargo test && cargo clippy

See diagnostics showing compilation errors at lib.rs:442 and lib.rs:366-368

[claude]

Code Review for PR #487: Build Optimization

Summary

This PR addresses an important problem (200+ GB disk usage) with targeted optimizations. The approach is generally sound, but there are several critical issues that need to be addressed before merging.

---

Critical Issues

1. Compilation Error in terraphim_service/src/lib.rs:442

Location: crates/terraphim_service/src/lib.rs:442

let is_file_not_found = e.to().to_string().contains("file not found");

Issue: Typo - e.to().to_string() should be e.to_string(). This will not compile.

Fix:

let is_file_not_found = e.to_string().contains("file not found");

2. Unsafe Cleanup Script Patterns

Location: scripts/cleanup-build.sh

Issues:

• Uses eval on line 62, which is a security risk
• Uses find ... -delete and find ... -exec rm -rf {} + without confirmation
• No input validation on paths being deleted

Recommendations:

• Remove eval and execute commands directly
• Add a confirmation prompt for non-dry-run mode
• Add safety checks to ensure we're in the correct directory
• Use more specific patterns to avoid accidental deletions

Example improvement:

# Instead of eval, use direct execution
if [ "$DRY_RUN" = false ]; then
    find target/release -name '*.rlib' -mtime +7 -delete 2>/dev/null || true
fi

3. Ineffective Cleanup Workflow

Location: .github/workflows/cleanup-target.yml

Issue: This workflow runs cargo clean on a GitHub Actions runner, but:

• Runners are ephemeral - each run starts fresh
• The cleanup doesn't affect persistent storage
• No disk space is actually saved by this workflow

Recommendation: Either remove this workflow or repurpose it to clean up actual persistent artifacts (like caching directories, old releases, etc.).

---

Code Quality Issues

4. Fragile Error Detection

Location: Multiple places in terraphim_service/src/lib.rs

Issue: Checking for "file not found" by string matching is fragile:

let is_file_not_found = e.to_string().contains("file not found")
    || e.to_string().contains("not found:");

Better approach: Check the error type/kind directly:

let is_file_not_found = matches!(
    e.downcast_ref::<std::io::Error>(),
    Some(io_err) if io_err.kind() == std::io::ErrorKind::NotFound
);

5. Breaking Change Without Documentation

Location: crates/terraphim_automata/src/lib.rs:359

Issue: Remote loading is now completely disabled unless the remote-loading feature is enabled. This is a behavioral change that should be:

1. Documented in the PR description
2. Considered for semantic versioning impact
3. Potentially gated differently (e.g., return the original behavior if feature is enabled, otherwise error)

---

Performance Considerations

6. CI Profiles Look Good

Positive: The new CI profiles in Cargo.toml are well-designed:

• [profile.ci] with strip = true for faster builds
• [profile.ci-release] with strip = "symbols" balances size and debuggability
• Appropriate incremental compilation settings

Suggestion: Add comments in Cargo.toml explaining when to use each profile:

# Use for fast CI checks: cargo build --profile ci
[profile.ci]
# ...

# Use for smaller CI release binaries: cargo build --profile ci-release  
[profile.ci-release]

---

Security Concerns

7. Script Security

• The cleanup script uses eval which can execute arbitrary code
• No validation of the working directory before deletion operations
• Missing safety bounds on find operations

---

Test Coverage

8. Missing Tests

Missing test coverage for:

• Error handling changes in terraphim_service (logging level changes)
• Error message format in terraphim_automata (remote loading disabled)
• Cleanup script functionality

Recommendations:

• Add unit tests for the new error detection logic
• Add integration tests verifying the error messages
• Add shell script tests for the cleanup script (using dry-run mode)

---

Suggestions for Improvement

1. Immediate fixes needed:

   • Fix the typo at terraphim_service/src/lib.rs:442
   • Remove or fix the ineffective cleanup workflow
   • Secure the cleanup script (remove eval, add confirmations)

2. Code quality improvements:

   • Use error type checking instead of string matching
   • Document the breaking change for remote loading
   • Add tests for error handling logic

3. Documentation:

   • Add comments explaining CI profile usage
   • Document when to run the cleanup script
   • Update PR description to mention the remote loading change

---

Positive Points

• Addresses a real problem (disk usage)
• Good use of Cargo profiles for CI optimization
• Thoughtful error logging improvements (downgrading noise)
• Well-structured cleanup script with dry-run support

---

Overall Recommendation

Request Changes - The typo at line 442 is a blocker (compilation error). The other issues should be addressed before merge for code quality and security reasons.

Once the critical issues are fixed, this will be a solid optimization PR.

[AlexMikhalev]

Closing this PR as its changes have already been incorporated into main via PR #497 (Integration/merge all).

Merged changes include:

• [profile.ci] and [profile.ci-release] build profiles in Cargo.toml
• .github/workflows/cleanup-target.yml - weekly cleanup workflow
• scripts/cleanup-build.sh - local cleanup script
• Logging improvements for optional file handling in terraphim_automata and terraphim_service

The PR branch was 112 commits behind main, causing merge conflicts. All intended functionality is now live.