fix: Add encoding parameter to rclone S3 config to prevent filename q…

[phernandez]

…uoting

Files with spaces like 'Hello World.md' were being displayed with single quotes as ''Hello World.md'' on the server due to rclone's default S3 encoding behavior.

This fix adds 'encoding = Slash,InvalidUtf8' to the rclone configuration, which tells rclone to only encode slashes and invalid UTF-8, not spaces or other common filename characters.

Fixes #406

[github-actions]

Claude finished @phernandez's task —— View job

---

PR #409 Code Review - Critical Bug Found

Critical Bug 🚨

File: src/basic_memory/cli/commands/cloud/rclone_config.py
Lines: 97-105

The Issue

The code has a variable name mismatch that will cause a NameError at runtime:

# Line 94: Variable is defined as REMOTE_NAME
REMOTE_NAME = "basic-memory-cloud"

# Lines 97-105: Code uses undefined variable section_name
config.set(section_name, "type", "s3")
config.set(section_name, "provider", "Other")
config.set(section_name, "access_key_id", access_key)
config.set(section_name, "secret_access_key", secret_key)
config.set(section_name, "endpoint", endpoint)
config.set(section_name, "region", region)
config.set(section_name, "encoding", "Slash,InvalidUtf8")

Root Cause

Looking at the git history:

1. Commit 7c2b8b5 created configure_rclone_remote() correctly using REMOTE_NAME
2. Commit 8af7ae0 (this PR's first commit) added the encoding fix to add_tenant_to_rclone_config()
3. The code was later refactored, but the variable name from the old function (section_name) was incorrectly used instead of REMOTE_NAME

Required Fix

Change all instances of section_name to REMOTE_NAME on lines 97-105:

config.set(REMOTE_NAME, "type", "s3")
config.set(REMOTE_NAME, "provider", "Other")
config.set(REMOTE_NAME, "access_key_id", access_key)
config.set(REMOTE_NAME, "secret_access_key", secret_key)
config.set(REMOTE_NAME, "endpoint", endpoint)
config.set(REMOTE_NAME, "region", region)
config.set(REMOTE_NAME, "encoding", "Slash,InvalidUtf8")

---

Team Checklist Review

Code Quality & Standards

• Follows Basic Memory's coding conventions - Bug prevents code from running
• Python 3.12+ type annotations and async patterns - Type annotations present but code is broken
• 100-character line length limit maintained - No violations
• No commented-out code blocks - Clean

Testing & Documentation

• Unit tests for new functions/methods - No tests found for this function
• Test coverage for edge cases - Cannot test broken code
• Documentation updated - Comments explain the encoding parameter well
• CLAUDE.md updated if conventions change - No convention changes

Basic Memory Architecture

• Preserves local-first architecture principles - No impact
• Markdown file handling preserves integrity - Bug prevents function from executing

Security & Performance

• No hardcoded secrets or credentials - Credentials passed as parameters
• Input validation for MCP tools - Not applicable (CLI command)
• Proper error handling and logging - Cannot execute due to bug
• No sensitive data in logs or commits - Clean

Encoding Fix (When Bug is Fixed)

• Correct encoding parameter - Slash,InvalidUtf8 is the right choice for Tigris/S3
• Good comments explaining why - Lines 103-104 clearly explain the purpose
• Addresses the reported issue - Will fix Beta Test Results: PR #405 - SPEC-20 Simplified Project-Scoped Rclone Sync #406 once variable name is corrected

---

Additional Findings

No Test Coverage

There are no tests for configure_rclone_remote() or rclone_config.py. This critical bug would have been caught with even basic unit tests.

Recommendation: Add tests before merging:

def test_configure_rclone_remote_creates_correct_config():
    """Test that configure_rclone_remote creates proper rclone config."""
    # This test would immediately catch the NameError

Why This Wasn't Caught

1. No linting ran - Python would catch NameError on first execution
2. No tests ran - Would fail immediately
3. Code not executed - This is a setup command, likely not run in CI

---

Summary

Status: ❌ Cannot approve - Critical bug blocks functionality

Impact:

• Severity: HIGH - Function will crash on first use
• Scope: All users running bm cloud setup will experience failure
• User Experience: Complete failure, no cloud setup possible

Required Actions:

1. ✅ Fix variable name: section_name → REMOTE_NAME (lines 97-105)
2. ⚠️ Add basic unit test for configure_rclone_remote()
3. ⚠️ Run test to verify fix works
4. ⚠️ Re-test bm cloud setup command end-to-end

Good Things About This PR:

• ✅ Encoding fix is correct (Slash,InvalidUtf8 is right for Tigris)
• ✅ Good code comments explaining the why
• ✅ Addresses real user issue (Beta Test Results: PR #405 - SPEC-20 Simplified Project-Scoped Rclone Sync #406)
• ✅ Clean code otherwise

Just needs the variable name fixed!

---