-
Notifications
You must be signed in to change notification settings - Fork 1
feat(consent): local source discovery + per-source opt-in (task 002) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| //! File-backed [`ConsentStore`]: the user's per-source opt-in, persisted as a small JSON map | ||
| //! (`{ "<source-id>": true }`) in the app config dir. Consent is **not** a secret, so it lives | ||
| //! here as plain settings — never in the keychain (ADR 0012). A missing entry means the source | ||
| //! is **disabled**, so on a fresh install nothing is read until the user opts in. | ||
| use std::collections::BTreeMap; | ||
| use std::path::PathBuf; | ||
|
|
||
| use parking_lot::Mutex; | ||
|
|
||
| use mlt_core::domain::ProviderId; | ||
| use mlt_core::ports::{ConsentStore, PortError}; | ||
|
|
||
| /// In-memory consent map with write-through to a JSON file. The map is the source of truth at | ||
| /// runtime (so reads on the refresh hot path never touch disk); every change is persisted so | ||
| /// the choice survives a restart. | ||
| #[derive(Debug)] | ||
| pub struct FileConsentStore { | ||
| path: PathBuf, | ||
| state: Mutex<BTreeMap<String, bool>>, | ||
| } | ||
|
|
||
| impl FileConsentStore { | ||
| /// Load consent from `path`. Best-effort: a missing or unparseable file starts empty | ||
| /// (every source opted-out), so a corrupt settings file can never accidentally enable a | ||
| /// source — it fails closed. | ||
| pub fn load(path: PathBuf) -> Self { | ||
| let state = std::fs::read_to_string(&path) | ||
| .ok() | ||
| .and_then(|raw| serde_json::from_str::<BTreeMap<String, bool>>(&raw).ok()) | ||
| .unwrap_or_default(); | ||
| Self { | ||
| path, | ||
| state: Mutex::new(state), | ||
| } | ||
| } | ||
|
|
||
| fn persist(&self, map: &BTreeMap<String, bool>) -> Result<(), PortError> { | ||
| if let Some(parent) = self.path.parent() { | ||
| std::fs::create_dir_all(parent).map_err(|e| PortError::Io(e.to_string()))?; | ||
| } | ||
| let json = serde_json::to_string_pretty(map).map_err(|e| PortError::Io(e.to_string()))?; | ||
| std::fs::write(&self.path, json).map_err(|e| PortError::Io(e.to_string())) | ||
| } | ||
| } | ||
|
|
||
| impl ConsentStore for FileConsentStore { | ||
| fn is_enabled(&self, id: &ProviderId) -> Result<bool, PortError> { | ||
| Ok(self.state.lock().get(id.as_str()).copied().unwrap_or(false)) | ||
| } | ||
|
|
||
| fn set_enabled(&self, id: &ProviderId, enabled: bool) -> Result<(), PortError> { | ||
| let mut map = self.state.lock(); | ||
| let previous = map.insert(id.as_str().to_string(), enabled); | ||
| if let Err(e) = self.persist(&map) { | ||
| // Persist failed — undo the in-memory change so runtime consent never diverges | ||
| // from disk. Fail closed: a source is only ever treated as opted-in once that | ||
| // choice has been durably recorded. | ||
| match previous { | ||
| Some(prev) => { | ||
| map.insert(id.as_str().to_string(), prev); | ||
| } | ||
| None => { | ||
| map.remove(id.as_str()); | ||
| } | ||
| } | ||
| return Err(e); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn temp_path(tag: &str) -> PathBuf { | ||
| std::env::temp_dir().join(format!( | ||
| "mlt-consent-test-{}-{tag}/consent.json", | ||
| std::process::id() | ||
| )) | ||
| } | ||
|
|
||
| #[test] | ||
| fn unknown_source_defaults_to_disabled() { | ||
| let store = FileConsentStore::load(temp_path("default")); | ||
| assert!(!store.is_enabled(&ProviderId::new("claude-code")).unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn opt_in_persists_across_a_reload() { | ||
| let path = temp_path("persist"); | ||
| let _ = std::fs::remove_dir_all(path.parent().unwrap()); | ||
| let id = ProviderId::new("claude-code"); | ||
|
|
||
| let store = FileConsentStore::load(path.clone()); | ||
| store.set_enabled(&id, true).unwrap(); | ||
| // A fresh instance reading the same file (i.e. an app restart) still sees the opt-in. | ||
| let reloaded = FileConsentStore::load(path.clone()); | ||
| assert!( | ||
| reloaded.is_enabled(&id).unwrap(), | ||
| "consent survives restart" | ||
| ); | ||
|
|
||
| // Opting back out is likewise persisted. | ||
| reloaded.set_enabled(&id, false).unwrap(); | ||
| assert!(!FileConsentStore::load(path.clone()) | ||
| .is_enabled(&id) | ||
| .unwrap()); | ||
|
|
||
| let _ = std::fs::remove_dir_all(path.parent().unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn per_source_toggles_are_independent() { | ||
| let path = temp_path("independent"); | ||
| let _ = std::fs::remove_dir_all(path.parent().unwrap()); | ||
| let store = FileConsentStore::load(path.clone()); | ||
|
|
||
| store.set_enabled(&ProviderId::new("a"), true).unwrap(); | ||
| store.set_enabled(&ProviderId::new("b"), false).unwrap(); | ||
| assert!(store.is_enabled(&ProviderId::new("a")).unwrap()); | ||
| assert!(!store.is_enabled(&ProviderId::new("b")).unwrap()); | ||
|
|
||
| let _ = std::fs::remove_dir_all(path.parent().unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn failed_persist_leaves_runtime_consent_unchanged() { | ||
| let dir = std::env::temp_dir().join(format!( | ||
| "mlt-consent-test-{}-failclosed", | ||
| std::process::id() | ||
| )); | ||
| let _ = std::fs::remove_dir_all(&dir); | ||
| std::fs::create_dir_all(&dir).unwrap(); | ||
| // Put a *file* where the consent dir's parent should be, so `create_dir_all` (and | ||
| // therefore `persist`) fails deterministically — no real disk fault needed. | ||
| let blocker = dir.join("blocker"); | ||
| std::fs::write(&blocker, "x").unwrap(); | ||
| let store = FileConsentStore::load(blocker.join("consent.json")); | ||
| let id = ProviderId::new("claude-code"); | ||
|
|
||
| assert!(store.set_enabled(&id, true).is_err(), "persist must fail"); | ||
| // The opt-in must NOT have taken effect in memory: disk and runtime stay in lockstep, | ||
| // so a source is never read on the strength of a write that never landed. | ||
| assert!(!store.is_enabled(&id).unwrap()); | ||
|
|
||
| let _ = std::fs::remove_dir_all(&dir); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| //! [`SourceProbe`] implementation: metadata-only discovery of local sources (ADR 0012). | ||
| //! | ||
| //! Dispatches a source id to its presence check. Every branch decides presence from existence | ||
| //! alone (a credentials file, a Keychain item) and never reads a secret — the per-source | ||
| //! checks live with their credential adapter (e.g. [`crate::claude::ClaudeCredentials::is_present`]). | ||
| use async_trait::async_trait; | ||
|
|
||
| use mlt_core::domain::ProviderId; | ||
| use mlt_core::ports::SourceProbe; | ||
|
|
||
| use crate::claude::ClaudeCredentials; | ||
|
|
||
| /// Probes the real machine for each known source. Unknown ids report absent rather than | ||
| /// erroring, so the catalog can list a source before its probe exists. | ||
| #[derive(Debug, Default, Clone, Copy)] | ||
| pub struct LocalSourceProbe; | ||
|
|
||
| #[async_trait] | ||
| impl SourceProbe for LocalSourceProbe { | ||
| async fn is_present(&self, id: &ProviderId) -> bool { | ||
| match id.as_str() { | ||
| "claude-code" => ClaudeCredentials::is_present(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified, but deferring (same as CodeRabbit's note on the same probe). It is a real blocking-in-async call, but:
ADR 0015's per-probe timeout/failure-gate is mandated but not yet implemented; it should land as one uniform resilience pass over the probe and fetch paths, not a spot-fix here. Tracking it there rather than diverging in this consent PR.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tracked as #3 — uniform |
||
| _ => false, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[tokio::test] | ||
| async fn unknown_source_is_absent() { | ||
| assert!(!LocalSourceProbe.is_present(&ProviderId::new("nope")).await); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: ogrodev/MLT
Length of output: 3990
🏁 Script executed:
Repository: ogrodev/MLT
Length of output: 6499
Presence probe can block tokio runtime and lacks per-probe timeout
crates/adapters/src/claude.rs(keychain_present, ~147-153) runs/usr/bin/security find-generic-password ...via synchronousstd::process::Command::output()with no timeout; it’s called directly from the asyncLocalSourceProbe::is_present(crates/adapters/src/sources.rs) withoutspawn_blocking/bounded wait. Ifsecuritystalls, it can block runtime worker threads. Errors are mapped tofalse(unwrap_or(false)), but hangs aren’t bounded—ADR 0015 requires a per-probe timeout/failure gate for upstream probes. Move the blocking subprocess off the async executor and enforce a hard timeout (and kill the child on timeout).🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified, but pushing back on doing this in this PR.
The finding is real in the narrow sense (a blocking subprocess awaited on the async executor, no timeout), but two things matter:
keychain_presentcallssecurity find-generic-passwordwithout-w— it reads the item's attributes, never decrypts the secret, so it does not trigger a Keychain ACL prompt and returns promptly. The variant that can actually block on a user prompt isread_keychain(-w), which is pre-existing and on the consentedloadpath.read_keychain(-w),detect_user_agent(claude --version), andClaudeCredentials::load'sstd::fs::read_to_stringall run synchronously inside async fns. The same refresh loop that calls this probe also callsclaude_usage → load → read_keychain, so wrapping only the new probe inspawn_blocking+ timeout would create a divergent pattern and false assurance while the larger exposure (the fetch path) stays unbounded.ADR 0015's per-probe timeout / failure-gate is a mandated-but-not-yet-implemented resilience pattern. It should be applied uniformly across the adapter's blocking calls and the usage fetch as a dedicated resilience change, not spot-welded onto this one probe. Deferring it there rather than diverging here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ogrodev, that's a well-reasoned pushback — accepted. The distinction betweenkeychain_present(attributes-only, no ACL prompt) andread_keychain(-w, secret decrypt) is the key point I missed, and the argument for uniform ADR 0015 treatment rather than a divergent spot-fix is sound. Applyingspawn_blocking+ timeout to only this probe while leavingread_keychain,detect_user_agent, andfs::read_to_stringinload()unbounded would create inconsistency without meaningfully reducing the actual exposure.Would you like me to open a follow-up issue to track the ADR 0015 resilience work (uniform
spawn_blocking+ bounded timeout across all blocking adapter calls)?✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks — I went ahead and opened #3 to track the uniform ADR 0015 pass (spawn_blocking + bounded timeout across
read_keychain,detect_user_agent,load's fs read, and this probe), so no need for the bot to file one.