Convert file names to URL-safe format

[hibukki]

• Convert URLs to filesystem-safe filenames by replacing special characters
• Preserve file extensions from URLs, add .html if no extension present
• Add override notification when downloading to existing file
• Update all tests and snapshots to match new filename format

This makes downloaded files more discoverable and readable compared to the previous hash-based naming scheme.

[claude]

PR Review: Convert file names to URL-safe format

Summary

This PR changes the filename generation from hash-based to URL-based naming, making downloaded files more human-readable. Overall, this is a valuable improvement that enhances usability.

Code Quality & Best Practices

✅ Strengths

• Clear improvement in UX: URL-based filenames are much more discoverable than hashes
• Good defensive programming: The 240-character length limit prevents filesystem issues
• Comprehensive test updates: All tests and snapshots updated to match new behavior
• Helpful user feedback: The "overriding existing file" message is a nice touch

⚠️ Issues & Concerns

1. Filename Collision Risk (High Priority)

The new approach creates potential for silent data overwrites:

Different URLs can map to the same filename:

• https://example.com/path/to/file → https_example.com_path_to_file.html
• https://example.com/path_to_file → https_example.com_path_to_file.html ⚠️ COLLISION

Recommendation: Consider adding a short hash suffix to guarantee uniqueness while maintaining readability, like https_example.com_path_to_file-a3f2.html

2. Extension Detection Logic (Medium Priority)

The extension detection at src/main.rs:131-135 only checks if the last path segment contains a dot.

Problems:

• https://example.com/file. → Has extension (empty extension)
• https://example.com/.hidden → Has extension (dot file, no extension)

Recommendation: More robust extension detection that checks for non-empty extensions after the last dot.

3. Length Truncation Could Break Extensions (Low Priority)

At src/main.rs:146-149, truncation happens on byte boundaries, not character boundaries, which could panic on multi-byte UTF-8 characters.

Recommendation: Use is_char_boundary() to ensure safe UTF-8 truncation.

4. Missing URL Normalization (Medium Priority)

The code uses url.as_str() directly without normalization:

• https://Example.com/Path and https://example.com/path create different files
• Trailing slashes matter: https://example.com/ vs https://example.com

Recommendation: Normalize URLs before generating filenames (lowercase domain, remove trailing slash, etc.)

Performance Considerations

✅ No concerns

• Replacing hash computation with string replacement is actually faster
• No algorithmic complexity issues

Security Concerns

⚠️ Minor Issues

1. Character Replacement Might Not Be Exhaustive

• Null bytes could cause issues on some systems
• Newlines/control characters in URLs are not handled
• Unicode characters that are invalid in filenames on some systems

2. Path Traversal Prevention

The code replaces / and \\, which is good. However, the resulting filename could still contain patterns like .. if the URL has consecutive dots.

Recommendation: Add safe_name = safe_name.replace("..", "_");

Test Coverage

⚠️ Critical Issue: Tests Do Not Actually Test the Code

The test file has excellent TODO comments that correctly identify a fundamental problem:

All tests reimplement the logic instead of calling WebFetch::fetch()

This means:

• Tests verify test logic, not production code
• Changes to error messages will not be caught
• No actual integration testing of the MCP server

Example: test_http_404_error (lines 141-146) uses error text "The server returned an error response." which does not exist in main.rs:91-94.

Missing Test Cases

1. Filename collisions (different URLs → same filename)
2. Very long URLs (>240 chars)
3. Unicode in URLs
4. File override behavior (actual behavior, not just the exists flag)
5. Edge case extensions (., .., .hidden)

Recommendations Summary

Must Fix (Before Merge)

1. Add collision prevention: Use hash suffix or implement collision detection
2. Fix test architecture: Tests should call actual WebFetch::fetch() function, not reimplement logic

Should Fix

3. Improve extension detection: Handle edge cases like dot files and trailing dots
4. Add UTF-8 safety: Ensure truncation does not split multi-byte characters
5. Normalize URLs: Prevent case/trailing-slash creating duplicate files

Nice to Have

6. Enhanced sanitization: Handle control characters and null bytes
7. Add missing test cases: Collisions, long URLs, Unicode, override behavior

Verdict

This is a valuable UX improvement, but has reliability issues that should be addressed:

• The collision risk could cause data loss in real usage
• The test suite does not actually test the production code

I recommend addressing the collision prevention and test architecture issues before merging.

---

Review generated with assistance from Claude Code

[hibukki]

@claude main has changed,

please check what changed and either merge or maybe it would be easier to re implement this over the latest code

[claude]

Claude finished @hibukki's task —— View job

---

Re-implementing URL-based filenames on latest main

• Fetch latest changes from origin
• Compare current PR branch with origin/main
• Analyze what changed in main
• Decide: merge or re-implement
• Create src/lib.rs with URL-based filename generation
• Update src/main.rs to use lib
• Add .claude/settings.json and tests/CLAUDE.md from main
• Update Cargo.toml with [lib] section
• Update tests to call actual WebFetch::fetch()
• Update test snapshots to match new error messages
• Run tests to verify changes (waiting for cargo test approval)
• Commit and push changes

Changes Applied

I've successfully re-implemented the URL-based filename feature on top of main's refactored code structure:

Files created:

• src/lib.rs - Contains WebFetch implementation with URL-based filename generation
• .claude/settings.json - Claude Code permissions configuration
• tests/CLAUDE.md - Test documentation

Files modified:

• src/main.rs - Now uses the lib (just contains main() function)
• Cargo.toml - Added [lib] section
• tests/integration_test.rs - Updated to call actual WebFetch::fetch() instead of reimplementing logic
• tests/snapshots/integration_test__http_404_error.snap - Updated error message to match lib implementation

Key changes to URL-based filename generation:

• Converts URLs to filesystem-safe names (e.g., https://grugbrain.dev/ → https_grugbrain.dev.html)
• Preserves file extensions from URLs
• Adds .html extension if URL has no extension
• Limits filename length to 240 characters
• Shows override notification when downloading to existing file

Now waiting for cargo test approval to verify all tests pass...

--- | Branch: claude/filename-by-url-conversion-011CV2wHKV9gRiGZ45sdEAsr