feat: injectable credential storage + per-CLI config file

[jpage-godaddy]

Summary

Two related capabilities, plus a small refactor:

1. Injectable credential storage (disable the keyring)

Credential persistence is decoupled from PkceAuthProvider behind a CredentialStorage trait with three backends:

• KeyringStorage (default) — system keychain only
• FileStorage — unencrypted JSON under the config base dir (the WSL/headless escape hatch)
• AutoStorage — keychain with transparent file fallback

The mode is selectable, precedence highest→lowest: explicit .with_storage() / .with_credential_store() → --credential-store flag → ${PREFIX}_CREDENTIAL_STORE env → [credentials].store in config → default keyring. Default behavior is unchanged (keyring-only, hard error when unavailable). with_file_fallback is deprecated in favor of with_credential_store.

2. General per-CLI config file

A single per-application TOML file (<config-base>/<app_id>/config.toml) the engine shares with consumers:

• Engine settings live in reserved top-level tables ([credentials]); consumers own every other table.
• Read it from handlers (ctx.config().section::<T>("deploy")?) and module registration (ModuleContext::config()); toml stays encapsulated (typed access only).
• Opt-in built-in config group (CliConfig::with_config_commands()): config get/set/path/list. config set is format-preserving (toml_edit, keeps comments), dry-run aware, and validates engine-reserved keys.

3. Shared fs module

Filesystem/path primitives (config_base_dir, is_safe_path_component, atomic write_string_atomic) factored into a new fs module, reused by both config and credential storage instead of living in config and being reached into from storage.

Testing

• Unit tests across config, fs, auth::storage, auth::pkce (incl. a MemoryStorage double proving the provider delegates).
• Integration tests via Cli::run: tests/credential_store_config.rs (config/env/flag precedence) and tests/config_file.rs (consumer section read; config set→get round-trip preserving [credentials] + comments; dry-run no-write; config path).
• Gates pass for both default and pkce-auth: cargo fmt --check, clippy --all-targets -D warnings, RUSTDOCFLAGS='-D warnings' cargo doc, full tests + doc tests, missing-docs = 0.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds a shared per-CLI config file and makes PKCE credential persistence pluggable, enabling keyring-less environments (e.g., headless/WSL) via file-based storage while keeping the default keyring-only behavior unchanged for pkce-auth.

Changes:

• Introduces CredentialStorage + CredentialStore selection (flag/env/config/default) and updates PkceAuthProvider to use injectable storage backends.
• Adds a per-application TOML config file (<config-base>/<app_id>/config.toml) with typed consumer section reads and an opt-in built-in config command group.
• Factors shared filesystem/path/atomic-write logic into a new fs module and updates docs/tests accordingly.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/foundation.rs	Updates expected global-flag defaults to include credential_store.
tests/exhaustive_public_api.rs	Adds parsing coverage for --credential-store file.
tests/credential_store_config.rs	New integration tests for credential-store selection precedence via config/env/flag.
tests/config_file.rs	New integration tests for config file read/write and built-in config commands.
src/module.rs	Exposes ModuleContext::config() for registration-time config access.
src/middleware.rs	Stores loaded ConfigFile in middleware for shared access during a run.
src/lib.rs	Publishes new config, config_commands, fs modules and related re-exports.
src/fs.rs	New shared filesystem utilities: base dir resolution, safe path component validation, atomic writes.
src/flags.rs	Adds --credential-store flag and factors app-id env prefix logic into app_id_env_prefix().
src/config.rs	New config file implementation + credential-store resolution utilities and flag/env/config precedence.
src/config_commands.rs	New built-in config command group (path/get/set/list).
src/command.rs	Exposes CommandContext::config() for handler-time config access.
src/cli.rs	Loads config at startup, mounts optional config group, and publishes --credential-store selection per run.
src/auth/storage.rs	New injectable credential storage trait + file/keyring/auto backends and selectors.
src/auth/pkce.rs	Refactors PKCE provider to persist credentials via CredentialStorage and adds injection/selection APIs.
src/auth/mod.rs	Exposes new storage module + re-exports.
docs/concepts.md	Documents credential storage modes and the shared config file + built-in commands.
docs/auth.md	Updates PKCE docs to describe the storage abstraction and selection precedence.
Cargo.toml	Adds toml and toml_edit dependencies.
Cargo.lock	Locks new TOML dependencies and transitive updates.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

[jgowdy-godaddy]

Blockers coming

[jgowdy-godaddy]

Code Review Findings & Fixes

Reviewed the implementation of injectable credential storage + per-CLI config file. I've pushed fixes for all findings to this branch as commit ed3cf5c. Each finding is documented below with its severity and the specific change applied.

---

🔴 Blocking — Fix 1: Dual TOML dependency with string round-trip

Finding: Cargo.toml added both toml = "0.8" and toml_edit = "0.22" as direct dependencies. ConfigFile stored a read_model: toml::Table field that was kept in sync with the toml_edit::DocumentMut document via a string round-trip: toml::from_str(&doc.to_string()).unwrap_or_default(). This is fragile for two reasons: (a) two independent TOML crates with their own parse implementations can diverge in what they accept; (b) the .unwrap_or_default() silently yields an empty config on failure, masking any divergence.

Fix (Cargo.toml, src/config.rs): Remove toml = "0.8". Enable the serde feature on toml_edit = "0.22" and use toml_edit::de::from_document / toml_edit::de::from_str directly for all typed deserialization. Remove the read_model field from ConfigFile and the parse_read_model function. The section<T> method now extracts the named table's key-value pairs into a temporary DocumentMut at the root level and deserializes from that. All tests that called toml::from_str are updated to use toml_edit::de::from_str.

---

🔴 Blocking — Fix 2: Process-global CREDENTIAL_STORE_FLAG race condition

Finding: CREDENTIAL_STORE_FLAG: AtomicU8 was a process-wide static. With Ordering::Relaxed store/load, concurrent Cli::run calls on different OS threads could observe each other's flag. Even for sequential calls, the flag persisted after a run with --credential-store and could theoretically be observed by a subsequent call before it overwrote the value. The integration tests papered over this with a "reset the flag latch" second CLI invocation, which is a code smell.

Fix (src/config.rs, src/cli.rs): Replace the AtomicU8 global with a thread_local! { Cell<u8> }. Each OS thread now has its own flag value; concurrent Cli::run calls on separate threads cannot interfere. Additionally, add clear_credential_store_flag() called from finish_run() so the flag is explicitly cleared at the end of every run, preventing leakage across sequential runs on the same thread. Document the remaining limitation: concurrent Cli::run calls on the same OS thread (e.g. same tokio task) are not supported.

---

🟡 Non-blocking — Fix 3: default_storage reads thread-local state without documentation

Finding: default_storage(app_id) calls resolve_credential_store which reads the per-thread latch set by Cli::run. The public function was documented only as "resolution consults the CLI flag" without noting that the "CLI flag" is thread-local state. Callers outside of a Cli::run execution would silently get None for the flag and might not understand why the flag appears to have no effect.

Fix (src/auth/storage.rs): Expand the default_storage docstring to explicitly state it reads thread-local state set by Cli::run, what happens when called outside that context, and when to prefer storage_for(mode) with an explicit CredentialStore argument.

---

🟡 Non-blocking — Fix 4: KeyringStorage::load conflates "no entry" and "unavailable" without documentation

Finding: KeyringStorage::load collapses the three-state keychain read (Some(Some(json)) = found, Some(None) = reachable but empty, None = backend unavailable) to two states: entry present or None. Callers of KeyringStorage directly cannot distinguish "not logged in" from "keychain unavailable". This is intentional by design (keychain-only mode should hard-error on save, not fall back silently on load), but the docstring did not make this explicit.

Fix (src/auth/storage.rs): Expand the KeyringStorage struct docstring to explicitly state that load collapses both "no entry" and "backend unavailable" to None, that callers cannot distinguish them, and that read_three_state is available for code that needs the distinction (as AutoStorage uses it).

---

🟡 Non-blocking — Fix 5: infer_toml_value type coercion not documented for edge cases

Finding: config set coerces the string value to a TOML type using parse::<bool> → parse::<i64> → parse::<f64> → string. The help text said only "parsed as bool/int/float when possible." Edge cases like "1e5" (stored as float 100000.0), "inf", "nan", and "null" (stored as a string, not TOML null which doesn't exist) could surprise users. There was no way to force a numeric-looking value to be stored as a string.

Fix (src/config.rs, src/config_commands.rs): Expand the ConfigFile::set docstring with explicit coercion rules and a note about the limitation (no quoting escape hatch). Update the config set VALUE clap help text to document the coercion chain including float syntax examples.

---

🟡 Non-blocking — Fix 6: Engine-reserved key validation only covered credentials.store

Finding: ConfigFile::set validated credentials.store against the CredentialStore enum but allowed any other key under [credentials] to be written (e.g. config set credentials.timeout 30). The engine silently ignores unknown keys in [credentials], so this would write a no-op value that misleads users into thinking they've configured something.

Fix (src/config.rs): Change the guard to reject any key whose first segment is in ENGINE_RESERVED_TABLES (["credentials"]) unless the full key matches a known engine key. Only credentials.store is accepted; all other credentials.* keys return a clear error message. Add a test covering rejection of unknown credentials.* keys.

---

🟡 Non-blocking — Fix 7: config get vs config set snapshot asymmetry undocumented

Finding: config set loads the config fresh from disk before writing (to avoid clobbering concurrent edits), while ctx.config() in command handlers returns the startup snapshot loaded at Cli::new. A config set in process A does not update the snapshot in process A's subsequent command handlers. This is correct and expected for a one-shot CLI, but was not documented — it would surprise library consumers calling Cli::run multiple times on the same Cli instance.

Fix (src/command.rs, src/module.rs, src/config_commands.rs): Add "Snapshot semantics" notes to CommandContext::config() and ModuleContext::config(). Add a note to the config set handler explaining that the startup snapshot in context.config() is not updated by the write, and that a new Cli is needed to see the change.

---

🟡 Non-blocking — Fix 8: write_string_atomic and ConfigFile::load perform blocking I/O without documentation

Finding: write_string_atomic uses synchronous std::fs I/O throughout (create_dir_all, set_permissions, file open/write, rename). ConfigFile::load uses std::fs::read_to_string. Both are called in contexts that may be on an async executor thread: ConfigFile::load is called from Cli::new (which is sync but called in an async binary), and write_string_atomic was previously called directly from a synchronous path. The async FileStorage::save already wraps the call in spawn_blocking correctly, but that context was not clear from the function signatures.

Fix (src/fs.rs, src/config.rs): Add **Blocking**: this function uses synchronous filesystem I/O notes to write_string_atomic and ConfigFile::load, with guidance to use spawn_blocking when calling from async contexts.

---

All eight fixes compile clean, pass cargo fmt --check, cargo clippy --all-targets -D warnings, RUSTDOCFLAGS='-D warnings' cargo doc --no-deps, cargo test --workspace --all-targets, cargo test --doc, and cargo rustdoc --lib -- -W missing-docs (0 missing-docs).