Fix cheating integration tests

[hibukki]

Tests were "cheating" by reimplementing logic instead of testing actual code:

• Created src/lib.rs to expose WebFetch as a library for testing
• Made generate_filename public for test access
• Fixed test_filename_generation_with_extension to call real WebFetch::generate_filename
• Fixed test_http_404_error to match actual error format (removed "The server returned an error response.")
• Updated snapshot for http_404_error test

Out of scope (would require full MCP server infrastructure):

• test_fetch_grugbrain, test_fetch_creates_directory, test_file_path_is_relative

[claude]

Pull Request Review: Fix cheating integration tests

Summary

This PR successfully addresses the issue of tests reimplementing logic instead of testing actual code. The refactoring creates a proper library structure that enables genuine integration testing.

Positive Changes

Code Organization ✓

• Excellent refactoring: Moved core logic from src/main.rs to src/lib.rs, following Rust best practices for library/binary separation
• Clean main.rs: Now a minimal 7-line file that just bootstraps the MCP server
• Proper visibility: Made generate_filename public, enabling test access without duplicating logic

Test Improvements ✓

• Fixed test_filename_generation_with_extension (tests/integration_test.rs:133-150): Now calls WebFetch::generate_filename instead of reimplementing hashing logic
• Fixed test_http_404_error (tests/integration_test.rs:114-129): Updated to match actual error format, removing phantom "The server returned an error response." text
• Correct snapshot update: Updated integration_test__http_404_error.snap to reflect actual implementation

Code Quality ✓

• All changes are backward compatible
• No breaking API changes
• Maintains existing error handling patterns

Areas for Consideration

Test Coverage Gaps

The PR correctly identifies tests that still need work (marked as "out of scope"):

• test_fetch_grugbrain (tests/integration_test.rs:8-65): Still reimplements fetch logic with reqwest
• test_fetch_creates_directory (tests/integration_test.rs:71-92): Manually creates directory instead of testing that fetch() does it
• test_file_path_is_relative (tests/integration_test.rs:158-179): Creates test files manually rather than calling fetch()

Recommendation: Consider filing a follow-up issue to address these remaining tests once the MCP server infrastructure for testing is available.

Minor Suggestions

1. Cargo.toml Library Configuration

Consider adding a [lib] section to explicitly define the library:

[lib]
name = "webfetch2"
path = "src/lib.rs"

This makes the library structure explicit (though Cargo will infer it correctly).

2. Documentation

The generate_filename function is now public but lacks documentation. Consider adding:

/// Generates a deterministic filename from a URL using a hash of the full URL.
/// 
/// The filename format is `{hash}.{extension}` where:
/// - `hash` is a hexadecimal hash of the full URL string
/// - `extension` is extracted from the URL path, defaulting to "html"
///
/// # Example
/// ```
/// use url::Url;
/// let url = Url::parse("https://example.com/file.json").unwrap();
/// let filename = WebFetch::generate_filename(&url);
/// assert!(filename.ends_with(".json"));
/// ```
pub fn generate_filename(url: &url::Url) -> String {

3. Test Assertions

In test_filename_generation_with_extension:149, the assertion message is excellent and descriptive. This is good practice throughout.

Security Review ✓

• No security concerns introduced
• URL parsing and validation remain unchanged
• File path handling is safe (uses PathBuf)
• No exposure of sensitive functionality

Performance Review ✓

• No performance impact
• Code movement doesn't change execution paths
• No additional allocations or computations

Potential Issues

None Critical

No bugs or critical issues identified. The changes are clean and focused.

Testing Recommendations

To verify the changes work correctly:

# Run tests to ensure everything passes
cargo test

# Check that the binary still works
cargo build --release
./target/release/webfetch2 --help

# Verify library can be imported (create a simple test crate)
cargo new --bin test_import
# Add webfetch2 = { path = "../webfetch2" } to Cargo.toml
# Try importing: use webfetch2::WebFetch;

Conclusion

Recommendation: Approve with minor suggestions

This is a well-executed refactoring that addresses the core issue described in the PR. The separation of library and binary concerns is a Rust best practice, and the test fixes ensure they're testing actual implementation code. The remaining test improvements are correctly scoped out and don't diminish the value of this PR.

The code quality is solid, follows Rust conventions, and maintains backward compatibility. Great work on improving test integrity!

---

Review performed by Claude Code

[hibukki]

@claude

1. one of the tests fail
2. adding an explicit "[lib]" sounds good
3. Make sure that the tests that were not fixed still have comments explaining their problems. (ok that they are out of scope, but we want this still marked to fix later)

[claude]

Claude finished @hibukki's task —— View job

---

Tasks Completed ✓

What I Did:

1. Fixed the failing test (test_file_path_is_relative)

   • Changed to use PathBuf::from() and .join() for consistent path handling
   • Added verification that directory exists before attempting to write
   • Enhanced TODO comment to explain why this test is "cheating" and note it's out of scope

2. Added explicit [lib] section to Cargo.toml

   • Added proper library configuration as requested:

     [lib]
     name = "webfetch2"
     path = "src/lib.rs"

3. Enhanced TODO comments on out-of-scope tests

   • All "cheating" tests now have comprehensive comments explaining:
     • What they're doing wrong (reimplementing logic instead of calling actual functions)
     • Why they need to be fixed (changes to real implementation won't be caught)
     • That they're "Out of scope for now - requires full MCP server infrastructure"
   • Updated line number references to match current code
   • Tests marked with enhanced comments:
     • test_fetch_grugbrain (line 3-7)
     • test_fetch_creates_directory (line 67-70)
     • test_invalid_url_format (line 96-100)
     • test_file_path_is_relative (line 153-157)

Changes committed and pushed to branch claude/fix-cheating-tests-011CV2xha7GQD8MQcWY8L6Gh

The CI should now pass with the fixed test, and future developers will clearly understand which tests still need proper implementation.

--- | Branch: claude/fix-cheating-tests-011CV2xha7GQD8MQcWY8L6Gh

[claude]

Pull Request Review: Fix cheating integration tests

Summary

This PR addresses a significant testing anti-pattern where tests were reimplementing production logic instead of calling the actual code under test. The refactoring extracts the core logic into a library crate (src/lib.rs) to make it accessible for proper testing.

---

Positives

1. Excellent architectural improvement: Separating the binary from the library is the correct approach for testability. This follows Rust best practices.

2. Honest TODO comments: The comments acknowledging limitations (tests still not calling MCP server) show good engineering transparency.

3. Concrete test improvement: test_filename_generation_with_extension now correctly calls WebFetch::generate_filename instead of duplicating its logic.

4. Fixed snapshot test: The test_http_404_error snapshot was updated to match the actual error format, catching the discrepancy.

5. Better PathBuf usage: test_file_path_is_relative now uses PathBuf consistently, which is more idiomatic Rust.

---

Code Quality and Best Practices

Minor: edition = 2024 is experimental

Cargo.toml:4 sets edition = "2024" but Rust 2024 edition doesn't exist yet (latest stable is 2021). This will cause compilation errors on stable Rust. Should be edition = "2021".

Visibility: generate_filename is now public

src/lib.rs:116 makes generate_filename public. This is fine for testing, but consider whether this should be part of your public API. If it's only for tests, document that in a comment.

---

Potential Bugs

Critical: Hash instability for filenames (src/lib.rs:117-122)

DefaultHasher's output is explicitly not stable across Rust versions or executions. This means:

• The same URL may generate different filenames on different runs
• Files won't be reused/found if the server restarts
• Tests may become flaky

Recommendation: Use a stable hash algorithm like sha256, or sanitize the URL into a valid filename.

Minor: Unsafe file extension parsing (src/lib.rs:125-135)

Edge cases not handled:

• https://example.com/file.tar.gz extracts ".gz" instead of ".tar.gz"
• https://example.com/.htaccess extracts "htaccess" (dot file)
• https://example.com/file.backup.old extracts ".old"

---

Performance Considerations

URL cloning: In fetch at line 64, url.clone() is passed to reqwest::get. Since you've already parsed it to parsed_url, consider using reqwest::get(parsed_url.as_str()).await to avoid cloning.

---

Security Concerns

Path traversal risk (Low severity): src/lib.rs:60-61

Currently safe because generate_filename only returns a hash + extension, but if the implementation changes to include URL path segments, this could allow path traversal. Add validation to ensure filename contains no path separators.

No size limit on downloads: src/lib.rs:94

The code reads entire responses into memory (response.bytes().await) without checking Content-Length. A malicious server could cause OOM with a multi-GB response. Recommend adding a MAX_SIZE check (e.g., 100MB limit).

---

Test Coverage

Improved but still limited

Now properly tested:

• generate_filename logic (src/lib.rs:116)
• Error message formatting (snapshots updated)

Still cheating (acknowledged in TODOs):

• test_fetch_grugbrain: Uses reqwest directly
• test_fetch_creates_directory: Manually creates directory
• test_invalid_url_format: Calls url::Url::parse directly
• test_file_path_is_relative: Creates test files manually

Missing test coverage:

1. Error paths in fetch (timeout, connection, file write errors)
2. Edge cases (empty response, redirects, non-UTF8 content)
3. No end-to-end MCP protocol test

Recommendation: Consider adding unit tests for fetch using mockito or wiremock for HTTP mocking.

---

Summary and Recommendations

Priority Fixes:

1. Critical: Change edition = "2024" to "2021" (compilation blocker)
2. High: Replace DefaultHasher with stable hash (correctness issue)
3. Medium: Add file size limit (security + stability)

Nice-to-haves:
4. Add HTTP mocking tests for fetch error paths
5. Validate generated filenames for path separators
6. Handle multi-extension files better

Verdict: This PR is a significant step forward in test quality. The architectural refactoring is sound. After fixing the edition and hash stability issues, this should be good to merge. The remaining test limitations are well-documented and can be addressed in future work.