Skip to content

feat: consolidate config files into ~/.limacharlie.d/ directory#257

Merged
tomaz-lc merged 7 commits intocli-v2from
feat/config-directory-consolidation
Mar 20, 2026
Merged

feat: consolidate config files into ~/.limacharlie.d/ directory#257
tomaz-lc merged 7 commits intocli-v2from
feat/config-directory-consolidation

Conversation

@tomaz-lc
Copy link
Copy Markdown
Contributor

@tomaz-lc tomaz-lc commented Mar 18, 2026

Details

Consolidates all CLI config files (credentials, JWT cache, search checkpoints) under a single directory with platform-appropriate defaults. With CLI v2 (5.x) being a major release, this is the right time to establish consistent file layout conventions and follow platform best practices - rather than shipping with scattered dotfiles (~/.limacharlie, ~/.limacharlie_jwt_cache, ~/.limacharlie.d/) and accumulating tech debt that would be harder to change post-release.

The new layout unifies everything under ~/.limacharlie.d/ on Unix and %APPDATA%/limacharlie/ on Windows, with a single paths.py module as the source of truth for all path resolution. Full backward compatibility is preserved - existing ~/.limacharlie configs are auto-detected with a UserWarning, and users can migrate via limacharlie config migrate.

Migration is intentionally not automatic (consistent with Docker, AWS CLI, Terraform conventions). The CLI reads from the legacy location and emits a warning; users control when migration happens.

Changes

New files

  • limacharlie/paths.py - Centralized path resolution module. Single source of truth for config file, JWT cache, and checkpoint directory paths. Handles platform detection (Unix vs Windows %APPDATA%), env var overrides (LC_CONFIG_DIR, LC_CREDS_FILE, LC_LEGACY_CONFIG, LC_NO_MIGRATION_WARNING), new-then-legacy fallback with warnings, and per-process caching.
  • limacharlie/commands/config_cmd.py - New config CLI command group:
    • config show-paths - Display all resolved paths, existence flags, active env var overrides
    • config migrate - Copy files from legacy to new layout (--dry-run, --remove-old, --force). Content verification before legacy file removal prevents data loss when destination has different content.

Modified files

  • config.py - Delegates path resolution to paths.py, ensures parent directory exists on write
  • jwt_cache.py - Delegates cache path to paths.py
  • search_checkpoint.py - Delegates checkpoint dir to paths.py
  • constants.py - Removed duplicate CONFIG_FILE_PATH
  • commands/auth.py, help_topics.py - Updated path references in help text
  • doc/authentication.md - Added config migration guide, JWT caching docs
  • README.md - Added documentation section with links
  • pyproject.toml - Added pytest config: testpaths includes microbenchmarks with --benchmark-disable
  • cloudbuild_pr.yaml - Unit test steps now include microbenchmarks as correctness tests; dedicated benchmark step uses -o 'addopts=' to override
  • .github/workflows/publish-to-pypi.yml - Pre-publish tests now include microbenchmarks
  • 8 test fixture files - Updated from monkeypatching CONFIG_FILE_PATH to using LC_CONFIG_DIR env var + _reset_path_cache()

New test files

  • tests/unit/test_paths.py (62 tests) - Path resolution correctness, env var priority, caching, security (path traversal, symlinks, null bytes), edge cases (unicode, spaces, long paths), warning type verification (UserWarning not DeprecationWarning), LC_NO_MIGRATION_WARNING strict =="1" semantics
  • tests/unit/test_config_cmd.py (32 tests) - Migrate correctness, dry-run, force, content verification before removal, security (symlink rejection, secure permissions), robustness (unwritable dirs, empty/large/unicode files, partial failure, content mismatch exit codes)
  • tests/integration/test_config_directory.py (29 tests) - End-to-end scenarios: fresh install, legacy-to-migration lifecycle, LC_CONFIG_DIR override, LC_LEGACY_CONFIG mode, LC_CREDS_FILE override, cross-subsystem consistency, permissions, concurrent-like access patterns
  • tests/microbenchmarks/test_paths_microbenchmark.py (29 benchmarks) - Overhead measurement for all path resolution and config loading operations

Path resolution behavior

Scenario Config file JWT cache Checkpoints
Fresh install ~/.limacharlie.d/config.yaml ~/.limacharlie.d/jwt_cache.json ~/.limacharlie.d/search_checkpoints/
Legacy files exist ~/.limacharlie (+ warning) ~/.limacharlie_jwt_cache ~/.limacharlie.d/search_checkpoints/
After config migrate ~/.limacharlie.d/config.yaml ~/.limacharlie.d/jwt_cache.json ~/.limacharlie.d/search_checkpoints/
LC_LEGACY_CONFIG=1 ~/.limacharlie (no warning) ~/.limacharlie_jwt_cache ~/.limacharlie.d/search_checkpoints/
LC_CONFIG_DIR=/foo /foo/config.yaml /foo/jwt_cache.json /foo/search_checkpoints/
LC_CREDS_FILE=/foo/bar /foo/bar /foo/bar_jwt_cache /foo/bar.d/search_checkpoints/
Windows (fresh) %APPDATA%/limacharlie/config.yaml %APPDATA%/limacharlie/jwt_cache.json %APPDATA%/limacharlie/search_checkpoints/

New environment variables

Env var Purpose
LC_CONFIG_DIR Override config directory (all files go here)
LC_CREDS_FILE Override config file path directly (existing, highest priority)
LC_LEGACY_CONFIG=1 Force legacy flat-file layout, no warnings
LC_NO_MIGRATION_WARNING=1 Suppress warning only (keeps new layout resolution)
LC_EPHEMERAL_CREDS Disable all disk persistence (existing, unchanged)

Blast radius / isolation

  • Affected: config.py, jwt_cache.py, search_checkpoint.py path resolution (all now delegate to paths.py). Help text in auth.py, help_topics.py. CI config (cloudbuild_pr.yaml, publish-to-pypi.yml).
  • NOT affected: API calls, credential resolution logic, JWT generation/caching logic, search execution, all SDK classes. Only the where files live on disk changed, not what they contain or how they're used.
  • All existing LC_CREDS_FILE and LC_EPHEMERAL_CREDS behavior is preserved unchanged.

Performance characteristics

Path resolution uses per-process caching. Benchmark data from test_paths_microbenchmark.py:

Operation Latency Notes
get_config_path() cached ~66ns Dict lookup, no I/O. Every CLI command pays this.
get_jwt_cache_path() cached ~57ns Dict lookup, no I/O
get_all_paths() cached ~241ns Four cached lookups
get_config_path() cold ~3.6us First call per process: env var + stat
get_all_paths() cold ~9us First call: four cold resolutions
Legacy fallback cold ~13us Worst case: new miss + legacy hit + warning check
load_config() cached ~57ns In-memory dict return
load_config() cold (small) ~170us YAML parse dominates
load_config() cold (20 envs) ~3.8ms YAML parse scales with size
Full CLI startup cold ~156us Path resolution + config load + credential resolve
Full CLI startup warm ~4.3us All cached
save_config() small ~167us YAML dump + atomic write
Migration per file ~80-88us Read + atomic write + verify

Summary: Per-process caching makes the overhead negligible. After the first call (~156us cold startup), subsequent path/config lookups are ~57-66ns (pure dict return). No regression vs the previous direct CONFIG_FILE_PATH constant lookup.

Notable contracts / APIs

  • New CLI commands: config show-paths, config migrate
  • New env vars: LC_CONFIG_DIR, LC_LEGACY_CONFIG, LC_NO_MIGRATION_WARNING
  • Config file format unchanged (same YAML structure, just different path)
  • JWT cache format unchanged (same JSON structure)
  • Warning uses UserWarning (not DeprecationWarning) so it's visible by default for installed packages. SDK consumers can filter via standard warnings.filterwarnings().
  • config migrate --remove-old exits with code 3 when legacy files have different content than destination (requires manual review). Exit code 0 only when all operations succeed.

Test plan

  • 2017 existing + new unit tests pass (no regressions)
  • 62 unit tests for paths.py (correctness, priority, security, edge cases, warning semantics)
  • 32 unit tests for config_cmd.py (migrate, show-paths, security, robustness, exit codes)
  • 29 integration tests for full lifecycle scenarios
  • 29 microbenchmarks for overhead measurement
  • Microbenchmarks run as correctness tests in CI (--benchmark-disable)
  • Dedicated benchmark step in Cloud Build for timing data
  • Manual verification: config show-paths, config migrate --dry-run, warning emission, LC_LEGACY_CONFIG=1 / LC_NO_MIGRATION_WARNING=1 suppression
  • Test on macOS
  • Test on Windows

🤖 Generated with Claude Code

Unify all CLI config files (credentials, JWT cache, search checkpoints)
under a single directory with platform-appropriate defaults. CLI v2
(5.x) is the right time for this - establishing consistent conventions
before the public release rather than accumulating tech debt.

New layout:
- Unix: ~/.limacharlie.d/config.yaml, jwt_cache.json, search_checkpoints/
- Windows: %APPDATA%/limacharlie/config.yaml, jwt_cache.json, search_checkpoints/

Backward compatible: reads from legacy ~/.limacharlie with deprecation
warning. New env vars LC_CONFIG_DIR, LC_LEGACY_CONFIG. Existing
LC_CREDS_FILE continues to work. Migration via `limacharlie config migrate`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tomaz-lc tomaz-lc force-pushed the feat/config-directory-consolidation branch 11 times, most recently from 442b49f to 1f8c13f Compare March 19, 2026 07:51
@tomaz-lc tomaz-lc requested a review from maximelb March 19, 2026 11:53
…head

Benchmarks cover cached (hot) and uncached (cold) path resolution,
config loading at various sizes, credential resolution, config writing,
migration overhead, and simulated CLI startup cost.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tomaz-lc tomaz-lc force-pushed the feat/config-directory-consolidation branch from 1f8c13f to 162c381 Compare March 19, 2026 11:54
@tomaz-lc tomaz-lc marked this pull request as ready for review March 19, 2026 11:55
@maximelb
Copy link
Copy Markdown
Contributor

@tomaz-lc Code review for this PR:

Bug: Priority Inconsistency Between get_config_dir() and get_config_path()

paths.py:119-152 vs paths.py:155-205

get_config_dir() checks env vars in this order:

  1. LC_CONFIG_DIR
  2. LC_LEGACY_CONFIG
  3. Platform default

get_config_path() checks in this order:

  1. LC_CREDS_FILE
  2. LC_LEGACY_CONFIG
  3. LC_CONFIG_DIR
  4. New location / legacy fallback

When both LC_LEGACY_CONFIG=1 and LC_CONFIG_DIR are set (without LC_CREDS_FILE):

  • get_config_dir() returns LC_CONFIG_DIR (it wins at step 1)
  • get_config_path() returns ~/.limacharlie (legacy wins at step 2)
  • get_jwt_cache_path() returns ~/.limacharlie_jwt_cache (legacy wins)
  • get_checkpoint_dir() returns ~/.limacharlie.d/search_checkpoints/ (legacy wins)

So get_config_dir() says "your config is in /custom/dir" while the actual config file is at ~/.limacharlie. The config show-paths command would show a misleading config_dir.

The test test_multiple_env_vars_set_simultaneously actually documents this inconsistency as expected behavior, but it's confusing for users. The fix: get_config_dir() should check LC_LEGACY_CONFIG before LC_CONFIG_DIR to match all other path functions.


Security: JWT Cache Directory Created Without Secure Permissions

jwt_cache.py:213-214

if parent and not os.path.isdir(parent):
    os.makedirs(parent, exist_ok=True)

This creates the JWT cache parent directory with default permissions (potentially 0o755), while the migration and config writing paths use secure_makedirs (0o700). Since the JWT cache contains sensitive auth tokens, the directory should be created with restricted permissions too. Should use secure_makedirs(parent) instead.


Redundant Exception Catch

config_cmd.py:71

except (OSError, Exception):
    return False

Exception is a superclass of OSError, making OSError redundant here. Should be just except Exception: or, better, catch only expected exceptions like OSError to avoid silencing bugs like TypeError or NameError in safe_open_read.


Over-Testing Standard Python Behavior

Several tests in test_paths.py test Python's standard library rather than the code under review:

  • test_config_dir_with_spaces — Python handles spaces in paths
  • test_config_dir_with_unicode — Python 3 handles unicode paths
  • test_config_dir_with_dotdot_resolvedos.path.abspath resolves ..
  • test_creds_file_with_dotdot_resolved — same
  • test_null_byte_in_path_does_not_crash — Python's null byte handling
  • test_config_dir_is_root/ is a valid path
  • test_very_long_path — OS handles long paths
  • test_creds_file_with_trailing_slash — standard path handling

Per the project's guidelines ("Never write unit tests that test obvious things ... or test standard APIs"), these should be removed.


Duplicated _output Helper

config_cmd.py:75-78 and auth.py:31-34

Identical _output helper function is defined in both modules:

def _output(ctx: click.Context, data: Any) -> None:
    fmt = ctx.obj.output_format or detect_output_format()
    if not ctx.obj.quiet:
        click.echo(format_output(data, fmt))

This should be extracted to a shared location (e.g., the output.py module or cli.py).


Minor Issues

  1. Module-level path computation (paths.py:49-50): _LEGACY_CONFIG_FILE and _LEGACY_JWT_CACHE_FILE are computed at import time via os.path.expanduser. This is fine for production but means every test must monkeypatch the module-level variables. The tests handle this correctly, but it's fragile.

  2. config migrate exit codes not documented in --help: The command uses exit codes 0 (success), 1 (legacy mode), 2 (I/O error), and 3 (content mismatch). These are only documented in the PR description and test assertions, not in the command's help text.

  3. _deprecation_warned flag bypasses warnings module dedup: paths.py:100-101 uses a manual _deprecation_warned flag in addition to Python's built-in warning deduplication. The manual flag prevents re-emission after _reset_path_cache() (unless that also resets the flag, which it does). This is fine, but the code comment could note that this is intentional redundancy.


What's Done Well

  • Clean separation: paths.py as single source of truth, existing modules delegate
  • Safety-first migration: --dry-run, content verification before --remove-old, exit code 3 for mismatches
  • UserWarning instead of DeprecationWarning (visible to installed packages)
  • Per-process caching with clean reset for testing
  • The show-paths diagnostic command is very useful for debugging
  • Symlink rejection at all I/O boundaries
  • Atomic writes prevent partial reads
  • Integration tests cover realistic lifecycle scenarios (fresh install → legacy → migration → post-migration)

tomaz-lc and others added 5 commits March 19, 2026 19:02
- Fix priority inconsistency: get_config_dir() now checks LC_LEGACY_CONFIG
  before LC_CONFIG_DIR, matching the priority order used by get_config_path()
  and all other path functions. Previously, setting both env vars would cause
  get_config_dir() and get_config_path() to disagree on where config lives.

- Fix JWT cache directory permissions: _save_cache() now uses
  secure_makedirs() (0o700) instead of os.makedirs() (default 0o755) when
  creating the parent directory. JWT cache contains auth tokens and should
  have restricted permissions matching other config directories.

- Fix redundant exception catch: _safe_content_match() now catches only
  OSError instead of (OSError, Exception), since OSError is already a
  subclass of Exception.

- Extract duplicated _output helper: config_cmd.py and auth.py now import
  from shared _output_helpers.py instead of duplicating the same 4-line
  function.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add TestPriorityConsistency: verifies get_config_dir() and
  get_config_path() agree on priority when both LC_LEGACY_CONFIG and
  LC_CONFIG_DIR are set.

- Add TestSaveCacheDirectoryPermissions: verifies _save_cache() creates
  parent directories with 0o700 permissions via secure_makedirs.

- Add TestSafeContentMatch: verifies _safe_content_match() handles
  matching files, different files, missing files, and permission errors.

- Update test_multiple_env_vars_set_simultaneously to expect the fixed
  priority behavior (legacy mode wins over LC_CONFIG_DIR in
  get_config_dir).

- Remove 8 tests that exercised Python stdlib behavior rather than our
  path resolution logic: spaces in paths, unicode paths, null bytes,
  root path, very long paths, trailing slashes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… races

New test classes in test_file_utils.py covering gaps identified in review:

Symlink rejection (_reject_symlink, safe_open_read, atomic_write):
- Relative symlinks (os.symlink("target", link))
- Chained symlinks (link -> link -> file)
- Circular/self-referencing symlinks
- Dangling symlinks on read path
- Symlinks in secure_makedirs path components
- No temp file leak on symlink rejection

Permission model (secure_makedirs, atomic_write):
- secure_makedirs tightens existing permissive (0o755, 0o777) dirs to 0o700
- All intermediate dirs created with 0o700 (not just leaf)
- atomic_write overwriting world-readable (0o644) or world-writable (0o666)
  file results in 0o600
- safe_open_read does not alter file permissions

Race conditions / concurrency (atomic_write):
- Concurrent writers produce no partial reads (atomicity via os.replace)
- Final file after concurrent writes is valid (not a mix of two writers)
- Permissions preserved at 0o600 after concurrent writes
- No temp files left after concurrent writes complete
- os.replace replaces symlink entry itself, does not follow (TOCTOU defense)

Integration (config.save_config, config.load_config, jwt_cache.clear):
- save_config refuses to write through symlinked config.yaml
- load_config raises OSError on symlinked config.yaml (does not silently
  return attacker-controlled data)
- clear_jwt_cache removes symlink itself, not the symlink target

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root bypasses file permission checks (0o000 is still readable), so the
test_unreadable_file_returns_false test fails in CI which runs as root.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ests

Removed 14 tests that tested stdlib/OS behavior rather than our code:
- test_o_nofollow_rejects_symlink_at_kernel_level (tested os.open directly)
- test_os_replace_does_not_follow_symlinks (tested os.replace directly)
- _reject_symlink variant tests for relative/chained/circular/self-ref
  symlinks (all exercise the same os.path.islink call)
- Duplicate symlink variant tests on atomic_write and safe_open_read
  (relative, chained, dangling - same _reject_symlink code path)
- test_rejects_symlink_in_path_components (secure_makedirs delegates to
  os.mkdir which follows symlinks - stdlib behavior)
- test_read_does_not_change_file_permissions (verifying absence of code
  that doesn't exist)

Kept: all tests that exercise our security logic - _reject_symlink core
paths (file, parent dir, regular file accept, dangling), atomic_write
symlink rejection + temp cleanup, safe_open_read fd leak prevention,
permission tightening, concurrency, and integration tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@tomaz-lc
Copy link
Copy Markdown
Contributor Author

@maximelb Thanks for the review.

Feedback has been addressed in (+ additional improvements):

@tomaz-lc tomaz-lc merged commit 9b13e4a into cli-v2 Mar 20, 2026
1 check passed
@tomaz-lc tomaz-lc deleted the feat/config-directory-consolidation branch March 20, 2026 06:25
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tomaz-lc added a commit that referenced this pull request Mar 20, 2026
…265)

- Add config_cmd to _COMMAND_MODULE_MAP in cli.py (missing after #257
  and #261 were merged independently)
- Add config to EXPECTED_TOP_LEVEL_COMMANDS, EXPECTED_MODULE_MAP, and
  EXPECTED_SUBCOMMANDS in regression tests
- Fix test_cli_import_does_not_load_output to handle third-party deps
  already loaded by other tests in the same pytest process
- Add ci.yml GHA workflow that runs unit tests and dist checks on every
  push and PR

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants