diff --git a/crates/nono-cli/Cargo.toml b/crates/nono-cli/Cargo.toml index d4db6513c..a3c9449f0 100644 --- a/crates/nono-cli/Cargo.toml +++ b/crates/nono-cli/Cargo.toml @@ -95,7 +95,7 @@ urlencoding = "2" tempfile.workspace = true vt100 = "0.16.2" shlex = "2" -jsonc-parser = { version = "0.34", features = ["serde"] } +jsonc-parser = { version = "0.34", features = ["serde", "cst"] } [target.'cfg(target_os = "linux")'.dependencies] nix = { workspace = true, features = ["process", "signal", "fs", "user", "term", "dir"] } diff --git a/crates/nono-cli/src/exec_strategy.rs b/crates/nono-cli/src/exec_strategy.rs index e24327056..37656d306 100644 --- a/crates/nono-cli/src/exec_strategy.rs +++ b/crates/nono-cli/src/exec_strategy.rs @@ -113,6 +113,7 @@ pub(crate) struct ProfileSaveOffer<'a> { pub(crate) compared_profile: Option<&'a str>, pub(crate) sandbox_violations: &'a [nono::SandboxViolation], pub(crate) ignored_denial_paths: &'a [std::path::PathBuf], + pub(crate) profile_save_files: &'a [crate::profile::ProfileSourceFile], pub(crate) url_denials: &'a [UrlDenialRecord], } @@ -292,6 +293,8 @@ pub struct ExecConfig<'a> { pub profile_save_base: Option<&'a str>, /// Denied paths that should not be offered in the save-profile prompt. pub ignored_denial_paths: &'a [std::path::PathBuf], + /// Writable profile files a save may target, highest precedence first. + pub profile_save_files: &'a [crate::profile::ProfileSourceFile], /// Non-filesystem sandbox operations suppressed from diagnostic footers. pub suppressed_system_service_operations: &'a [String], /// Optional startup timeout for known interactive CLIs that were launched @@ -1762,6 +1765,7 @@ pub fn execute_supervised bool>( compared_profile: config.profile_save_base, sandbox_violations: &visible_sandbox_violations, ignored_denial_paths: config.ignored_denial_paths, + profile_save_files: config.profile_save_files, url_denials: &url_denials, }; // An optional profile-save failure never changes the child status. diff --git a/crates/nono-cli/src/exec_strategy/clone_files/tests.rs b/crates/nono-cli/src/exec_strategy/clone_files/tests.rs index d74c098b1..b5e031794 100644 --- a/crates/nono-cli/src/exec_strategy/clone_files/tests.rs +++ b/crates/nono-cli/src/exec_strategy/clone_files/tests.rs @@ -61,6 +61,7 @@ fn config(caps: &CapabilitySet, filesystem: bool, proxy: bool) -> ExecConfig<'_> protected_paths: &[], profile_save_base: None, ignored_denial_paths: &[], + profile_save_files: &[], suppressed_system_service_operations: &[], startup_timeout: None, seccomp_policy: SeccompPolicy { diff --git a/crates/nono-cli/src/execution_runtime.rs b/crates/nono-cli/src/execution_runtime.rs index 370f77c46..4c99aa633 100644 --- a/crates/nono-cli/src/execution_runtime.rs +++ b/crates/nono-cli/src/execution_runtime.rs @@ -728,6 +728,7 @@ pub(crate) fn execute_sandboxed(plan: LaunchPlan) -> Result<()> { .as_deref() .or(recommended_profile), ignored_denial_paths: &flags.ignored_denial_paths, + profile_save_files: &flags.profile_save_files, suppressed_system_service_operations: &flags.suppressed_system_service_operations, startup_timeout: flags .startup_timeout_secs diff --git a/crates/nono-cli/src/launch_runtime.rs b/crates/nono-cli/src/launch_runtime.rs index 41ab15d84..431e5db24 100644 --- a/crates/nono-cli/src/launch_runtime.rs +++ b/crates/nono-cli/src/launch_runtime.rs @@ -245,6 +245,7 @@ pub(crate) struct ExecutionFlags { pub(crate) proc_comm_notify: bool, pub(crate) bypass_protection_paths: Vec, pub(crate) ignored_denial_paths: Vec, + pub(crate) profile_save_files: Vec, pub(crate) suppressed_system_service_operations: Vec, pub(crate) profile_display_name: Option, pub(crate) session: SessionLaunchOptions, @@ -305,6 +306,7 @@ impl ExecutionFlags { proc_comm_notify: prepared.proc_comm_notify, bypass_protection_paths: prepared.bypass_protection_paths.clone(), ignored_denial_paths: prepared.ignored_denial_paths.clone(), + profile_save_files: prepared.profile_save_files.clone(), suppressed_system_service_operations: prepared .suppressed_system_service_operations .clone(), diff --git a/crates/nono-cli/src/main.rs b/crates/nono-cli/src/main.rs index 371b7fca0..37024e71d 100644 --- a/crates/nono-cli/src/main.rs +++ b/crates/nono-cli/src/main.rs @@ -50,6 +50,7 @@ mod platform_client; mod policy; mod profile; mod profile_cmd; +mod profile_file_edit; mod profile_runtime; mod profile_save_runtime; mod protected_paths; @@ -326,6 +327,7 @@ mod tests { open_url_allow_localhost: false, bypass_protection_paths: Vec::new(), ignored_denial_paths: Vec::new(), + profile_save_files: Vec::new(), suppressed_system_service_operations: Vec::new(), redaction_extra_env_vars: Vec::new(), network_denial_audit: Default::default(), @@ -405,6 +407,7 @@ mod tests { open_url_allow_localhost: false, bypass_protection_paths: Vec::new(), ignored_denial_paths: Vec::new(), + profile_save_files: Vec::new(), suppressed_system_service_operations: Vec::new(), redaction_extra_env_vars: Vec::new(), network_denial_audit: Default::default(), diff --git a/crates/nono-cli/src/pack_update_hint.rs b/crates/nono-cli/src/pack_update_hint.rs index ef0500975..9aae4dfe4 100644 --- a/crates/nono-cli/src/pack_update_hint.rs +++ b/crates/nono-cli/src/pack_update_hint.rs @@ -160,7 +160,7 @@ fn collect_profile_packs(profile_name: &str) -> Vec<(String, String)> { } // Walk extends for all profiles, pack-provided or not, so a user // profile that extends a pack profile is handled correctly. - if let Some(bases) = crate::profile::load_profile_extends(&name) { + if let Some(bases) = crate::profile::load_profile_extends_resolved(&name) { queue.extend(bases); } } @@ -345,4 +345,49 @@ mod tests { assert!(is_opted_out()); } + + #[test] + fn collect_profile_packs_follows_relative_path_extends() { + crate::test_env::with_isolated_config_home(|config_home| { + crate::test_env::write_fake_pack( + config_home, + "acme", + "widget", + "widget", + r#"{ "meta": { "name": "widget" } }"#, + &[], + None, + ); + let lockfile = crate::package::lockfile_path().expect("lockfile path"); + std::fs::create_dir_all(lockfile.parent().expect("parent")).expect("mkdir"); + std::fs::write( + &lockfile, + r#"{ "lockfile_version": 1, "packages": { + "acme/widget": { "version": "1.2.3", "installed_at": "2026-01-01T00:00:00Z" } + } }"#, + ) + .expect("write lockfile"); + + let nono_dir = config_home.join("proj/.nono"); + let shared = config_home.join("proj/shared"); + std::fs::create_dir_all(&nono_dir).expect("mkdir .nono"); + std::fs::create_dir_all(&shared).expect("mkdir shared"); + std::fs::write( + shared.join("base.json"), + r#"{ "meta": { "name": "base" }, "extends": "widget" }"#, + ) + .expect("write base"); + let agent = nono_dir.join("agent.json"); + std::fs::write( + &agent, + r#"{ "meta": { "name": "agent" }, "extends": "../shared/base.json" }"#, + ) + .expect("write agent"); + + assert_eq!( + collect_profile_packs(agent.to_str().expect("utf-8 path")), + vec![("acme/widget".to_string(), "1.2.3".to_string())] + ); + }); + } } diff --git a/crates/nono-cli/src/package_status.rs b/crates/nono-cli/src/package_status.rs index f07561579..ca2a9c44c 100644 --- a/crates/nono-cli/src/package_status.rs +++ b/crates/nono-cli/src/package_status.rs @@ -231,7 +231,7 @@ fn walk_extends_chain( } visited.push(name_or_path.to_string()); - let Some(bases) = profile::load_profile_extends(name_or_path) else { + let Some(bases) = profile::load_profile_extends_resolved(name_or_path) else { return false; }; bases @@ -436,6 +436,32 @@ mod tests { }); } + #[test] + fn claude_code_detection_follows_relative_path_extends() { + with_isolated_config_home(|config_home| { + let nono_dir = config_home.join("proj/.nono"); + let shared = config_home.join("proj/shared"); + std::fs::create_dir_all(&nono_dir).expect("mkdir .nono"); + std::fs::create_dir_all(&shared).expect("mkdir shared"); + std::fs::write( + shared.join("base.json"), + r#"{ "meta": { "name": "base" }, "extends": "nolabs-ai/claude" }"#, + ) + .expect("write base"); + let agent = nono_dir.join("agent.json"); + std::fs::write( + &agent, + r#"{ "meta": { "name": "agent" }, "extends": "../shared/base.json" }"#, + ) + .expect("write agent"); + + assert!( + selects_claude_code(agent.to_str().expect("utf-8 path")), + "a relative path base reaching the pack ref must be treated as Claude Code" + ); + }); + } + /// Aliases live in the pack manifest, not in `CLAUDE_PACK.profiles`, so /// detection must ask the resolver instead of the hardcoded name list. #[test] diff --git a/crates/nono-cli/src/policy.rs b/crates/nono-cli/src/policy.rs index 3516eaab5..c47149345 100644 --- a/crates/nono-cli/src/policy.rs +++ b/crates/nono-cli/src/policy.rs @@ -187,6 +187,7 @@ impl ProfileDef { command_args: self.command_args.clone(), unsafe_macos_seatbelt_rules: self.unsafe_macos_seatbelt_rules.clone(), platform_overrides: self.platform_overrides.clone(), + source_files: Vec::new(), } } } diff --git a/crates/nono-cli/src/profile/extends_ref.rs b/crates/nono-cli/src/profile/extends_ref.rs new file mode 100644 index 000000000..41a40193d --- /dev/null +++ b/crates/nono-cli/src/profile/extends_ref.rs @@ -0,0 +1,349 @@ +//! Classification and resolution of `extends` entries. +//! +//! An entry is a built-in/user profile name, a registry pack reference, or a +//! relative `./`/`../` path to a profile file. + +use super::{ + is_registry_ref, is_under_dir, is_under_user_profile_draft_dir, is_valid_profile_name, +}; +use nono::{NonoError, Result}; +use std::path::{Path, PathBuf}; + +const ERR_ABSOLUTE_PATH: &str = + "absolute and `~/` paths are not supported in `extends`; use a relative path"; +const ERR_PACK_STORE_TARGET: &str = "extend a pack by name (`org/pack`), not by path"; +const ERR_NOT_JSON: &str = "path entries in `extends` must end in `.json` or `.jsonc`"; +const ERR_PATH_IN_PACK_PROFILE: &str = "path entries in `extends` are not allowed in pack profiles"; +const ERR_PATH_IN_DRAFT: &str = "path entries in `extends` are not allowed in profile drafts"; +const ERR_PATH_IN_BUILTIN: &str = "path entries in `extends` are not allowed in built-in profiles"; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ExtendsRef { + Name(String), + Registry(String), + /// Canonical path to the base profile file. + Path(PathBuf), +} + +/// Where an `extends` entry was written. +#[derive(Debug, Clone, Copy)] +pub(crate) enum ExtendsOrigin<'a> { + /// Entry written in the profile file at this path (canonical when loaded; + /// may not exist yet for `profile init`). + File(&'a Path), + /// Entry from `--extends`; paths resolve against this directory. + Cli(&'a Path), + Builtin, +} + +impl ExtendsRef { + /// Key identifying this base in cycle detection: the name, or the + /// canonical path. + pub(crate) fn visited_key(&self) -> String { + match self { + Self::Name(s) | Self::Registry(s) => s.clone(), + Self::Path(p) => p.display().to_string(), + } + } +} + +fn inheritance_error(msg: String) -> NonoError { + NonoError::ProfileInheritance(msg) +} + +pub(crate) fn classify_extends_entry(raw: &str, origin: ExtendsOrigin<'_>) -> Result { + if raw.starts_with("./") || raw.starts_with("../") { + return resolve_path_entry(raw, origin).map(ExtendsRef::Path); + } + if raw.starts_with('/') || raw.starts_with('~') { + return Err(inheritance_error(format!("'{raw}': {ERR_ABSOLUTE_PATH}"))); + } + if is_registry_ref(raw) { + return Ok(ExtendsRef::Registry(raw.to_string())); + } + if is_valid_profile_name(raw) { + return Ok(ExtendsRef::Name(raw.to_string())); + } + Err(inheritance_error(format!( + "invalid base profile name '{raw}'" + ))) +} + +fn resolve_path_entry(raw: &str, origin: ExtendsOrigin<'_>) -> Result { + let base_dir = match origin { + ExtendsOrigin::Cli(dir) => dir.to_path_buf(), + ExtendsOrigin::Builtin => { + return Err(inheritance_error(format!("'{raw}': {ERR_PATH_IN_BUILTIN}"))); + } + ExtendsOrigin::File(file) => { + // The rejection checks compare canonical prefixes; a non-canonical + // spelling of a store or draft path would otherwise slip past them. + let file = &nono::try_canonicalize(file); + if is_under_pack_store(file) { + return Err(inheritance_error(format!( + "'{raw}': {ERR_PATH_IN_PACK_PROFILE}" + ))); + } + if is_under_user_profile_draft_dir(file) { + return Err(inheritance_error(format!("'{raw}': {ERR_PATH_IN_DRAFT}"))); + } + let parent = file.parent().ok_or_else(|| { + inheritance_error(format!( + "'{raw}': profile file {} has no parent directory", + file.display() + )) + })?; + parent.to_path_buf() + } + }; + + if !(raw.ends_with(".json") || raw.ends_with(".jsonc")) { + return Err(inheritance_error(format!("'{raw}': {ERR_NOT_JSON}"))); + } + + let joined = base_dir.join(raw); + let target = joined.canonicalize().map_err(|e| { + inheritance_error(format!( + "extends '{raw}' resolves to {}, which cannot be read: {e}", + joined.display() + )) + })?; + if is_under_pack_store(&target) { + return Err(inheritance_error(format!( + "'{raw}': {ERR_PACK_STORE_TARGET}" + ))); + } + Ok(target) +} + +/// True when `path` is inside the installed pack store. A missing store +/// contains nothing. +pub(crate) fn is_under_pack_store(path: &Path) -> bool { + is_under_dir(path, crate::package::package_store_dir()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_env::with_isolated_config_home; + use std::fs; + + fn origin_file(p: &Path) -> ExtendsOrigin<'_> { + ExtendsOrigin::File(p) + } + + fn err_text(r: Result) -> String { + r.expect_err("expected error").to_string() + } + + #[test] + fn bare_name_is_name() { + let r = classify_extends_entry("default", ExtendsOrigin::Builtin).expect("classify"); + assert_eq!(r, ExtendsRef::Name("default".into())); + } + + #[test] + fn registry_ref_is_registry() { + let r = classify_extends_entry("nolabs-ai/claude", ExtendsOrigin::Builtin).expect("ok"); + assert_eq!(r, ExtendsRef::Registry("nolabs-ai/claude".into())); + } + + #[test] + fn dot_slash_resolves_against_declaring_file_dir() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + fs::write(d.join("base.json"), "{}").expect("write"); + let child = d.join("child.json"); + let r = classify_extends_entry("./base.json", origin_file(&child)).expect("classify"); + assert_eq!(r, ExtendsRef::Path(d.join("base.json"))); + }); + } + + #[test] + fn dot_dot_resolves_to_parent() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + fs::create_dir(d.join("sub")).expect("mkdir"); + fs::write(d.join("base.json"), "{}").expect("write"); + let child = d.join("sub").join("child.json"); + let r = classify_extends_entry("../base.json", origin_file(&child)).expect("classify"); + assert_eq!(r, ExtendsRef::Path(d.join("base.json"))); + }); + } + + #[test] + fn cli_origin_resolves_against_given_dir() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + fs::write(d.join("x.json"), "{}").expect("write"); + let r = classify_extends_entry("./x.json", ExtendsOrigin::Cli(&d)).expect("classify"); + assert_eq!(r, ExtendsRef::Path(d.join("x.json"))); + }); + } + + #[test] + fn missing_path_errors_with_resolved_path() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + let msg = err_text(classify_extends_entry( + "./nope.json", + ExtendsOrigin::Cli(&d), + )); + assert!(msg.contains("./nope.json"), "{msg}"); + assert!( + msg.contains(&d.join("./nope.json").display().to_string()), + "{msg}" + ); + }); + } + + #[test] + fn path_without_json_extension_errors() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + fs::write(d.join("base"), "{}").expect("write"); + let msg = err_text(classify_extends_entry("./base", ExtendsOrigin::Cli(&d))); + assert!(msg.contains(ERR_NOT_JSON), "{msg}"); + }); + } + + #[test] + fn jsonc_extension_is_accepted() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + fs::write(d.join("base.jsonc"), "{}").expect("write"); + let r = classify_extends_entry("./base.jsonc", ExtendsOrigin::Cli(&d)).expect("ok"); + assert_eq!(r, ExtendsRef::Path(d.join("base.jsonc"))); + }); + } + + #[test] + fn absolute_and_tilde_rejected() { + for raw in ["/x.json", "~/x.json"] { + let msg = err_text(classify_extends_entry(raw, ExtendsOrigin::Builtin)); + assert!(msg.contains(ERR_ABSOLUTE_PATH), "{raw}: {msg}"); + } + } + + #[test] + fn path_into_pack_store_rejected() { + with_isolated_config_home(|cfg| { + let pack = cfg.join("nono/packages/ns/p/profiles"); + fs::create_dir_all(&pack).expect("mkdir"); + fs::write(pack.join("x.json"), "{}").expect("write"); + let msg = err_text(classify_extends_entry( + "./x.json", + ExtendsOrigin::Cli(&pack), + )); + assert!(msg.contains(ERR_PACK_STORE_TARGET), "{msg}"); + }); + } + + #[test] + fn path_entry_in_pack_store_profile_rejected() { + with_isolated_config_home(|cfg| { + let pack = cfg.join("nono/packages/ns/p/profiles"); + fs::create_dir_all(&pack).expect("mkdir"); + let file = pack.join("x.json"); + let msg = err_text(classify_extends_entry("./y.json", origin_file(&file))); + assert!(msg.contains(ERR_PATH_IN_PACK_PROFILE), "{msg}"); + }); + } + + #[test] + fn path_entry_in_draft_rejected() { + with_isolated_config_home(|cfg| { + let drafts = cfg.join("nono/profile-drafts"); + fs::create_dir_all(&drafts).expect("mkdir"); + let file = drafts.join("x.json"); + let msg = err_text(classify_extends_entry("./y.json", origin_file(&file))); + assert!(msg.contains(ERR_PATH_IN_DRAFT), "{msg}"); + }); + } + + #[test] + fn path_entry_in_draft_rejected_via_non_canonical_spelling() { + with_isolated_config_home(|cfg| { + let drafts = cfg.join("nono/profile-drafts"); + fs::create_dir_all(&drafts).expect("mkdir"); + fs::write(drafts.join("y.json"), "{}").expect("write"); + let file = cfg.join("nono/../nono/profile-drafts/x.json"); + let msg = err_text(classify_extends_entry("./y.json", origin_file(&file))); + assert!(msg.contains(ERR_PATH_IN_DRAFT), "{msg}"); + }); + } + + #[cfg(unix)] + #[test] + fn symlink_outside_store_into_pack_store_rejected() { + with_isolated_config_home(|cfg| { + let pack = cfg.join("nono/packages/ns/p/profiles"); + fs::create_dir_all(&pack).expect("mkdir"); + fs::write(pack.join("x.json"), "{}").expect("write"); + let tmp = tempfile::tempdir().expect("tempdir"); + let d = tmp.path().canonicalize().expect("canon"); + std::os::unix::fs::symlink(pack.join("x.json"), d.join("link.json")).expect("link"); + let child = d.join("child.json"); + let msg = err_text(classify_extends_entry("./link.json", origin_file(&child))); + assert!(msg.contains(ERR_PACK_STORE_TARGET), "{msg}"); + }); + } + + #[test] + fn path_entry_in_builtin_rejected() { + let msg = err_text(classify_extends_entry("./y.json", ExtendsOrigin::Builtin)); + assert!(msg.contains(ERR_PATH_IN_BUILTIN), "{msg}"); + } + + #[test] + fn cli_origin_skips_file_rules() { + with_isolated_config_home(|cfg| { + let drafts = cfg.join("nono/profile-drafts"); + fs::create_dir_all(&drafts).expect("mkdir"); + fs::write(drafts.join("x.json"), "{}").expect("write"); + let r = classify_extends_entry("./x.json", ExtendsOrigin::Cli(&drafts)).expect("ok"); + assert_eq!(r, ExtendsRef::Path(drafts.join("x.json"))); + }); + } + + #[cfg(unix)] + #[test] + fn symlinked_target_returns_canonical() { + with_isolated_config_home(|_| { + let tmp = tempfile::tempdir().expect("tempdir"); + let root = tmp.path().canonicalize().expect("canon"); + let (d, e) = (root.join("d"), root.join("e")); + fs::create_dir(&d).expect("mkdir"); + fs::create_dir(&e).expect("mkdir"); + fs::write(e.join("real.json"), "{}").expect("write"); + std::os::unix::fs::symlink(e.join("real.json"), d.join("base.json")).expect("link"); + let child = d.join("child.json"); + let r = classify_extends_entry("./base.json", origin_file(&child)).expect("ok"); + assert_eq!(r, ExtendsRef::Path(e.join("real.json"))); + }); + } + + #[test] + fn shared_slash_json_is_still_invalid() { + let msg = err_text(classify_extends_entry( + "shared/base.json", + ExtendsOrigin::Builtin, + )); + assert!(msg.contains("invalid base profile name"), "{msg}"); + } + + #[test] + fn visited_key_is_name_or_canonical_path() { + assert_eq!(ExtendsRef::Name("a".into()).visited_key(), "a"); + assert_eq!( + ExtendsRef::Path(PathBuf::from("/x/y.json")).visited_key(), + "/x/y.json" + ); + } +} diff --git a/crates/nono-cli/src/profile/mod.rs b/crates/nono-cli/src/profile/mod.rs index 17ff999a5..469f6da2a 100644 --- a/crates/nono-cli/src/profile/mod.rs +++ b/crates/nono-cli/src/profile/mod.rs @@ -6,6 +6,10 @@ pub(crate) mod builtin; mod credential_provider; +mod extends_ref; +pub(crate) use extends_ref::{ + ExtendsOrigin, ExtendsRef, classify_extends_entry, is_under_pack_store, +}; pub use credential_provider::{ CredentialProviderDef, CredentialProviderRequestBodyFormat, @@ -76,7 +80,7 @@ where deserialize_conditional_string_vec(deserializer, "origin") } -fn deserialize_conditional_string_vec<'de, D>( +pub(crate) fn deserialize_conditional_string_vec<'de, D>( deserializer: D, value_key: &'static str, ) -> std::result::Result, D::Error> @@ -2680,6 +2684,71 @@ pub struct Profile { /// are child-wins, lists are dedup-appended. #[serde(default, skip_serializing_if = "Option::is_none")] pub platform_overrides: Option, + /// Files this profile was loaded from, bases first and the top-level file + /// last. Built-in bases have no file and are not listed. + #[serde(skip)] + pub(crate) source_files: Vec, +} + +impl Profile { + /// Source files a save may write to, highest precedence first. + pub(crate) fn writable_source_files(&self) -> Vec { + self.source_files + .iter() + .rev() + .filter(|file| file.kind.is_writable()) + .cloned() + .collect() + } +} + +/// A file a loaded profile came from. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct ProfileSourceFile { + /// Canonical path to the profile file. + pub(crate) path: PathBuf, + pub(crate) kind: ProfileSourceKind, + /// The profile the session was started with, as opposed to a base. + pub(crate) top_level: bool, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(crate) enum ProfileSourceKind { + /// Under the user profile directory. + User, + /// Under the user profile-drafts directory. + Draft, + /// Any other file, such as a project-local `.nono/` profile. + Project, + /// Inside the installed pack store. + Pack, +} + +impl ProfileSourceFile { + /// Classify `canonical` by the directory it lives in. + pub(crate) fn new(canonical: PathBuf) -> Self { + let kind = if is_under_pack_store(&canonical) { + ProfileSourceKind::Pack + } else if is_under_user_profile_draft_dir(&canonical) { + ProfileSourceKind::Draft + } else if is_under_user_profile_dir(&canonical) { + ProfileSourceKind::User + } else { + ProfileSourceKind::Project + }; + Self { + path: canonical, + kind, + top_level: false, + } + } +} + +impl ProfileSourceKind { + /// Pack-store files are owned by the pack and never edited in place. + pub(crate) fn is_writable(self) -> bool { + self != Self::Pack + } } /// Per-OS patches for [`Profile::platform_overrides`]. @@ -2833,6 +2902,7 @@ impl From for Profile { command_args: raw.command_args, unsafe_macos_seatbelt_rules: raw.unsafe_macos_seatbelt_rules, platform_overrides: raw.platform_overrides, + source_files: Vec::new(), } } } @@ -2860,65 +2930,88 @@ pub fn is_user_override(name: &str) -> bool { .unwrap_or(false) } -/// Load a profile's raw (unresolved) extends target names. -/// -/// Returns `Some(base_names)` if the profile declares `extends`, `None` otherwise. -/// This reads the raw profile definition before inheritance resolution clears the field. -pub fn load_profile_extends(name_or_path: &str) -> Option> { - // This is a metadata-only preview parse: the caller is asking for the - // `extends` field, not the full resolved profile. The caller almost - // always follows up with a real `load_profile` call (see e.g. - // `cmd_show`, `cmd_list`, `print_profile_line`, `prepare_sandbox`), - // and that real load is the one whose deprecation warnings should - // reach the user. Without suppression we'd emit each warning twice - // for the same file. Drains still run, populating canonical state; - // only stderr emission and counter increments are suppressed. - let _suppress = crate::deprecation_warnings::WarningSuppressionGuard::begin(); - - // Direct file path +/// Find the profile file that `name_or_path` loads from, in resolver order: +/// direct path, registry ref (pack store), user profile, pack store. Returns +/// `None` for built-ins and for names that resolve to nothing. +fn locate_profile_file(name_or_path: &str) -> Option { if is_file_path_ref(name_or_path) { - return parse_profile_file(Path::new(name_or_path)) - .ok() - .and_then(|p| p.extends); + return Some(nono::try_canonicalize(Path::new(name_or_path))); } - // Registry refs are not valid bare profile names, so resolve them from the // pack store before the name check below rejects them. if is_registry_ref(name_or_path) { - return find_pack_store_profile(name_or_path) - .and_then(|(profile_path, _)| parse_profile_file(&profile_path).ok()) - .and_then(|p| p.extends); + return find_pack_store_profile(name_or_path).map(|(path, _)| path); } - if !is_valid_profile_name(name_or_path) { return None; } - - // User profile if let Ok(profile_path) = resolve_user_profile_path(name_or_path) && profile_path.exists() { - return parse_profile_file(&profile_path) - .ok() - .and_then(|p| p.extends); + return Some(profile_path); } + // Any installed pack that declares a profile artifact with matching + // `install_as`. + find_pack_store_profile(name_or_path).map(|(path, _)| path) +} - // Pack-store: any installed pack that declares a profile artifact with - // matching `install_as`. - if let Some((profile_path, _)) = find_pack_store_profile(name_or_path) { - return parse_profile_file(&profile_path) - .ok() - .and_then(|p| p.extends); +fn builtin_profile_extends(name: &str) -> Option> { + if !is_valid_profile_name(name) { + return None; } + let policy = crate::policy::load_embedded_policy().ok()?; + policy + .profiles + .get(name)? + .extends + .as_ref() + .map(|s| vec![s.clone()]) +} - // Built-in profile - if let Ok(policy) = crate::policy::load_embedded_policy() - && let Some(def) = policy.profiles.get(name_or_path) - { - return def.extends.as_ref().map(|s| vec![s.clone()]); +/// Load a profile's raw (unresolved) extends target names. +/// +/// Returns `Some(base_names)` if the profile declares `extends`, `None` otherwise. +/// This reads the raw profile definition before inheritance resolution clears the field. +pub fn load_profile_extends(name_or_path: &str) -> Option> { + // This is a metadata-only preview parse: the caller is asking for the + // `extends` field, not the full resolved profile. The caller almost + // always follows up with a real `load_profile` call (see e.g. + // `cmd_show`, `cmd_list`, `print_profile_line`, `prepare_sandbox`), + // and that real load is the one whose deprecation warnings should + // reach the user. Without suppression we'd emit each warning twice + // for the same file. Drains still run, populating canonical state; + // only stderr emission and counter increments are suppressed. + let _suppress = crate::deprecation_warnings::WarningSuppressionGuard::begin(); + + match locate_profile_file(name_or_path) { + Some(file) => parse_profile_file(&file).ok().and_then(|p| p.extends), + None => builtin_profile_extends(name_or_path), } +} - None +/// Like [`load_profile_extends`], but path entries become canonical absolute +/// path strings, so a caller can walk the chain without knowing which file an +/// entry was written in. Entries that fail to classify are dropped: callers +/// use this for best-effort detection, and the real load reports the error. +pub(crate) fn load_profile_extends_resolved(name_or_path: &str) -> Option> { + let _suppress = crate::deprecation_warnings::WarningSuppressionGuard::begin(); + + let Some(file) = locate_profile_file(name_or_path) else { + return builtin_profile_extends(name_or_path); + }; + let extends = parse_profile_file(&file).ok()?.extends?; + Some( + extends + .into_iter() + .filter_map( + |raw| match classify_extends_entry(&raw, ExtendsOrigin::File(&file)) { + Ok(ExtendsRef::Path(path)) => Some(path.display().to_string()), + Ok(_) => Some(raw), + Err(_) => None, + }, + ) + .collect(), + ) } /// Load a profile by name or file path @@ -2936,21 +3029,32 @@ pub fn load_profile_extends(name_or_path: &str) -> Option> { /// 4. Auto-pull prompt for the registry pack `nolabs-ai/claude` when /// the requested profile is `claude-code` (or inherits from it). pub fn load_profile(name_or_path: &str) -> Result { - load_profile_impl(name_or_path, &[]) + load_profile_with_cli_bases(name_or_path, &[]) } /// Load a profile by name or file path, injecting additional CLI-selected bases. /// -/// Non-empty `cli_extends` behaves as if those base names were prepended to the -/// selected profile's raw `extends` list before inheritance resolution. Bases -/// still resolve through the normal profile resolver, so cycle checks, sibling +/// `cli_extends` bases are merged ahead of the selected profile's own `extends` +/// bases. Path entries resolve against the current directory; names resolve +/// exactly as if written in the selected profile, so cycle checks, sibling /// lookup, pack provenance, migration prompts, and validation remain shared /// with JSON-authored inheritance. pub fn load_profile_with_extends(name_or_path: &str, cli_extends: &[String]) -> Result { - load_profile_impl(name_or_path, cli_extends) + // Only `--extends` entries need the cwd; a profile load without them must + // not fail because the cwd is gone. + let cli = if cli_extends.is_empty() { + Vec::new() + } else { + let cwd = std::env::current_dir().map_err(NonoError::Io)?; + cli_extends + .iter() + .map(|raw| classify_extends_entry(raw, ExtendsOrigin::Cli(&cwd))) + .collect::>>()? + }; + load_profile_with_cli_bases(name_or_path, &cli) } -fn load_profile_impl(name_or_path: &str, cli_extends: &[String]) -> Result { +fn load_profile_with_cli_bases(name_or_path: &str, cli: &[ExtendsRef]) -> Result { // Enable the chain-aware migration prompt for the duration of this // call: if `extends` resolution hits a pack-provided base that isn't // installed (e.g. user profile that `extends: ["claude-code"]`), @@ -2958,7 +3062,7 @@ fn load_profile_impl(name_or_path: &str, cli_extends: &[String]) -> Result Result bool { /// and `Err(_)` on validation/IO failures. Shared between `load_profile` /// (which then runs the migration prompt) and `load_profile_no_migrate` /// (which surfaces a not-found error directly). -fn load_profile_inner(name_or_path: &str, cli_extends: &[String]) -> Result> { +fn load_profile_inner(name_or_path: &str, cli: &[ExtendsRef]) -> Result> { if is_registry_ref(name_or_path) { - return load_registry_profile(name_or_path, cli_extends).map(Some); + return load_registry_profile(name_or_path, cli).map(Some); } if is_file_path_ref(name_or_path) { - return load_profile_from_path_impl(Path::new(name_or_path), cli_extends).map(Some); + return load_profile_from_path_impl(Path::new(name_or_path), cli).map(Some); } if !is_valid_profile_name(name_or_path) { return Err(NonoError::ProfileParse(format!( @@ -3061,14 +3165,14 @@ fn load_profile_inner(name_or_path: &str, cli_extends: &[String]) -> Result Result Result Re /// Load a profile from a registry pack. If the pack isn't installed locally, /// pull it first (Docker-style auto-pull with Sigstore verification). -fn load_registry_profile(name_or_path: &str, cli_extends: &[String]) -> Result { +fn load_registry_profile(name_or_path: &str, cli: &[ExtendsRef]) -> Result { let package_ref = crate::package::parse_package_ref(name_or_path)?; let install_dir = crate::package::package_install_dir(&package_ref.namespace, &package_ref.name)?; @@ -3313,7 +3417,7 @@ fn load_registry_profile(name_or_path: &str, cli_extends: &[String]) -> Result

Result { load_profile_from_path_impl(path, &[]) } -fn load_profile_from_path_impl(path: &Path, cli_extends: &[String]) -> Result { +fn load_profile_from_path_impl(path: &Path, cli: &[ExtendsRef]) -> Result { if !path.exists() { return Err(NonoError::ProfileRead { path: path.to_path_buf(), @@ -3343,7 +3447,7 @@ fn load_profile_from_path_impl(path: &Path, cli_extends: &[String]) -> Result Result { /// Resolve inheritance and apply implicit default-group merging for a raw profile. pub(crate) fn resolve_and_finalize_profile(profile: Profile) -> Result { - finalize_profile(resolve_extends(profile, &mut Vec::new(), 0, None, None)?) + finalize_profile(resolve_extends( + profile, + &mut Vec::new(), + 0, + None, + None, + &[], + )?) +} + +/// Like [`resolve_and_finalize_profile`] for a profile read from `draft_path`, +/// so path entries in `extends` are rejected as draft entries. +pub(crate) fn resolve_and_finalize_draft_profile( + profile: Profile, + draft_path: &Path, +) -> Result { + finalize_profile(resolve_extends( + profile, + &mut Vec::new(), + 0, + None, + Some(draft_path), + &[], + )?) } /// Get the implicit default groups for a finalized profile. @@ -3539,31 +3666,25 @@ pub(crate) fn parse_profile_bytes(content: &[u8]) -> Result { /// base and make `show`/`validate` disagree with post-`promote` resolution /// (#1565). Drafts resolve `extends` the same way as `resolve_and_finalize_profile` /// (user profiles → packs → builtins). -fn load_from_file(path: &Path, cli_extends: &[String]) -> Result { +fn load_from_file(path: &Path, cli: &[ExtendsRef]) -> Result { let (mut profile, source_path) = parse_file_backed_profile(path)?; - prepend_cli_extends(&mut profile, cli_extends); + profile.source_files.push(ProfileSourceFile { + top_level: true, + ..ProfileSourceFile::new(source_path.clone()) + }); let context_dir = if is_under_user_profile_draft_dir(&source_path) { None } else { source_path.parent() }; - resolve_extends(profile, &mut Vec::new(), 0, context_dir, Some(&source_path)) -} - -fn prepend_cli_extends(profile: &mut Profile, cli_extends: &[String]) { - if cli_extends.is_empty() { - return; - } - let mut extends = Vec::with_capacity( - cli_extends - .len() - .saturating_add(profile.extends.as_ref().map_or(0, Vec::len)), - ); - extends.extend(cli_extends.iter().cloned()); - if let Some(existing) = profile.extends.take() { - extends.extend(existing); - } - profile.extends = Some(extends); + resolve_extends( + profile, + &mut Vec::new(), + 0, + context_dir, + Some(&source_path), + cli, + ) } // ============================================================================ @@ -3582,8 +3703,11 @@ const MAX_INHERITANCE_DEPTH: usize = 10; /// profiles can reference each other by name. `source_file` is the path of /// the file whose extends are being resolved so sibling lookup can skip /// self-references (e.g. `.nono/codex.json` extending `"codex"` should not -/// resolve to itself). The `visited` vec tracks profile names already in the -/// chain to detect circular dependencies. +/// resolve to itself). Entries written in `source_file` are classified against +/// it; with no `source_file` they are treated as built-in. `leading` holds +/// already-classified bases merged ahead of the profile's own entries. The +/// `visited` vec tracks the `ExtendsRef::visited_key` of each base already in +/// the chain to detect circular dependencies. /// /// Shared transitive bases are handled naturally: `visited` tracks only the /// current ancestor chain (push before recurse, pop after). When two siblings @@ -3596,11 +3720,11 @@ fn resolve_extends( depth: usize, context_dir: Option<&Path>, source_file: Option<&Path>, + leading: &[ExtendsRef], ) -> Result { - let base_names = match child.extends { - Some(ref names) => names.clone(), - None => return Ok(child), - }; + if child.extends.is_none() && leading.is_empty() { + return Ok(child); + } if depth >= MAX_INHERITANCE_DEPTH { return Err(NonoError::ProfileInheritance(format!( @@ -3610,29 +3734,66 @@ fn resolve_extends( ))); } + let origin = match source_file { + Some(file) => ExtendsOrigin::File(file), + None => ExtendsOrigin::Builtin, + }; + let mut bases = leading.to_vec(); + for raw in child.extends.iter().flatten() { + bases.push(classify_extends_entry(raw, origin)?); + } + // Resolve each base and fold-merge them left-to-right let mut accumulated_base: Option = None; - for base_name in &base_names { - if visited.contains(base_name) { + for base in &bases { + let key = base.visited_key(); + if visited.contains(&key) { return Err(NonoError::ProfileInheritance(format!( "circular dependency detected: {} -> {}", visited.join(" -> "), - base_name + key ))); } - visited.push(base_name.clone()); + visited.push(key); - let resolved = load_base_profile_raw(base_name, context_dir, source_file)?; + let resolved = match base { + ExtendsRef::Path(path) => { + let (profile, source_path) = parse_file_backed_profile(path)?; + ResolvedBase::Sibling(profile, source_path) + } + ExtendsRef::Name(name) | ExtendsRef::Registry(name) => { + load_base_profile_raw(name, context_dir, source_file)? + } + }; let resolved_base = match resolved { - ResolvedBase::Sibling(base, source_path) => resolve_extends( - base, - visited, - depth + 1, - source_path.parent(), - Some(&source_path), - )?, - ResolvedBase::Global(base) => resolve_extends(base, visited, depth + 1, None, None)?, + ResolvedBase::Sibling(mut base, source_path) => { + base.source_files + .push(ProfileSourceFile::new(source_path.clone())); + resolve_extends( + base, + visited, + depth + 1, + source_path.parent(), + Some(&source_path), + &[], + )? + } + // Pack-store bases keep their file so path entries in them hit the + // pack-profile rule; name lookup still gets no sibling context. + ResolvedBase::Global(mut base, pack_path) => { + if let Some(path) = &pack_path { + // A non-canonical path could miss the pack-store check and + // be classed as writable, so a canonicalize failure errors. + let canonical = + std::fs::canonicalize(path).map_err(|e| NonoError::ProfileRead { + path: path.clone(), + source: e, + })?; + base.source_files.push(ProfileSourceFile::new(canonical)); + } + resolve_extends(base, visited, depth + 1, None, pack_path.as_deref(), &[])? + } }; // Pop to restore the stack to the pre-base state. On the error path // above (? propagation), visited is abandoned so the missing pop is harmless. @@ -3653,10 +3814,11 @@ fn resolve_extends( /// Distinguishes where a base profile was resolved from so `resolve_extends` /// can propagate the canonical source directory only for sibling-resolved /// profiles. Global sources clear the context to prevent project-local files -/// from hijacking built-in inheritance chains. +/// from hijacking built-in inheritance chains. A pack-store base carries its +/// profile path; a built-in carries `None`. enum ResolvedBase { Sibling(Profile, PathBuf), - Global(Profile), + Global(Profile, Option), } /// Load a base profile by name WITHOUT applying implicit default-group merging. @@ -3683,13 +3845,6 @@ fn load_base_profile_raw( context_dir: Option<&Path>, source_file: Option<&Path>, ) -> Result { - if !is_valid_profile_name(name) && !is_registry_ref(name) { - return Err(NonoError::ProfileInheritance(format!( - "invalid base profile name '{}'", - name - ))); - } - // 0. Sibling in the same directory as the child profile. // Skip if the sibling path is the source file itself to avoid // self-references (e.g. `.nono/codex.json` extending "codex"). @@ -3726,13 +3881,13 @@ fn load_base_profile_raw( if !base.packs.contains(&pack_key) { base.packs.push(pack_key.clone()); } - return Ok(ResolvedBase::Global(base)); + return Ok(ResolvedBase::Global(base, Some(profile_path))); } // 3. Built-in profile from embedded policy. let policy = crate::policy::load_embedded_policy()?; if let Some(def) = policy.profiles.get(name) { - return Ok(ResolvedBase::Global(def.to_raw_profile())); + return Ok(ResolvedBase::Global(def.to_raw_profile(), None)); } // 4. Pack-provided rescue: when we were entered through @@ -3755,7 +3910,7 @@ fn load_base_profile_raw( base.packs.push(pack_key.clone()); } resolve_store_pack_session_hooks(&mut base, &pack_key)?; - return Ok(ResolvedBase::Global(base)); + return Ok(ResolvedBase::Global(base, Some(profile_path))); } } crate::migration::MigrationOutcome::Skipped => { @@ -4107,6 +4262,7 @@ fn merge_profiles(base: Profile, child: Profile) -> Profile { &base.unsafe_macos_seatbelt_rules, &child.unsafe_macos_seatbelt_rules, ), + source_files: dedup_append(&base.source_files, &child.source_files), } } @@ -4223,19 +4379,28 @@ pub(crate) fn user_profile_draft_dir() -> Result { /// True when `path` is inside the user profile-drafts directory. /// -/// `path` must already be canonical. The only caller, `load_from_file`, -/// passes the path returned by `parse_file_backed_profile`. +/// `path` must already be canonical. /// Uses [`Path::starts_with`] (not string prefix matching) so lookalike /// paths such as `profile-drafts-evil/` cannot match. pub(crate) fn is_under_user_profile_draft_dir(path: &Path) -> bool { - let Ok(drafts) = user_profile_draft_dir() else { + is_under_dir(path, user_profile_draft_dir()) +} + +/// True when canonical `path` is inside the user profile directory. +fn is_under_user_profile_dir(path: &Path) -> bool { + is_under_dir(path, user_profile_dir()) +} + +/// True when canonical `path` is inside `dir`, compared canonically. +fn is_under_dir(path: &Path, dir: Result) -> bool { + let Ok(dir) = dir else { return false; }; - let Ok(drafts_canon) = drafts.canonicalize() else { - // Drafts dir missing or unreadable → nothing can be under it. + let Ok(dir_canon) = dir.canonicalize() else { + // Directory missing or unreadable → nothing can be under it. return false; }; - path.starts_with(&drafts_canon) + path.starts_with(&dir_canon) } pub(crate) fn get_user_profile_draft_path(name: &str) -> Result { @@ -5190,6 +5355,108 @@ mod tests { Ok(()) } + /// Writes `{ "filesystem": { "read": [read] } }` to `path`, returning its + /// canonical form. + fn write_read_only_profile(path: &Path, read: &str) -> PathBuf { + std::fs::write( + path, + format!( + r#"{{ "meta": {{ "name": "extra" }}, "filesystem": {{ "read": ["{read}"] }} }}"# + ), + ) + .expect("write profile"); + path.canonicalize().expect("canonicalize profile") + } + + #[test] + fn test_cli_path_extends_with_pack_profile() { + let read = crate::test_env::with_isolated_config_home(|config_home| { + crate::test_env::write_fake_pack( + config_home, + "acme", + "packy", + "packy", + r#"{ "meta": { "name": "packy" }, "filesystem": { "read": ["/tmp/packy"] } }"#, + &[], + None, + ); + let extra = write_read_only_profile(&config_home.join("extra.json"), "/tmp/extra"); + load_profile_with_cli_bases("acme/packy", &[ExtendsRef::Path(extra)]) + .expect("pack profile with CLI path base") + .filesystem + .read + }); + assert!(read.contains(&"/tmp/extra".to_string()), "got {read:?}"); + assert!(read.contains(&"/tmp/packy".to_string()), "got {read:?}"); + } + + #[test] + fn test_cli_path_extends_with_builtin_profile() { + let read = crate::test_env::with_isolated_config_home(|config_home| { + let extra = write_read_only_profile(&config_home.join("extra.json"), "/tmp/extra"); + load_profile_with_cli_bases("default", &[ExtendsRef::Path(extra)]) + .expect("built-in profile with CLI path base") + .filesystem + .read + }); + assert!(read.contains(&"/tmp/extra".to_string()), "got {read:?}"); + } + + #[test] + fn test_cli_path_resolves_against_cwd_not_profile_dir() { + let read = crate::test_env::with_isolated_config_home(|config_home| { + let a = config_home.join("a"); + let b = config_home.join("b"); + std::fs::create_dir_all(&a).expect("mkdir a"); + std::fs::create_dir_all(&b).expect("mkdir b"); + write_read_only_profile(&a.join("extra.json"), "/tmp/from-a"); + write_read_only_profile(&b.join("extra.json"), "/tmp/from-b"); + let agent = a.join("agent.json"); + std::fs::write(&agent, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + + let cli = classify_extends_entry("./extra.json", ExtendsOrigin::Cli(&b)) + .expect("classify CLI path"); + load_profile_with_cli_bases(agent.to_str().expect("utf-8 path"), &[cli]) + .expect("load with CLI path base") + .filesystem + .read + }); + assert!(read.contains(&"/tmp/from-b".to_string()), "got {read:?}"); + assert!(!read.contains(&"/tmp/from-a".to_string()), "got {read:?}"); + } + + #[test] + fn test_load_profile_with_extends_resolves_cli_path_against_cwd() { + let read = crate::test_env::with_isolated_config_home(|config_home| { + let project = config_home.join("proj"); + let cwd = config_home.join("cwd"); + std::fs::create_dir_all(&project).expect("mkdir proj"); + std::fs::create_dir_all(&cwd).expect("mkdir cwd"); + write_read_only_profile(&project.join("x.json"), "/tmp/decoy"); + write_read_only_profile(&cwd.join("x.json"), "/tmp/from-cwd"); + let agent = project.join("agent.json"); + std::fs::write(&agent, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + + // The cwd is process-global; the config-home helper holds ENV_LOCK, + // and catch_unwind guarantees the restore even if loading panics. + let original = std::env::current_dir().expect("cwd"); + std::env::set_current_dir(&cwd).expect("chdir"); + let result = std::panic::catch_unwind(|| { + load_profile_with_extends( + agent.to_str().expect("utf-8 path"), + &["./x.json".to_string()], + ) + }); + std::env::set_current_dir(original).expect("restore cwd"); + match result { + Ok(loaded) => loaded.expect("load with CLI path base").filesystem.read, + Err(panic) => std::panic::resume_unwind(panic), + } + }); + assert!(read.contains(&"/tmp/from-cwd".to_string()), "got {read:?}"); + assert!(!read.contains(&"/tmp/decoy".to_string()), "got {read:?}"); + } + #[test] fn test_load_profile_with_extends_deduplicates_duplicate_base() -> std::result::Result<(), Box> { @@ -7080,6 +7347,7 @@ mod tests { command_args: vec![], unsafe_macos_seatbelt_rules: vec![], platform_overrides: None, + source_files: Vec::new(), } } @@ -7171,6 +7439,7 @@ mod tests { command_args: vec![], unsafe_macos_seatbelt_rules: vec![], platform_overrides: None, + source_files: Vec::new(), } } @@ -7779,6 +8048,143 @@ mod tests { Ok(()) } + #[test] + fn test_extends_relative_path_sibling_dir() + -> std::result::Result<(), Box> { + let dir = tempdir()?; + let nono_dir = dir.path().join("proj/.nono"); + let shared_dir = dir.path().join("proj/shared"); + std::fs::create_dir_all(&nono_dir)?; + std::fs::create_dir_all(&shared_dir)?; + std::fs::write( + shared_dir.join("base.json"), + r#"{ "meta": { "name": "base" }, "filesystem": { "read": ["/base"] } }"#, + )?; + std::fs::write( + nono_dir.join("agent.json"), + r#"{ "extends": "../shared/base.json", "meta": { "name": "agent" }, + "filesystem": { "read": ["/child"] } }"#, + )?; + + let profile = load_from_file(&nono_dir.join("agent.json"), &[])?; + + assert_eq!(profile.filesystem.read, vec!["/base", "/child"]); + Ok(()) + } + + #[test] + fn test_extends_relative_path_base_finds_own_sibling() + -> std::result::Result<(), Box> { + let dir = tempdir()?; + let child_dir = dir.path().join("child"); + let shared_dir = dir.path().join("shared"); + std::fs::create_dir_all(&child_dir)?; + std::fs::create_dir_all(&shared_dir)?; + std::fs::write( + shared_dir.join("base.json"), + r#"{ "extends": "common", "meta": { "name": "base" } }"#, + )?; + std::fs::write( + shared_dir.join("common.json"), + r#"{ "meta": { "name": "common" }, "filesystem": { "read": ["/shared-common"] } }"#, + )?; + std::fs::write( + child_dir.join("common.json"), + r#"{ "meta": { "name": "decoy" }, "filesystem": { "read": ["/decoy"] } }"#, + )?; + std::fs::write( + child_dir.join("agent.json"), + r#"{ "extends": "../shared/base.json", "meta": { "name": "agent" } }"#, + )?; + + let profile = load_from_file(&child_dir.join("agent.json"), &[])?; + + assert_eq!(profile.filesystem.read, vec!["/shared-common"]); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn test_extends_relative_path_symlink_uses_target_dir() + -> std::result::Result<(), Box> { + let dir = tempdir()?; + let link_dir = dir.path().join("d"); + let target_dir = dir.path().join("e"); + std::fs::create_dir_all(&link_dir)?; + std::fs::create_dir_all(&target_dir)?; + std::fs::write( + target_dir.join("real.json"), + r#"{ "extends": "sib", "meta": { "name": "real" } }"#, + )?; + std::fs::write( + target_dir.join("sib.json"), + r#"{ "meta": { "name": "sib" }, "filesystem": { "read": ["/target-sib"] } }"#, + )?; + std::fs::write( + link_dir.join("sib.json"), + r#"{ "meta": { "name": "decoy" }, "filesystem": { "read": ["/link-sib"] } }"#, + )?; + std::fs::write( + link_dir.join("child.json"), + r#"{ "extends": "./link.json", "meta": { "name": "child" } }"#, + )?; + std::os::unix::fs::symlink(target_dir.join("real.json"), link_dir.join("link.json"))?; + + let profile = load_from_file(&link_dir.join("child.json"), &[])?; + + assert_eq!(profile.filesystem.read, vec!["/target-sib"]); + Ok(()) + } + + #[test] + fn test_extends_relative_path_cycle_detected() { + let dir = tempdir().expect("tmpdir"); + std::fs::write( + dir.path().join("a.json"), + r#"{ "extends": "./b.json", "meta": { "name": "a" } }"#, + ) + .expect("write a"); + std::fs::write( + dir.path().join("b.json"), + r#"{ "extends": "./a.json", "meta": { "name": "b" } }"#, + ) + .expect("write b"); + + let err = load_from_file(&dir.path().join("a.json"), &[]).expect_err("cycle must error"); + + assert!( + err.to_string().contains("circular dependency"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_extends_same_file_two_spellings_is_not_cycle() + -> std::result::Result<(), Box> { + let dir = tempdir()?; + std::fs::create_dir_all(dir.path().join("x"))?; + std::fs::create_dir_all(dir.path().join("y"))?; + std::fs::write( + dir.path().join("x/a.json"), + r#"{ "meta": { "name": "a" }, "filesystem": { "read": ["/a"] } }"#, + )?; + std::fs::write( + dir.path().join("y/b.json"), + r#"{ "extends": "../x/a.json", "meta": { "name": "b" }, + "filesystem": { "read": ["/b"] } }"#, + )?; + std::fs::write( + dir.path().join("child.json"), + r#"{ "extends": ["./x/a.json", "./y/b.json"], "meta": { "name": "child" } }"#, + )?; + + let profile = load_from_file(&dir.path().join("child.json"), &[])?; + + assert!(profile.filesystem.read.contains(&"/a".to_string())); + assert!(profile.filesystem.read.contains(&"/b".to_string())); + Ok(()) + } + #[test] fn test_extends_user_profile() { // Test user-to-user file-based inheritance by parsing two temp files @@ -7858,7 +8264,7 @@ mod tests { // Resolve B first let resolved_b = - resolve_extends(b_profile, &mut Vec::new(), 0, None, None).expect("resolve b"); + resolve_extends(b_profile, &mut Vec::new(), 0, None, None, &[]).expect("resolve b"); // Then merge A on top let merged = merge_profiles(resolved_b, a_profile); @@ -7874,7 +8280,7 @@ mod tests { ..Default::default() }; - let result = resolve_extends(profile, &mut Vec::new(), 0, None, None); + let result = resolve_extends(profile, &mut Vec::new(), 0, None, None, &[]); assert!(result.is_err()); let err = result.expect_err("missing base should error"); assert!( @@ -7893,7 +8299,7 @@ mod tests { }; let mut visited = vec!["a".to_string(), "b".to_string()]; - let result = resolve_extends(profile, &mut visited, 2, None, None); + let result = resolve_extends(profile, &mut visited, 2, None, None, &[]); assert!(result.is_err()); let err = result.expect_err("circular dep should error"); assert!( @@ -7911,7 +8317,7 @@ mod tests { }; let mut visited = vec!["self-ref".to_string()]; - let result = resolve_extends(profile, &mut visited, 1, None, None); + let result = resolve_extends(profile, &mut visited, 1, None, None, &[]); assert!(result.is_err()); let err = result.expect_err("self-reference should error"); assert!( @@ -7937,6 +8343,7 @@ mod tests { MAX_INHERITANCE_DEPTH, None, None, + &[], ); assert!(result.is_err()); let err = result.expect_err("depth limit should error"); @@ -8584,13 +8991,13 @@ mod tests { #[test] fn test_extends_empty_string_in_array_rejected() { - // An empty string passes deserialization but is caught by load_base_profile_raw + // An empty string passes deserialization but is caught by classify_extends_entry let profile = Profile { extends: Some(vec!["".to_string()]), ..Default::default() }; - let result = resolve_extends(profile, &mut Vec::new(), 0, None, None); + let result = resolve_extends(profile, &mut Vec::new(), 0, None, None, &[]); assert!(result.is_err()); let err = result.expect_err("empty string base should error"); assert!( @@ -8719,7 +9126,7 @@ mod tests { ..Default::default() }; - let result = resolve_extends(profile, &mut Vec::new(), 0, None, None); + let result = resolve_extends(profile, &mut Vec::new(), 0, None, None, &[]); assert!( result.is_ok(), "duplicate base should be deduplicated, not error: {:?}", @@ -10795,6 +11202,290 @@ mod tests { ) } + #[test] + fn test_extends_path_entry_in_pack_profile_rejected() { + let err = with_config_env(|config_dir| { + build_fake_pack_store( + config_dir, + "acme", + "pathy", + "pathy", + r#"{ "extends": "./other.json", "meta": { "name": "pathy" } }"#, + None, + ); + load_profile("acme/pathy").expect_err("path entry in pack profile must error") + }); + + assert!( + err.to_string() + .contains("path entries in `extends` are not allowed in pack profiles"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_extends_path_entry_in_pack_base_rejected() { + let err = with_config_env(|config_dir| { + build_fake_pack_store( + config_dir, + "acme", + "pathy", + "pathy", + r#"{ "extends": "./other.json", "meta": { "name": "pathy" } }"#, + None, + ); + let dir = tempdir().expect("tmpdir"); + let child = dir.path().join("child.json"); + std::fs::write( + &child, + r#"{ "extends": "pathy", "meta": { "name": "child" } }"#, + ) + .expect("write child"); + load_from_file(&child, &[]).expect_err("path entry in pack base must error") + }); + + assert!( + err.to_string() + .contains("path entries in `extends` are not allowed in pack profiles"), + "unexpected error: {err}" + ); + } + + fn source_file(path: &Path, kind: ProfileSourceKind) -> ProfileSourceFile { + ProfileSourceFile { + path: path.canonicalize().expect("canonicalize source file"), + kind, + top_level: false, + } + } + + #[test] + fn test_source_files_record_kinds() { + let (profile, expected) = with_config_env(|config_dir| { + let install_dir = build_fake_pack_store( + config_dir, + "acme", + "packy", + "packy", + r#"{ "meta": { "name": "packy" } }"#, + None, + ); + let dir = tempdir().expect("tmpdir"); + std::fs::create_dir_all(dir.path().join("proj")).expect("mkdir proj"); + std::fs::create_dir_all(dir.path().join("shared")).expect("mkdir shared"); + let base = dir.path().join("shared/base.json"); + std::fs::write(&base, r#"{ "meta": { "name": "base" } }"#).expect("write base"); + let agent = dir.path().join("proj/agent.json"); + std::fs::write( + &agent, + r#"{ "extends": ["../shared/base.json", "packy", "default"], + "meta": { "name": "agent" } }"#, + ) + .expect("write agent"); + + let profile = load_profile_from_path(&agent).expect("load agent"); + let expected = vec![ + source_file(&base, ProfileSourceKind::Project), + source_file( + &install_dir.join("profiles/packy.json"), + ProfileSourceKind::Pack, + ), + ProfileSourceFile { + top_level: true, + ..source_file(&agent, ProfileSourceKind::Project) + }, + ]; + (profile, expected) + }); + + assert_eq!(profile.source_files, expected); + assert_eq!( + profile.writable_source_files(), + vec![expected[2].clone(), expected[0].clone()] + ); + } + + #[test] + fn test_writable_source_files_precedence_order() { + let dir = tempdir().expect("tmpdir"); + let write = |name: &str, json: &str| { + let path = dir.path().join(name); + std::fs::write(&path, json).expect("write profile"); + path.canonicalize().expect("canonicalize profile") + }; + let c = write("c.json", r#"{ "meta": { "name": "c" } }"#); + let b = write("b.json", r#"{ "meta": { "name": "b" } }"#); + let a = write( + "a.json", + r#"{ "extends": "./c.json", "meta": { "name": "a" } }"#, + ); + let top = write( + "top.json", + r#"{ "extends": ["./a.json", "./b.json"], "meta": { "name": "top" } }"#, + ); + + let profile = load_from_file(&top, &[]).expect("load top"); + let paths: Vec = profile + .writable_source_files() + .into_iter() + .map(|file| file.path) + .collect(); + + assert_eq!(paths, vec![top.clone(), b, a, c]); + let top_level: Vec = profile + .source_files + .iter() + .filter(|file| file.top_level) + .map(|file| file.path.clone()) + .collect(); + assert_eq!(top_level, vec![top]); + } + + #[test] + fn test_cli_extends_file_under_builtin_is_not_top_level() { + let profile = with_config_env(|config_dir| { + let profiles = config_dir.join("nono/profiles"); + std::fs::create_dir_all(&profiles).expect("mkdir profiles"); + std::fs::write( + profiles.join("extra.json"), + r#"{ "meta": { "name": "extra" } }"#, + ) + .expect("write extra"); + load_profile_with_extends("default", &["extra".to_string()]).expect("load") + }); + + assert_eq!(profile.source_files.len(), 1, "{:?}", profile.source_files); + assert!(!profile.source_files[0].top_level); + } + + #[test] + fn test_source_files_draft_and_user_kinds() { + let (user, draft, expected_user, expected_draft) = with_config_env(|config_dir| { + let profiles = config_dir.join("nono/profiles"); + let drafts = config_dir.join("nono/profile-drafts"); + std::fs::create_dir_all(&profiles).expect("mkdir profiles"); + std::fs::create_dir_all(&drafts).expect("mkdir drafts"); + let user_path = profiles.join("mine.json"); + std::fs::write(&user_path, r#"{ "meta": { "name": "mine" } }"#).expect("write user"); + let draft_path = drafts.join("draft.json"); + std::fs::write(&draft_path, r#"{ "meta": { "name": "draft" } }"#).expect("write draft"); + + let user = load_profile_no_migrate("mine").expect("load user profile"); + let draft = load_profile_from_path(&draft_path).expect("load draft"); + ( + user, + draft, + ProfileSourceFile { + top_level: true, + ..source_file(&user_path, ProfileSourceKind::User) + }, + ProfileSourceFile { + top_level: true, + ..source_file(&draft_path, ProfileSourceKind::Draft) + }, + ) + }); + + assert_eq!(user.source_files, vec![expected_user]); + assert_eq!(draft.source_files, vec![expected_draft]); + } + + #[test] + fn test_top_level_pack_profile_is_recorded_and_not_writable() { + let (profile, pack_file) = with_config_env(|config_dir| { + let install_dir = build_fake_pack_store( + config_dir, + "acme", + "packy", + "packy", + r#"{ "meta": { "name": "packy" } }"#, + None, + ); + let profile = load_profile("packy").expect("load pack profile"); + let pack_file = install_dir + .join("profiles/packy.json") + .canonicalize() + .expect("canonicalize pack file"); + (profile, pack_file) + }); + + let pack_entries: Vec<&ProfileSourceFile> = profile + .source_files + .iter() + .filter(|file| file.kind == ProfileSourceKind::Pack) + .collect(); + assert_eq!(pack_entries.len(), 1, "{:?}", profile.source_files); + assert_eq!(pack_entries[0].path, pack_file); + assert!( + profile.writable_source_files().is_empty(), + "{:?}", + profile.source_files + ); + } + + #[test] + fn test_jsonc_user_profile_is_top_level_user_source() { + let (profile, expected) = with_config_env(|config_dir| { + let profiles = config_dir.join("nono/profiles"); + std::fs::create_dir_all(&profiles).expect("mkdir profiles"); + let path = profiles.join("mine.jsonc"); + std::fs::write( + &path, + "{\n // comment\n \"meta\": { \"name\": \"mine\" },\n}\n", + ) + .expect("write jsonc profile"); + let profile = load_profile_no_migrate("mine").expect("load jsonc user profile"); + ( + profile, + ProfileSourceFile { + top_level: true, + ..source_file(&path, ProfileSourceKind::User) + }, + ) + }); + + assert_eq!(profile.source_files, vec![expected]); + } + + #[test] + fn test_load_profile_extends_resolved_drops_unclassifiable_entries() { + let (resolved, base) = with_config_env(|_| { + let dir = tempdir().expect("tmpdir"); + let base = dir.path().join("base.json"); + std::fs::write(&base, r#"{ "meta": { "name": "base" } }"#).expect("write base"); + let child = dir.path().join("child.json"); + std::fs::write( + &child, + r#"{ "extends": ["/abs.json", "./base.json", "default", "~/x.json"], + "meta": { "name": "child" } }"#, + ) + .expect("write child"); + let resolved = load_profile_extends_resolved(child.to_str().expect("utf-8 path")); + (resolved, base.canonicalize().expect("canonicalize base")) + }); + + assert_eq!( + resolved, + Some(vec![base.display().to_string(), "default".to_string()]) + ); + } + + #[test] + fn test_source_files_not_serialized() { + let profile = Profile { + source_files: vec![ProfileSourceFile { + path: PathBuf::from("/tmp/agent.json"), + kind: ProfileSourceKind::Project, + top_level: true, + }], + ..Default::default() + }; + + let value = serde_json::to_value(&profile).expect("serialize profile"); + + assert!(value.get("source_files").is_none(), "got {value}"); + } + /// Test 1: A hook script starting with `$PACK_DIR` in a registry-pack profile /// is expanded to the full path `/`. #[test] diff --git a/crates/nono-cli/src/profile_cmd.rs b/crates/nono-cli/src/profile_cmd.rs index a49034624..2af53846b 100644 --- a/crates/nono-cli/src/profile_cmd.rs +++ b/crates/nono-cli/src/profile_cmd.rs @@ -125,12 +125,16 @@ fn cmd_init(args: ProfileInitArgs) -> Result<()> { // Validate --extends target exists in any of the three sources the // resolver knows about (user dir, pack store, built-in). - if let Some(ref base) = args.extends - && !profile_exists(base) - { - return Err(NonoError::ProfileParse(extends_target_not_found_message( - base, - ))); + if let Some(ref base) = args.extends { + // The entry is written as given and later resolved against the file + // that holds it, so validate it the same way. + let entry = + profile::classify_extends_entry(base, profile::ExtendsOrigin::File(&output_path))?; + if !matches!(entry, profile::ExtendsRef::Path(_)) && !profile_exists(base) { + return Err(NonoError::ProfileParse(extends_target_not_found_message( + base, + ))); + } } // Validate --groups against embedded policy @@ -2608,7 +2612,7 @@ pub(crate) fn cmd_promote(args: ProfilePromoteArgs) -> Result<()> { raw_profile.meta.name, args.name ))); } - let resolved_profile = profile::resolve_and_finalize_profile(raw_profile)?; + let resolved_profile = profile::resolve_and_finalize_draft_profile(raw_profile, &draft_path)?; validate_promote_profile(&resolved_profile)?; let target_exists = regular_file_exists(&target_path, "target profile")?; @@ -3495,6 +3499,65 @@ mod tests { assert!(err.to_string().contains("not found")); } + fn init_with_extends(xdg: &std::path::Path, out: &std::path::Path, base: &str) -> Result<()> { + let xdg_str = xdg.to_str().expect("utf8 xdg"); + let _env = crate::test_env::EnvVarGuard::set_all(&[("XDG_CONFIG_HOME", xdg_str)]); + cmd_init(ProfileInitArgs { + name: "new".to_string(), + extends: Some(base.to_string()), + groups: vec![], + description: None, + full: false, + output: Some(out.to_path_buf()), + force: false, + }) + } + + #[test] + fn test_init_extends_relative_path_validates_against_output_dir() { + let _guard = match crate::test_env::ENV_LOCK.lock() { + Ok(guard) => guard, + Err(poisoned) => poisoned.into_inner(), + }; + let dir = tempfile::tempdir().expect("tempdir"); + let xdg = dir.path().join("config"); + std::fs::create_dir_all(&xdg).expect("create xdg"); + let proj = dir.path().join("proj"); + std::fs::create_dir_all(proj.join(".nono")).expect("create .nono"); + let out = proj.join(".nono/new.json"); + + let err = init_with_extends(&xdg, &out, "../base.json").expect_err("base is absent"); + assert!(err.to_string().contains("cannot be read"), "got: {err}"); + assert!(!out.exists()); + + std::fs::write(proj.join("base.json"), r#"{ "meta": { "name": "base" } }"#) + .expect("write base"); + init_with_extends(&xdg, &out, "../base.json").expect("base present"); + let written: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&out).expect("read")).expect("json"); + assert_eq!(written["extends"], "../base.json"); + } + + #[test] + fn test_init_extends_absolute_path_reports_unsupported() { + let _guard = match crate::test_env::ENV_LOCK.lock() { + Ok(guard) => guard, + Err(poisoned) => poisoned.into_inner(), + }; + let dir = tempfile::tempdir().expect("tempdir"); + let xdg = dir.path().join("config"); + std::fs::create_dir_all(&xdg).expect("create xdg"); + let out = dir.path().join("new.json"); + + let err = init_with_extends(&xdg, &out, "/abs.json").expect_err("absolute path"); + assert!( + err.to_string() + .contains("absolute and `~/` paths are not supported in `extends`"), + "got: {err}" + ); + assert!(!out.exists()); + } + #[test] fn test_init_blocked_when_shadowing_builtin() { let _guard = match crate::test_env::ENV_LOCK.lock() { @@ -4186,6 +4249,40 @@ mod tests { ); } + #[test] + fn promote_rejects_path_extends_in_draft() { + let _guard = match crate::test_env::ENV_LOCK.lock() { + Ok(guard) => guard, + Err(poisoned) => poisoned.into_inner(), + }; + let dir = tempfile::tempdir().expect("tempdir"); + let xdg = dir.path().join("config"); + std::fs::create_dir_all(&xdg).expect("create xdg"); + let xdg_str = xdg.to_str().expect("utf8 xdg"); + let _env = crate::test_env::EnvVarGuard::set_all(&[("XDG_CONFIG_HOME", xdg_str)]); + + let draft_dir = profile::user_profile_draft_dir().expect("draft dir"); + std::fs::create_dir_all(&draft_dir).expect("create drafts"); + std::fs::write(draft_dir.join("x.json"), r#"{ "meta": { "name": "x" } }"#) + .expect("write base"); + let draft_path = profile::get_user_profile_draft_path("agent-local").expect("draft path"); + std::fs::write( + &draft_path, + r#"{ "meta": { "name": "agent-local" }, "extends": "./x.json" }"#, + ) + .expect("write draft"); + + let err = cmd_promote(ProfilePromoteArgs { + name: "agent-local".to_string(), + diff: false, + yes: true, + help: None, + }) + .expect_err("draft path extends must be rejected"); + assert!(err.to_string().contains("profile drafts"), "got: {err}"); + assert!(draft_path.exists()); + } + #[test] fn promote_existing_profile_infers_missing_base_hash() { let _guard = match crate::test_env::ENV_LOCK.lock() { diff --git a/crates/nono-cli/src/profile_file_edit.rs b/crates/nono-cli/src/profile_file_edit.rs new file mode 100644 index 000000000..a5d8167d7 --- /dev/null +++ b/crates/nono-cli/src/profile_file_edit.rs @@ -0,0 +1,394 @@ +//! Comment-preserving edits to profile files on disk. +//! +//! The save prompt updates an existing profile by appending grants. Editing +//! the concrete syntax tree keeps the user's comments and formatting, which a +//! `serde_json` round trip would discard. + +use crate::profile; +use jsonc_parser::ParseOptions; +use jsonc_parser::cst::{CstArray, CstInputValue, CstObject, CstRootNode}; +use nono::{NonoError, Result}; +use serde::Deserialize; +use std::collections::HashSet; + +type PatchValues = fn(&profile::Profile) -> &[String]; + +/// The string lists `merge_profile_patch` dedup-appends, as (parent object +/// key or `None` for the root, array key, value key of a conditional +/// `{ "": ..., "when": ... }` entry or `None` if the list has none, +/// patch accessor). `open_urls.allow_localhost` is a bool and is handled +/// separately. +const PATCHED_LISTS: &[(Option<&str>, &str, Option<&str>, PatchValues)] = &[ + (Some("filesystem"), "allow", Some("path"), |p| { + &p.filesystem.allow + }), + (Some("filesystem"), "read", Some("path"), |p| { + &p.filesystem.read + }), + (Some("filesystem"), "write", Some("path"), |p| { + &p.filesystem.write + }), + (Some("filesystem"), "allow_file", Some("path"), |p| { + &p.filesystem.allow_file + }), + (Some("filesystem"), "read_file", Some("path"), |p| { + &p.filesystem.read_file + }), + (Some("filesystem"), "write_file", Some("path"), |p| { + &p.filesystem.write_file + }), + (Some("filesystem"), "bypass_protection", Some("path"), |p| { + &p.filesystem.bypass_protection + }), + ( + Some("filesystem"), + "suppress_save_prompt", + Some("path"), + |p| &p.filesystem.suppress_save_prompt, + ), + (None, "unsafe_macos_seatbelt_rules", None, |p| { + &p.unsafe_macos_seatbelt_rules + }), + (Some("open_urls"), "allow_origins", Some("origin"), |p| { + p.open_urls.as_ref().map_or(&[], |urls| &urls.allow_origins) + }), +]; + +/// Append the grants in `patch` to the profile source `text`, keeping +/// comments and formatting. Mirrors `merge_profile_patch`: list values not +/// already present are appended, and `allow_localhost` only flips to `true`. +pub(crate) fn apply_patch_to_profile_text(text: &str, patch: &profile::Profile) -> Result { + let root = CstRootNode::parse(text, &ParseOptions::default()) + .map_err(|e| NonoError::LearnError(format!("Failed to parse profile: {e}")))?; + let root_object = root + .object_value_or_create() + .ok_or_else(|| NonoError::LearnError("Profile root is not an object".to_string()))?; + + for (section, key, conditional_key, patch_values) in PATCHED_LISTS { + let values = patch_values(patch); + if values.is_empty() { + continue; + } + let field = section.map_or(key.to_string(), |s| format!("{s}.{key}")); + let parent = match section { + Some(section) => section_object(&root_object, section)?, + None => root_object.clone(), + }; + let array = parent.array_value_or_create(key).ok_or_else(|| { + NonoError::LearnError(format!("Profile field `{field}` is not an array")) + })?; + let mut present = effective_entries(&array, *conditional_key, &field)?; + for value in values { + if present.insert(value.clone()) { + array.append(value.as_str().into()); + } + } + } + + if patch + .open_urls + .as_ref() + .is_some_and(|urls| urls.allow_localhost) + { + let open_urls = section_object(&root_object, "open_urls")?; + match open_urls.get("allow_localhost") { + Some(prop) => prop.set_value(CstInputValue::Bool(true)), + None => { + open_urls.append("allow_localhost", CstInputValue::Bool(true)); + } + } + } + + Ok(root.to_string()) +} + +/// The entries of `array` as profile loading sees them on this platform: +/// a conditional entry counts by its value key when its `when` matches and +/// is skipped otherwise, so the edit agrees with `merge_profile_patch`. +fn effective_entries( + array: &CstArray, + conditional_key: Option<&'static str>, + field: &str, +) -> Result> { + let invalid = |e: String| NonoError::LearnError(format!("Profile field `{field}`: {e}")); + let value: serde_json::Value = crate::jsonc::parse(&array.to_string()).map_err(invalid)?; + let entries = match conditional_key { + Some(key) => profile::deserialize_conditional_string_vec(value, key), + None => Vec::::deserialize(value), + } + .map_err(|e| invalid(e.to_string()))?; + Ok(entries.into_iter().collect()) +} + +/// Sections whose profile field is an `Option`, so `null` loads as absent. +/// `"filesystem": null` does not load and stays an error. +const NULLABLE_SECTIONS: &[&str] = &["open_urls"]; + +/// The object at `section`, created when the key is missing, or `null` in a +/// nullable section. +fn section_object(root: &CstObject, section: &str) -> Result { + if NULLABLE_SECTIONS.contains(§ion) + && let Some(prop) = root.get(section) + && prop + .value() + .is_some_and(|value| value.as_null_keyword().is_some()) + { + prop.set_value(CstInputValue::Object(Vec::new())); + } + root.object_value_or_create(section) + .ok_or_else(|| NonoError::LearnError(format!("Profile field `{section}` is not an object"))) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::profile_save_runtime::merge_profile_patch; + + fn parse(text: &str) -> profile::Profile { + profile::parse_profile_bytes(text.as_bytes()).expect("parse profile") + } + + fn patch_with_read(paths: &[&str]) -> profile::Profile { + let mut patch = profile::Profile::default(); + patch.filesystem.read = paths.iter().map(|p| p.to_string()).collect(); + patch + } + + fn localhost_patch(allow_localhost: bool) -> profile::Profile { + profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: Vec::new(), + allow_localhost, + }), + ..Default::default() + } + } + + #[test] + fn keeps_comments_and_appends_new_entries() { + let input = r#"{ + // keep me + "meta": { "name": "x" }, + "filesystem": { + // keep me too + "read": ["/a"] + } +} +"#; + let output = apply_patch_to_profile_text(input, &patch_with_read(&["/a", "/b"])) + .expect("apply patch"); + + assert!(output.contains("// keep me\n"), "output: {output}"); + assert!(output.contains("// keep me too"), "output: {output}"); + assert_eq!(output.matches("\"/a\"").count(), 1, "output: {output}"); + assert_eq!(output.matches("\"/b\"").count(), 1, "output: {output}"); + assert_eq!(parse(&output).filesystem.read, vec!["/a", "/b"]); + } + + #[test] + fn creates_missing_sections() { + let input = r#"{ "meta": { "name": "x" } }"#; + let mut patch = profile::Profile::default(); + patch.filesystem.read_file = vec!["/f".to_string()]; + patch.open_urls = Some(profile::OpenUrlConfig { + allow_origins: vec!["https://example.com".to_string()], + allow_localhost: false, + }); + + let output = apply_patch_to_profile_text(input, &patch).expect("apply patch"); + let parsed = parse(&output); + + assert_eq!(parsed.filesystem.read_file, vec!["/f"]); + assert_eq!( + parsed.open_urls.expect("open_urls").allow_origins, + vec!["https://example.com"] + ); + } + + #[test] + fn appends_to_empty_inline_array() { + let input = r#"{ "meta": { "name": "x" }, "filesystem": { "read": [] } }"#; + let output = + apply_patch_to_profile_text(input, &patch_with_read(&["/a"])).expect("apply patch"); + + assert_eq!(parse(&output).filesystem.read, vec!["/a"]); + } + + #[test] + fn allow_localhost_only_flips_to_true() { + let input_false = + r#"{ "meta": { "name": "x" }, "open_urls": { "allow_localhost": false } }"#; + let output = + apply_patch_to_profile_text(input_false, &localhost_patch(true)).expect("apply patch"); + assert!(parse(&output).open_urls.expect("open_urls").allow_localhost); + + let input_true = r#"{ "meta": { "name": "x" }, "open_urls": { "allow_localhost": true } }"#; + let output = + apply_patch_to_profile_text(input_true, &localhost_patch(false)).expect("apply patch"); + assert!(parse(&output).open_urls.expect("open_urls").allow_localhost); + } + + #[test] + fn allow_localhost_creates_missing_open_urls_section() { + let input = r#"{ "meta": { "name": "x" } }"#; + let patch = localhost_patch(true); + + let output = apply_patch_to_profile_text(input, &patch).expect("apply patch"); + assert!(parse(&output).open_urls.expect("open_urls").allow_localhost); + + let mut expected = parse(input); + merge_profile_patch(&mut expected, &patch); + assert_eq!( + serde_json::to_value(parse(&output)).expect("serialize output"), + serde_json::to_value(expected).expect("serialize expected"), + "output: {output}" + ); + } + + #[test] + fn matches_merge_profile_patch() { + let input = r#"{ + // comment + "meta": { "name": "x" }, + "filesystem": { + "allow": ["/allow-old"], + "read": ["/read-old"], + "bypass_protection": ["/bp-old"] + }, + "unsafe_macos_seatbelt_rules": ["(allow old)"], + "open_urls": { "allow_origins": ["https://old.example.com"] } +} +"#; + + let mut filesystem_lists = profile::Profile::default(); + filesystem_lists.filesystem.allow = + vec!["/allow-old".to_string(), "/allow-new".to_string()]; + filesystem_lists.filesystem.read = vec!["/read-new".to_string()]; + filesystem_lists.filesystem.write = vec!["/write-new".to_string()]; + filesystem_lists.filesystem.allow_file = vec!["/af".to_string()]; + filesystem_lists.filesystem.read_file = vec!["/rf".to_string()]; + filesystem_lists.filesystem.write_file = vec!["/wf".to_string()]; + filesystem_lists.filesystem.suppress_save_prompt = vec!["/quiet".to_string()]; + + let mut bypass = profile::Profile::default(); + bypass.filesystem.bypass_protection = vec!["/bp-old".to_string(), "/bp-new".to_string()]; + + let seatbelt = profile::Profile { + unsafe_macos_seatbelt_rules: vec!["(allow old)".to_string(), "(allow new)".to_string()], + ..Default::default() + }; + + let open_urls = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec![ + "https://old.example.com".to_string(), + "https://new.example.com".to_string(), + ], + allow_localhost: true, + }), + ..Default::default() + }; + + let (current, other) = platforms(); + let conditional_input = format!( + r#"{{ + "meta": {{ "name": "x" }}, + "filesystem": {{ + "read": [{{ "path": "/read-new", "when": "{current}" }}], + "write": [{{ "path": "/write-new", "when": "{other}" }}] + }}, + "open_urls": {{ + "allow_origins": [{{ "origin": "https://new.example.com", "when": "{current}" }}] + }} +}} +"# + ); + + let patches = [filesystem_lists, bypass, seatbelt, open_urls]; + for input in [input, conditional_input.as_str()] { + for patch in &patches { + let output = apply_patch_to_profile_text(input, patch).expect("apply patch"); + let mut expected = parse(input); + merge_profile_patch(&mut expected, patch); + + assert_eq!( + serde_json::to_value(parse(&output)).expect("serialize output"), + serde_json::to_value(expected).expect("serialize expected"), + "output: {output}" + ); + } + } + } + + fn platforms() -> (&'static str, &'static str) { + let current = crate::platform::current_os_name(); + let other = if current == "linux" { "macos" } else { "linux" }; + (current, other) + } + + #[test] + fn matching_conditional_entry_counts_as_present() { + let (current, _) = platforms(); + let input = format!( + r#"{{ + "meta": {{ "name": "x" }}, + "filesystem": {{ "read": [{{ "path": "/a", "when": "{current}" }}] }}, + "open_urls": {{ + "allow_origins": [{{ "origin": "https://a.example.com", "when": "{current}" }}] + }} +}} +"# + ); + let mut patch = patch_with_read(&["/a"]); + patch.open_urls = Some(profile::OpenUrlConfig { + allow_origins: vec!["https://a.example.com".to_string()], + allow_localhost: false, + }); + + let output = apply_patch_to_profile_text(&input, &patch).expect("apply patch"); + + assert_eq!(output, input); + } + + #[test] + fn null_open_urls_section_is_created() { + let input = r#"{ "meta": { "name": "x" }, "open_urls": null }"#; + let patch = profile::Profile { + open_urls: Some(profile::OpenUrlConfig { + allow_origins: vec!["https://example.com".to_string()], + allow_localhost: true, + }), + ..Default::default() + }; + + let output = apply_patch_to_profile_text(input, &patch).expect("apply patch"); + let open_urls = parse(&output).open_urls.expect("open_urls"); + + assert_eq!(open_urls.allow_origins, vec!["https://example.com"]); + assert!(open_urls.allow_localhost); + } + + #[test] + fn null_filesystem_section_errors() { + let input = r#"{ "meta": { "name": "x" }, "filesystem": null }"#; + let result = apply_patch_to_profile_text(input, &patch_with_read(&["/a"])); + + let message = result.expect_err("null filesystem section").to_string(); + assert!( + message.contains("`filesystem` is not an object"), + "message: {message}" + ); + } + + #[test] + fn non_object_section_errors() { + let input = r#"{ "meta": { "name": "x" }, "filesystem": [] }"#; + let result = apply_patch_to_profile_text(input, &patch_with_read(&["/a"])); + + let message = result.expect_err("non-object section").to_string(); + assert!( + message.contains("`filesystem` is not an object"), + "message: {message}" + ); + } +} diff --git a/crates/nono-cli/src/profile_save_runtime.rs b/crates/nono-cli/src/profile_save_runtime.rs index 9b55d2396..507971b68 100644 --- a/crates/nono-cli/src/profile_save_runtime.rs +++ b/crates/nono-cli/src/profile_save_runtime.rs @@ -22,6 +22,28 @@ pub(crate) struct PreparedProfileSave { pub(crate) profile_name: String, pub(crate) profile_path: PathBuf, pub(crate) profile: profile::Profile, + pub(crate) patch: profile::Profile, +} + +/// Where a save prompt writes its rules. +#[derive(Debug, Clone, PartialEq, Eq)] +enum SaveTarget { + /// An existing profile file the session ran with. + File(PathBuf), + /// A user profile the user names at the prompt. + NewUserProfile, +} + +/// Save targets in precedence order. A new user profile is offered only when +/// the session ran with no writable profile file. +fn save_targets(save_files: &[profile::ProfileSourceFile]) -> Vec { + if save_files.is_empty() { + return vec![SaveTarget::NewUserProfile]; + } + save_files + .iter() + .map(|file| SaveTarget::File(file.path.clone())) + .collect() } #[derive(Clone, Copy)] @@ -271,7 +293,7 @@ pub(crate) fn offer_save_run_profile(offer: &ProfileSaveOffer<'_>) -> Result<()> )? else { // No filesystem patch — but we may still have URL denials - return offer_url_only_save(offer.url_denials, offer.command, offer.compared_profile); + return offer_url_only_save(offer); }; // Merge URL grants into the filesystem patch profile so extract_denial_items @@ -292,32 +314,23 @@ pub(crate) fn offer_save_run_profile(offer: &ProfileSaveOffer<'_>) -> Result<()> let Some(combined_patch) = build_combined_patch_from_items(&items) else { return Ok(()); }; - offer_save_with_patch( - &combined_patch, - &cmd_name, - offer.command, - offer.compared_profile, - ) + offer_save_with_patch(&combined_patch, &cmd_name, offer) } - None => offer_save_text_prompt(&patch, &cmd_name, offer.command, offer.compared_profile), + None => offer_save_text_prompt(&patch, &cmd_name, offer), } } /// Offer profile save when only URL denials exist (no filesystem patch). -fn offer_url_only_save( - url_denials: &[UrlDenialRecord], - command: &[std::ffi::OsString], - compared_profile: Option<&str>, -) -> Result<()> { - if url_denials.is_empty() { +fn offer_url_only_save(offer: &ProfileSaveOffer<'_>) -> Result<()> { + if offer.url_denials.is_empty() { return Ok(()); } - let Some(url_patch) = build_url_patch(url_denials) else { + let Some(url_patch) = build_url_patch(offer.url_denials) else { return Ok(()); }; - let Some(cmd_name) = offer_command_name(command) else { + let Some(cmd_name) = offer_command_name(offer.command) else { return Ok(()); }; @@ -326,17 +339,16 @@ fn offer_url_only_save( let Some(combined_patch) = build_combined_patch_from_items(&items) else { return Ok(()); }; - offer_save_with_patch(&combined_patch, &cmd_name, command, compared_profile) + offer_save_with_patch(&combined_patch, &cmd_name, offer) } - None => offer_save_text_prompt(&url_patch, &cmd_name, command, compared_profile), + None => offer_save_text_prompt(&url_patch, &cmd_name, offer), } } fn offer_save_with_patch( patch: &profile::Profile, cmd_name: &str, - command: &[std::ffi::OsString], - compared_profile: Option<&str>, + offer: &ProfileSaveOffer<'_>, ) -> Result<()> { let has_overrides = patch_has_policy_overrides(patch); if has_overrides @@ -348,42 +360,21 @@ fn offer_save_with_patch( return Ok(()); } - let has_suppressions = !patch.filesystem.suppress_save_prompt.is_empty(); - - if let Some(existing_profile) = compared_profile - .filter(|name| profile::is_valid_profile_name(name) && profile::is_user_override(name)) - { - let prepared = - prepare_profile_save_from_patch(patch, cmd_name, existing_profile, compared_profile)?; - write_profile(&prepared)?; - print_profile_save(&prepared, command); - if has_suppressions { - print_suppression_save_note(patch); - } - return Ok(()); - } - - let suggested = suggested_run_profile_name(compared_profile, cmd_name); - let Some(profile_name) = prompt_profile_name(suggested.as_deref())? else { + let Some(target) = choose_save_target( + save_targets(offer.profile_save_files), + top_level_profile(offer.profile_save_files), + &mut TtyPrompt, + )? + else { return Ok(()); }; - - let prepared = - prepare_profile_save_from_patch(patch, cmd_name, &profile_name, compared_profile)?; - write_profile(&prepared)?; - print_profile_save(&prepared, command); - if has_suppressions { - print_suppression_save_note(patch); - } - - Ok(()) + save_patch_to_target(&target, patch, cmd_name, offer) } fn offer_save_text_prompt( patch: &profile::Profile, cmd_name: &str, - command: &[std::ffi::OsString], - compared_profile: Option<&str>, + offer: &ProfileSaveOffer<'_>, ) -> Result<()> { let has_overrides = patch_has_policy_overrides(patch); let suppress_patch = build_suppress_save_prompt_patch(patch); @@ -392,53 +383,177 @@ fn offer_save_text_prompt( prompt_println(""); print_patch_preview(patch); - if let Some(existing_profile) = compared_profile - .filter(|name| profile::is_valid_profile_name(name) && profile::is_user_override(name)) - { - let choice = prompt_profile_save_choice(Some(existing_profile), suppress_patch.is_some())?; - let Some(selected_patch) = - selected_profile_save_patch(choice, patch, suppress_patch.as_ref(), has_overrides)? - else { - return Ok(()); - }; - - let prepared = prepare_profile_save_from_patch( - selected_patch, - cmd_name, - existing_profile, - compared_profile, - )?; - write_profile(&prepared)?; - print_profile_save(&prepared, command); - if choice == ProfileSaveChoice::Suppress { - print_suppression_save_note(selected_patch); - } - return Ok(()); - } - - let choice = prompt_profile_save_choice(None, suppress_patch.is_some())?; + let targets = save_targets(offer.profile_save_files); + let choice = prompt_profile_save_choice(&targets, suppress_patch.is_some(), &mut TtyPrompt)?; let Some(selected_patch) = selected_profile_save_patch(choice, patch, suppress_patch.as_ref(), has_overrides)? else { return Ok(()); }; - let suggested = suggested_run_profile_name(compared_profile, cmd_name); - let Some(profile_name) = prompt_profile_name(suggested.as_deref())? else { + let Some(target) = choose_save_target( + targets, + top_level_profile(offer.profile_save_files), + &mut TtyPrompt, + )? + else { return Ok(()); }; + save_patch_to_target(&target, selected_patch, cmd_name, offer) +} - let prepared = - prepare_profile_save_from_patch(selected_patch, cmd_name, &profile_name, compared_profile)?; - write_profile(&prepared)?; - print_profile_save(&prepared, command); - if choice == ProfileSaveChoice::Suppress { - print_suppression_save_note(selected_patch); +/// The top-level profile's file, when it is one of the writable save files. +fn top_level_profile(save_files: &[profile::ProfileSourceFile]) -> Option<&Path> { + save_files + .iter() + .find(|file| file.top_level) + .map(|file| file.path.as_path()) +} + +/// A lone target is written without asking only when it is the top-level +/// profile; a base reached through `--extends` under a pack or built-in is +/// shared, so the user picks it explicitly. +fn save_target_menu_needed(files: &[PathBuf], top_level: Option<&Path>) -> bool { + match files { + [] => false, + [only] => Some(only.as_path()) != top_level, + _ => true, + } +} + +/// The target to save to: the top-level profile when it is the only writable +/// file, otherwise the user's menu choice. `None` when the user skips. +fn choose_save_target( + targets: Vec, + top_level: Option<&Path>, + io: &mut impl PromptLines, +) -> Result> { + let files: Vec = targets + .iter() + .filter_map(|target| match target { + SaveTarget::File(path) => Some(path.clone()), + SaveTarget::NewUserProfile => None, + }) + .collect(); + if !save_target_menu_needed(&files, top_level) { + return Ok(targets.into_iter().next()); + } + Ok(prompt_save_target(&files, top_level, io)?.map(SaveTarget::File)) +} + +fn render_save_target_menu(files: &[PathBuf], top_level: Option<&Path>) -> String { + let paths: Vec = files + .iter() + .map(|path| path.display().to_string()) + .collect(); + let width = paths + .iter() + .map(|path| path.chars().count()) + .max() + .unwrap_or(0); + let mut menu = String::from("Save the selected rules to:\n"); + for (index, (file, path)) in files.iter().zip(&paths).enumerate() { + let label = if Some(file.as_path()) == top_level { + "this profile" + } else { + "base — applies to every profile that extends it" + }; + menu.push_str(&format!(" {}) {path: Option> { + match input.trim().to_ascii_lowercase().as_str() { + "" => Some(Some(0)), + "skip" => Some(None), + number => number + .parse::() + .ok() + .filter(|choice| (1..=count).contains(choice)) + .map(|choice| Some(choice - 1)), + } +} + +/// `input` is the raw line read, so an empty string means the input closed +/// (EOF) and cancels, while Enter arrives as a newline and picks file 1. +fn chosen_save_target(files: &[PathBuf], input: &str) -> Option> { + if input.is_empty() { + return Some(None); + } + parse_save_target_choice(input, files.len()) + .map(|choice| choice.and_then(|index| files.get(index).cloned())) +} + +fn prompt_save_target( + files: &[PathBuf], + top_level: Option<&Path>, + io: &mut impl PromptLines, +) -> Result> { + let menu = render_save_target_menu(files, top_level); + let (options, choice_prompt) = menu.rsplit_once('\n').unwrap_or(("", menu.as_str())); + for line in options.lines() { + io.println(line); } + loop { + io.print(choice_prompt); + let input = io.read_line()?; + if let Some(choice) = chosen_save_target(files, &input) { + return Ok(choice); + } + let help = format!( + "Enter a number from 1 to {}, press Enter for 1, or type skip.", + files.len() + ); + io.println(&format!("{}", help.red())); + } +} +/// Write `patch` to `target`, prompting for a name when it is a new user +/// profile, and report the save. +fn save_patch_to_target( + target: &SaveTarget, + patch: &profile::Profile, + cmd_name: &str, + offer: &ProfileSaveOffer<'_>, +) -> Result<()> { + let Some(prepared) = prepare_save_to_target(target, patch, cmd_name, offer.compared_profile)? + else { + return Ok(()); + }; + write_profile(&prepared)?; + print_profile_save(&prepared, offer.command); + print_suppression_save_note(patch); Ok(()) } +/// Prepare the save of `patch` to `target`. `None` when the user cancels the +/// new-profile name prompt. +fn prepare_save_to_target( + target: &SaveTarget, + patch: &profile::Profile, + cmd_name: &str, + compared_profile: Option<&str>, +) -> Result> { + match target { + SaveTarget::File(path) => { + let path_text = path.display().to_string(); + let run_with = compared_profile.unwrap_or(&path_text); + prepare_profile_save_to_file(patch, path, run_with).map(Some) + } + SaveTarget::NewUserProfile => { + let suggested = suggested_run_profile_name(compared_profile, cmd_name); + let Some(profile_name) = prompt_profile_name(suggested.as_deref())? else { + return Ok(None); + }; + prepare_profile_save_from_patch(patch, cmd_name, &profile_name, compared_profile) + .map(Some) + } + } +} + fn selected_profile_save_patch<'a>( choice: ProfileSaveChoice, grant_patch: &'a profile::Profile, @@ -544,32 +659,49 @@ fn prompt_profile_name(suggested: Option<&str>) -> Result> { } } +/// The save question names the file only when it is the sole target; with +/// several, the menu that follows picks the file. +fn profile_save_question(targets: &[SaveTarget], can_suppress: bool) -> String { + if targets.len() > 1 { + return if can_suppress { + "Save suggestions to a profile? [g] grant / [s] suppress / [Enter] skip: ".to_string() + } else { + "Save the shown rules to a profile? [g] save / [Enter] skip: ".to_string() + }; + } + let existing_profile = match targets.first() { + Some(SaveTarget::File(path)) => Some(path.as_path()), + Some(SaveTarget::NewUserProfile) | None => None, + }; + match (existing_profile, can_suppress) { + (Some(path), true) => format!( + "Update profile '{}' with suggestions? [g] grant / [s] suppress / [Enter] skip: ", + path.display() + ), + (Some(path), false) => format!( + "Update existing profile '{}' with the shown rules? [g] save / [Enter] skip: ", + path.display() + ), + (None, true) => { + "Save suggestions to a user profile? [g] grant / [s] suppress / [Enter] skip: " + .to_string() + } + (None, false) => { + "Save the shown rules in a user profile? [g] save / [Enter] skip: ".to_string() + } + } +} + fn prompt_profile_save_choice( - existing_profile: Option<&str>, + targets: &[SaveTarget], can_suppress: bool, + io: &mut impl PromptLines, ) -> Result { + let prompt = profile_save_question(targets, can_suppress); loop { - let prompt = match (existing_profile, can_suppress) { - (Some(name), true) => format!( - "Update user profile '{}' with suggestions? [g] grant / [s] suppress / [Enter] skip: ", - name - ), - (Some(name), false) => format!( - "Update existing user profile '{}' with the shown rules? [g] save / [Enter] skip: ", - name - ), - (None, true) => { - "Save suggestions to a user profile? [g] grant / [s] suppress / [Enter] skip: " - .to_string() - } - (None, false) => { - "Save the shown rules in a user profile? [g] save / [Enter] skip: ".to_string() - } - }; + io.print(&prompt); - prompt_print(&prompt, &[]); - - let input = read_input_line()?; + let input = io.read_line()?; if let Some(choice) = parse_profile_save_choice(&input, can_suppress) { return Ok(choice); } @@ -579,7 +711,7 @@ fn prompt_profile_save_choice( } else { "Enter g to save, or press Enter to skip." }; - prompt_println(&format!("{}", help.red())); + io.println(&format!("{}", help.red())); } } @@ -739,12 +871,51 @@ pub(crate) fn write_profile(prepared: &PreparedProfileSave) -> Result<()> { )) })?; - let profile_json = serde_json::to_string_pretty(&prepared.profile) - .map_err(|e| NonoError::LearnError(format!("Failed to serialize profile: {}", e)))?; - atomic_write( - &prepared.profile_path, - format!("{profile_json}\n").as_bytes(), - ) + let contents = match prepared.action { + SaveAction::Created => { + let profile_json = serde_json::to_string_pretty(&prepared.profile).map_err(|e| { + NonoError::LearnError(format!("Failed to serialize profile: {}", e)) + })?; + format!("{profile_json}\n") + } + SaveAction::Updated => { + // Checked again here as well as in prepare: the file may have been + // swapped in between, and every writer must go through the check. + writable_profile_path(&prepared.profile_path)?; + updated_profile_text(&prepared.profile_path, &prepared.patch)? + } + }; + atomic_write(&prepared.profile_path, contents.as_bytes()) +} + +/// Apply `patch` to the profile file's current text, keeping its comments. +/// The result must still parse as a profile; otherwise the file is left as is. +fn updated_profile_text(profile_path: &Path, patch: &profile::Profile) -> Result { + let original = std::fs::read_to_string(profile_path).map_err(|e| { + NonoError::LearnError(format!( + "Failed to read profile {}: {}", + profile_path.display(), + e + )) + })?; + let updated = crate::profile_file_edit::apply_patch_to_profile_text(&original, patch).map_err( + |e| match e { + NonoError::LearnError(message) => NonoError::LearnError(format!( + "Failed to update profile {}: {}", + profile_path.display(), + message + )), + other => other, + }, + )?; + profile::parse_profile_bytes(updated.as_bytes()).map_err(|e| { + NonoError::LearnError(format!( + "Updated profile {} would be invalid: {}", + profile_path.display(), + e + )) + })?; + Ok(updated) } /// Write `contents` to `path` atomically: write to a sibling temp file, fsync, @@ -761,13 +932,6 @@ fn atomic_write(path: &Path, contents: &[u8]) -> Result<()> { .file_name() .ok_or_else(|| NonoError::LearnError(format!("Invalid profile path {}", path.display())))?; - // Use a sibling temp file so the final rename is same-filesystem and - // therefore atomic on POSIX. - let mut tmp_name = std::ffi::OsString::from("."); - tmp_name.push(file_name); - tmp_name.push(format!(".tmp.{}", std::process::id())); - let tmp_path = dir.join(&tmp_name); - let write_err = |stage: &str, e: std::io::Error| { NonoError::LearnError(format!( "Failed to {} profile {}: {}", @@ -777,26 +941,32 @@ fn atomic_write(path: &Path, contents: &[u8]) -> Result<()> { )) }; - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(&tmp_path) - .map_err(|e| write_err("open temp file for", e))?; - if let Err(e) = file.write_all(contents) { - let _ = std::fs::remove_file(&tmp_path); - return Err(write_err("write", e)); - } - if let Err(e) = file.sync_all() { - let _ = std::fs::remove_file(&tmp_path); - return Err(write_err("sync", e)); - } - drop(file); - - if let Err(e) = std::fs::rename(&tmp_path, path) { - let _ = std::fs::remove_file(&tmp_path); - return Err(write_err("rename into place", e)); + // The directory may be writable by the sandboxed agent, so the temp file + // gets a random name and is created exclusively (O_EXCL): a name the agent + // planted, such as a symlink to a shell rc file, is never opened. A sibling + // keeps the final rename same-filesystem and therefore atomic on POSIX. + let mut prefix = std::ffi::OsString::from("."); + prefix.push(file_name); + prefix.push("."); + let mut builder = tempfile::Builder::new(); + builder.prefix(&prefix).suffix(".tmp"); + // Match a plain create: mode 0666 filtered by the umask. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + builder.permissions(std::fs::Permissions::from_mode(0o666)); } + let mut file = builder + .tempfile_in(dir) + .map_err(|e| write_err("open temp file for", e))?; + // Dropping `file` on an early return removes the temp file. + file.write_all(contents) + .map_err(|e| write_err("write", e))?; + file.as_file() + .sync_all() + .map_err(|e| write_err("sync", e))?; + file.persist(path) + .map_err(|e| write_err("rename into place", e.error))?; Ok(()) } @@ -1676,6 +1846,57 @@ fn prompt_line_for_tty(message: &str) -> String { format!("\r{message}\x1b[K\r\n") } +/// Prepare an update of the existing profile file at `path`. `run_with` is the +/// `--profile` value shown in the "Run with:" hint. +pub(crate) fn prepare_profile_save_to_file( + patch: &profile::Profile, + path: &Path, + run_with: &str, +) -> Result { + let path = writable_profile_path(path)?; + let mut existing = profile::load_raw_profile_from_path(&path)?; + merge_profile_patch(&mut existing, patch); + Ok(PreparedProfileSave { + action: SaveAction::Updated, + profile_name: run_with.to_string(), + profile_path: path, + profile: existing, + patch: patch.clone(), + }) +} + +/// Re-check a save target just before writing to it. `path` is the canonical +/// path recorded when the profile loaded, but the sandboxed agent may since +/// have swapped it, or a parent directory, for a symlink to another file (a +/// user profile, the pack store), or for a FIFO that would hang the read. +fn writable_profile_path(path: &Path) -> Result { + let canonical = std::fs::canonicalize(path).map_err(|e| { + NonoError::LearnError(format!( + "Cannot resolve profile file {}: {e}", + path.display() + )) + })?; + if canonical != path { + return Err(NonoError::LearnError(format!( + "Refusing to save to {}: it now resolves to {}", + path.display(), + canonical.display() + ))); + } + let is_file = std::fs::metadata(&canonical) + .map_err(|e| { + NonoError::LearnError(format!("Cannot read profile file {}: {e}", path.display())) + })? + .is_file(); + if !is_file { + return Err(NonoError::LearnError(format!( + "Refusing to save to {}: it is not a regular file", + path.display() + ))); + } + Ok(canonical) +} + pub(crate) fn prepare_profile_save_from_patch( patch: &profile::Profile, cmd_name: &str, @@ -1693,6 +1914,7 @@ pub(crate) fn prepare_profile_save_from_patch( profile_name: profile_name.to_string(), profile_path, profile: existing, + patch: patch.clone(), }); } @@ -1728,6 +1950,7 @@ pub(crate) fn prepare_profile_save_from_patch( profile_name: profile_name.to_string(), profile_path, profile: new_profile, + patch: patch.clone(), }) } @@ -1735,6 +1958,31 @@ fn read_input_line() -> Result { prompt_read_line() } +/// Line I/O for the save-menu loops, so tests can script the answers. +/// `read_line` returns the raw line: an empty string is EOF, Enter is "\n". +trait PromptLines { + fn print(&mut self, text: &str); + fn println(&mut self, line: &str); + fn read_line(&mut self) -> Result; +} + +/// The controlling terminal, as every other save prompt uses it. +struct TtyPrompt; + +impl PromptLines for TtyPrompt { + fn print(&mut self, text: &str) { + prompt_write(text); + } + + fn println(&mut self, line: &str) { + prompt_println(line); + } + + fn read_line(&mut self) -> Result { + read_input_line() + } +} + fn build_run_profile_patch( policy_explanations: &[PolicyExplanation], error_observation: &ErrorObservation, @@ -1964,6 +2212,8 @@ fn merge_access(existing: AccessMode, requested: AccessMode) -> AccessMode { } } +/// Keep the merged fields in step with `PATCHED_LISTS` in `profile_file_edit`, +/// which applies the same patch to profile text when updating a file. pub(crate) fn merge_profile_patch(profile: &mut profile::Profile, patch: &profile::Profile) { profile.filesystem.allow = profile::dedup_append(&profile.filesystem.allow, &patch.filesystem.allow); @@ -2691,6 +2941,683 @@ mod tests { ); } + fn project_source_file(path: &str) -> profile::ProfileSourceFile { + profile::ProfileSourceFile { + path: PathBuf::from(path), + kind: profile::ProfileSourceKind::Project, + top_level: false, + } + } + + #[test] + fn save_targets_empty_is_new_user_profile() { + assert_eq!(save_targets(&[]), vec![SaveTarget::NewUserProfile]); + } + + #[test] + fn save_targets_files_never_include_new_user_profile() { + let files = [ + project_source_file("/work/proj/agent.json"), + project_source_file("/work/shared/base.json"), + ]; + + assert_eq!( + save_targets(&files), + vec![ + SaveTarget::File(PathBuf::from("/work/proj/agent.json")), + SaveTarget::File(PathBuf::from("/work/shared/base.json")), + ] + ); + } + + #[test] + fn prepare_profile_save_to_file_updates_path_profile() { + let dir = TempDir::new().expect("tempdir"); + let profile_dir = dir.path().join("proj/.nono"); + std::fs::create_dir_all(&profile_dir).expect("mkdir"); + let profile_path = profile_dir.join("agent.json"); + std::fs::write( + &profile_path, + r#"{ "meta": { "name": "agent" }, "filesystem": { "read": ["/old"] } }"#, + ) + .expect("write profile"); + + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + + let prepared = prepare_profile_save_to_file( + &patch, + &profile_path.canonicalize().expect("canonicalize"), + "./proj/.nono/agent.json", + ) + .expect("prepare"); + + assert!(matches!(prepared.action, SaveAction::Updated)); + assert_eq!( + prepared.profile_path, + profile_path.canonicalize().expect("canonicalize") + ); + assert_eq!(prepared.profile_name, "./proj/.nono/agent.json"); + assert_eq!( + prepared.profile.filesystem.read, + vec!["/old".to_string(), "/new".to_string()] + ); + assert_eq!(prepared.patch.filesystem.read, vec!["/new".to_string()]); + } + + #[cfg(unix)] + #[test] + fn path_profile_symlinked_into_pack_store_not_offered() { + let (source_files, writable) = crate::test_env::with_isolated_config_home(|config| { + let install_dir = crate::test_env::write_fake_pack( + config, + "acme", + "packy", + "packy", + r#"{ "meta": { "name": "packy" } }"#, + &[], + None, + ); + let dir = TempDir::new().expect("tempdir"); + let link = dir.path().join("agent.json"); + std::os::unix::fs::symlink(install_dir.join("profiles/packy.json"), &link) + .expect("symlink"); + + let loaded = profile::load_profile_from_path(&link).expect("load via symlink"); + (loaded.source_files.clone(), loaded.writable_source_files()) + }); + + assert_eq!(source_files.len(), 1); + assert_eq!(source_files[0].kind, profile::ProfileSourceKind::Pack); + assert!(writable.is_empty(), "got {writable:?}"); + assert_eq!(save_targets(&writable), vec![SaveTarget::NewUserProfile]); + } + + #[test] + fn render_menu_labels_top_and_bases() { + let files = [ + PathBuf::from("/work/proj/.nono/agent.json"), + PathBuf::from("/work/shared/base.json"), + ]; + + let menu = render_save_target_menu(&files, Some(files[0].as_path())); + let lines: Vec<&str> = menu.split('\n').collect(); + + assert_eq!(lines.len(), 4, "{menu}"); + assert_eq!(lines[0], "Save the selected rules to:"); + assert_eq!( + lines[1], + " 1) /work/proj/.nono/agent.json (this profile)" + ); + assert!( + lines[2].starts_with(" 2) /work/shared/base.json "), + "{menu}" + ); + assert!( + lines[2].ends_with("(base — applies to every profile that extends it)"), + "{menu}" + ); + assert_eq!(lines[1].find('('), lines[2].find('('), "{menu}"); + assert_eq!(lines[3], "Choice [1]: "); + } + + #[test] + fn render_menu_labels_only_the_top_level_profile_as_this_profile() { + let files = [PathBuf::from("/work/extra.json")]; + + let menu = render_save_target_menu(&files, Some(Path::new("/work/agent.json"))); + assert!( + menu.contains("(base — applies to every profile that extends it)"), + "{menu}" + ); + assert!(!menu.contains("this profile"), "{menu}"); + + let menu = render_save_target_menu(&files, None); + assert!(!menu.contains("this profile"), "{menu}"); + } + + #[test] + fn save_target_menu_shown_unless_the_only_target_is_the_top_level_profile() { + let top = PathBuf::from("/work/agent.json"); + let extra = PathBuf::from("/work/extra.json"); + let top_level = Some(top.as_path()); + + assert!(!save_target_menu_needed( + std::slice::from_ref(&top), + top_level + )); + assert!(save_target_menu_needed( + std::slice::from_ref(&extra), + top_level + )); + assert!(save_target_menu_needed(std::slice::from_ref(&extra), None)); + assert!(save_target_menu_needed( + &[top.clone(), extra.clone()], + top_level + )); + assert!(!save_target_menu_needed(&[], None)); + } + + #[test] + fn prepare_save_to_file_target_runs_with_full_path_and_writes_patch() { + let dir = TempDir::new().expect("tempdir"); + let path = dir + .path() + .canonicalize() + .expect("canonicalize") + .join("agent.json"); + std::fs::write(&path, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + + let prepared = + prepare_save_to_target(&SaveTarget::File(path.clone()), &patch, "claude", None) + .expect("prepare") + .expect("file target never cancels"); + write_profile(&prepared).expect("write"); + + assert_eq!(prepared.profile_path, path); + assert_eq!(prepared.profile_name, path.display().to_string()); + let written = std::fs::read_to_string(&path).expect("read agent"); + let reparsed = profile::parse_profile_bytes(written.as_bytes()).expect("reparse"); + assert_eq!(reparsed.filesystem.read, vec!["/new"]); + } + + #[test] + fn parse_choice_enter_is_first() { + assert_eq!(parse_save_target_choice("", 2), Some(Some(0))); + assert_eq!(parse_save_target_choice("2", 2), Some(Some(1))); + assert_eq!(parse_save_target_choice(" skip ", 2), Some(None)); + assert_eq!(parse_save_target_choice("3", 2), None); + assert_eq!(parse_save_target_choice("0", 2), None); + assert_eq!(parse_save_target_choice("x", 2), None); + } + + /// Scripted answers for the save-menu loops; `output` collects every + /// prompt and help line they print. + struct ScriptedPrompt { + input: std::io::Cursor<&'static str>, + output: String, + } + + impl ScriptedPrompt { + fn new(input: &'static str) -> Self { + Self { + input: std::io::Cursor::new(input), + output: String::new(), + } + } + } + + impl PromptLines for ScriptedPrompt { + fn print(&mut self, text: &str) { + self.output.push_str(text); + } + + fn println(&mut self, line: &str) { + self.output.push_str(line); + self.output.push('\n'); + } + + fn read_line(&mut self) -> Result { + let mut line = String::new(); + self.input.read_line(&mut line).map_err(NonoError::Io)?; + Ok(line) + } + } + + const MENU_HELP: &str = "Enter a number from 1 to 2, press Enter for 1, or type skip."; + + fn menu_files() -> [PathBuf; 2] { + [ + PathBuf::from("/work/agent.json"), + PathBuf::from("/work/base.json"), + ] + } + + fn run_menu(input: &'static str) -> (Option, String) { + let files = menu_files(); + let mut io = ScriptedPrompt::new(input); + let choice = prompt_save_target(&files, Some(files[0].as_path()), &mut io).expect("menu"); + (choice, io.output) + } + + #[test] + fn menu_invalid_answer_prints_help_once_then_takes_the_valid_one() { + let (choice, output) = run_menu("x\n2\n"); + assert_eq!(choice, Some(PathBuf::from("/work/base.json"))); + assert_eq!(output.matches(MENU_HELP).count(), 1, "{output}"); + assert_eq!(output.matches("Choice [1]: ").count(), 2, "{output}"); + assert!(output.contains("Save the selected rules to:"), "{output}"); + } + + #[test] + fn menu_enter_picks_the_first_file() { + let (choice, output) = run_menu("\n"); + assert_eq!(choice, Some(PathBuf::from("/work/agent.json"))); + assert!(!output.contains(MENU_HELP), "{output}"); + } + + #[test] + fn menu_eof_cancels() { + assert_eq!(run_menu("").0, None); + } + + #[test] + fn menu_skip_cancels() { + assert_eq!(run_menu("skip\n").0, None); + } + + #[test] + fn lone_base_target_still_shows_the_menu() { + let base = PathBuf::from("/work/base.json"); + let mut io = ScriptedPrompt::new("1\n"); + let target = choose_save_target( + vec![SaveTarget::File(base.clone())], + Some(Path::new("/work/agent.json")), + &mut io, + ) + .expect("choose"); + assert!( + matches!(target, Some(SaveTarget::File(ref path)) if *path == base), + "lone base must be picked through the menu" + ); + assert!( + io.output.contains("Save the selected rules to:"), + "{}", + io.output + ); + assert!(io.output.contains("Choice [1]: "), "{}", io.output); + } + + fn run_save_question(input: &'static str, can_suppress: bool) -> (ProfileSaveChoice, String) { + let mut io = ScriptedPrompt::new(input); + let choice = + prompt_profile_save_choice(&[SaveTarget::NewUserProfile], can_suppress, &mut io) + .expect("question"); + (choice, io.output) + } + + #[test] + fn save_question_maps_g_s_and_enter() { + assert_eq!(run_save_question("g\n", true).0, ProfileSaveChoice::Grant); + assert_eq!( + run_save_question("s\n", true).0, + ProfileSaveChoice::Suppress + ); + assert_eq!(run_save_question("\n", true).0, ProfileSaveChoice::Skip); + assert_eq!(run_save_question("", true).0, ProfileSaveChoice::Skip); + } + + #[test] + fn save_question_reprompts_after_invalid_answer() { + let (choice, output) = run_save_question("x\ng\n", true); + assert_eq!(choice, ProfileSaveChoice::Grant); + let help = "Enter g to grant, s to suppress, or press Enter to skip."; + assert_eq!(output.matches(help).count(), 1, "{output}"); + let question = profile_save_question(&[SaveTarget::NewUserProfile], true); + assert_eq!(output.matches(question.as_str()).count(), 2, "{output}"); + } + + #[test] + fn save_question_rejects_s_when_suppress_is_not_offered() { + let (choice, output) = run_save_question("s\n\n", false); + assert_eq!(choice, ProfileSaveChoice::Skip); + let help = "Enter g to save, or press Enter to skip."; + assert_eq!(output.matches(help).count(), 1, "{output}"); + } + + #[test] + fn menu_choice_two_writes_base_only() { + let dir = TempDir::new().expect("tempdir"); + let root = dir.path().canonicalize().expect("canonicalize"); + let top = root.join("top.json"); + let base = root.join("base.json"); + let top_text = "{ \"meta\": { \"name\": \"top\" }, \"extends\": [\"./base.json\"] }\n"; + std::fs::write(&top, top_text).expect("write top"); + std::fs::write( + &base, + "{\n // shared rules\n \"meta\": { \"name\": \"base\" }\n}\n", + ) + .expect("write base"); + let files = vec![top.clone(), base.clone()]; + + assert_eq!(chosen_save_target(&files, "2"), Some(Some(base.clone()))); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let prepared = prepare_profile_save_to_file(&patch, &base, "./top.json").expect("prepare"); + write_profile(&prepared).expect("write base"); + + let written = std::fs::read_to_string(&base).expect("read base"); + assert!(written.contains("// shared rules"), "{written}"); + let reparsed = profile::parse_profile_bytes(written.as_bytes()).expect("reparse"); + assert_eq!(reparsed.filesystem.read, vec!["/new"]); + assert_eq!(std::fs::read_to_string(&top).expect("read top"), top_text); + } + + #[test] + fn menu_choice_skip_and_invalid() { + let files = vec![PathBuf::from("/a.json"), PathBuf::from("/b.json")]; + + assert_eq!( + chosen_save_target(&files, "\n"), + Some(Some(PathBuf::from("/a.json"))) + ); + assert_eq!(chosen_save_target(&files, "skip\n"), Some(None)); + assert_eq!(chosen_save_target(&files, "9"), None); + } + + #[test] + fn menu_choice_closed_input_cancels() { + let files = vec![PathBuf::from("/a.json"), PathBuf::from("/b.json")]; + + assert_eq!(chosen_save_target(&files, ""), Some(None)); + } + + #[test] + fn save_question_names_file_only_for_single_target() { + let one = [SaveTarget::File(PathBuf::from("/work/agent.json"))]; + let two = [ + SaveTarget::File(PathBuf::from("/work/agent.json")), + SaveTarget::File(PathBuf::from("/work/base.json")), + ]; + + assert_eq!( + profile_save_question(&one, true), + "Update profile '/work/agent.json' with suggestions? [g] grant / [s] suppress / [Enter] skip: " + ); + assert_eq!( + profile_save_question(&two, true), + "Save suggestions to a profile? [g] grant / [s] suppress / [Enter] skip: " + ); + assert_eq!( + profile_save_question(&two, false), + "Save the shown rules to a profile? [g] save / [Enter] skip: " + ); + assert_eq!( + profile_save_question(&[SaveTarget::NewUserProfile], true), + "Save suggestions to a user profile? [g] grant / [s] suppress / [Enter] skip: " + ); + } + + #[cfg(unix)] + #[test] + fn save_refuses_target_dir_swapped_into_pack_store() { + crate::test_env::with_isolated_config_home(|config| { + let install_dir = crate::test_env::write_fake_pack( + config, + "acme", + "packy", + "agent", + r#"{ "meta": { "name": "packy" } }"#, + &[], + None, + ); + let pack_file = install_dir.join("profiles/agent.json"); + let pack_before = std::fs::read(&pack_file).expect("read pack profile"); + + let project = TempDir::new().expect("tempdir"); + let nono_dir = project.path().join(".nono"); + std::fs::create_dir_all(&nono_dir).expect("mkdir"); + let agent = nono_dir.join("agent.json"); + std::fs::write(&agent, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + let files = [profile::ProfileSourceFile::new( + agent.canonicalize().expect("canonicalize"), + )]; + let targets = save_targets(&files); + let SaveTarget::File(path) = &targets[0] else { + panic!("expected a file target, got {targets:?}"); + }; + + std::fs::remove_dir_all(&nono_dir).expect("remove .nono"); + std::os::unix::fs::symlink(install_dir.join("profiles"), &nono_dir) + .expect("symlink .nono into pack store"); + + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let result = prepare_profile_save_to_file(&patch, path, "./.nono/agent.json") + .and_then(|prepared| write_profile(&prepared)); + + assert!( + matches!(result, Err(NonoError::LearnError(_))), + "expected LearnError, got {result:?}" + ); + assert_eq!( + std::fs::read(&pack_file).expect("read pack profile"), + pack_before + ); + }); + } + + #[cfg(unix)] + #[test] + fn save_refuses_target_swapped_for_symlink_to_another_profile() { + let project = TempDir::new().expect("tempdir"); + let root = project.path().canonicalize().expect("canonicalize"); + let agent = root.join("agent.json"); + std::fs::write(&agent, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + let other = root.join("other.json"); + let other_text = r#"{ "meta": { "name": "other" } }"#; + std::fs::write(&other, other_text).expect("write other"); + + std::fs::remove_file(&agent).expect("remove agent"); + std::os::unix::fs::symlink(&other, &agent).expect("symlink agent to other"); + + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let result = prepare_profile_save_to_file(&patch, &agent, "./agent.json") + .and_then(|prepared| write_profile(&prepared)); + + match result { + Err(NonoError::LearnError(msg)) => { + assert!(msg.contains(&agent.display().to_string()), "{msg}") + } + other => panic!("expected LearnError, got {other:?}"), + } + assert_eq!( + std::fs::read_to_string(&other).expect("read other"), + other_text + ); + } + + #[cfg(unix)] + #[test] + fn write_refuses_target_swapped_after_prepare() { + let project = TempDir::new().expect("tempdir"); + let root = project.path().canonicalize().expect("canonicalize"); + let agent = root.join("agent.json"); + std::fs::write(&agent, r#"{ "meta": { "name": "agent" } }"#).expect("write agent"); + let other = root.join("other.json"); + let other_text = r#"{ "meta": { "name": "other" } }"#; + std::fs::write(&other, other_text).expect("write other"); + + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let prepared = + prepare_profile_save_to_file(&patch, &agent, "./agent.json").expect("prepare"); + + std::fs::remove_file(&agent).expect("remove agent"); + std::os::unix::fs::symlink(&other, &agent).expect("symlink agent to other"); + + match write_profile(&prepared) { + Err(NonoError::LearnError(msg)) => { + assert!(msg.contains(&agent.display().to_string()), "{msg}") + } + other => panic!("expected LearnError, got {other:?}"), + } + assert_eq!( + std::fs::read_to_string(&other).expect("read other"), + other_text + ); + } + + #[cfg(unix)] + #[test] + fn save_refuses_non_regular_target() { + let project = TempDir::new().expect("tempdir"); + let fifo = project + .path() + .canonicalize() + .expect("canonicalize") + .join("agent.json"); + nix::unistd::mkfifo(&fifo, nix::sys::stat::Mode::S_IRWXU).expect("mkfifo"); + + let patch = profile::Profile::default(); + let result = prepare_profile_save_to_file(&patch, &fifo, "./agent.json"); + + match result { + Err(NonoError::LearnError(msg)) => { + assert!(msg.contains(&fifo.display().to_string()), "{msg}") + } + other => panic!("expected LearnError, got {:?}", other.map(|_| ())), + } + } + + fn write_jsonc_user_profile(name: &str, contents: &str) -> PathBuf { + let dir = profile::user_profile_dir().expect("profile dir"); + std::fs::create_dir_all(&dir).expect("mkdir"); + let path = dir.join(format!("{name}.jsonc")); + std::fs::write(&path, contents).expect("write profile"); + path + } + + #[test] + fn write_profile_update_keeps_jsonc_comments() { + let _env_lock = ENV_LOCK.lock().expect("env lock"); + let temp_home = TempDir::new().expect("temp home"); + let temp_config = TempDir::new().expect("temp config"); + let _env = EnvVarGuard::set_all(&[ + ("HOME", temp_home.path().to_str().expect("home path")), + ( + "XDG_CONFIG_HOME", + temp_config.path().to_str().expect("config path"), + ), + ]); + + let path = write_jsonc_user_profile( + "commented", + "{\n // why this profile exists\n \"meta\": { \"name\": \"commented\", \"version\": \"1.0.0\" },\n \"filesystem\": {\n // project data\n \"read\": [\"/old\"]\n }\n}\n", + ); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + + let prepared = + prepare_profile_save_from_patch(&patch, "claude", "commented", None).expect("prepare"); + assert!(matches!(prepared.action, SaveAction::Updated)); + write_profile(&prepared).expect("write profile"); + + let written = std::fs::read_to_string(&path).expect("read profile"); + assert!(written.contains("// why this profile exists"), "{written}"); + assert!(written.contains("// project data"), "{written}"); + let reparsed = profile::parse_profile_bytes(written.as_bytes()).expect("reparse"); + assert_eq!(reparsed.filesystem.read, vec!["/old", "/new"]); + } + + #[test] + fn write_profile_update_invalid_file_left_unchanged() { + let _env_lock = ENV_LOCK.lock().expect("env lock"); + let temp_home = TempDir::new().expect("temp home"); + let temp_config = TempDir::new().expect("temp config"); + let _env = EnvVarGuard::set_all(&[ + ("HOME", temp_home.path().to_str().expect("home path")), + ( + "XDG_CONFIG_HOME", + temp_config.path().to_str().expect("config path"), + ), + ]); + + let path = write_jsonc_user_profile( + "broken", + "{ \"meta\": { \"name\": \"broken\", \"version\": \"1.0.0\" } }\n", + ); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let prepared = + prepare_profile_save_from_patch(&patch, "claude", "broken", None).expect("prepare"); + + std::fs::write(&path, "{ invalid").expect("corrupt profile"); + let result = write_profile(&prepared); + + assert!(result.is_err(), "expected error, got {:?}", result.err()); + assert_eq!(std::fs::read(&path).expect("read profile"), b"{ invalid"); + } + + #[test] + fn write_profile_update_edit_error_names_file() { + let _env_lock = ENV_LOCK.lock().expect("env lock"); + let temp_home = TempDir::new().expect("temp home"); + let temp_config = TempDir::new().expect("temp config"); + let _env = EnvVarGuard::set_all(&[ + ("HOME", temp_home.path().to_str().expect("home path")), + ( + "XDG_CONFIG_HOME", + temp_config.path().to_str().expect("config path"), + ), + ]); + + let path = write_jsonc_user_profile( + "array-section", + "{ \"meta\": { \"name\": \"array-section\", \"version\": \"1.0.0\" } }\n", + ); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let prepared = prepare_profile_save_from_patch(&patch, "claude", "array-section", None) + .expect("prepare"); + + let array_section = "{ \"meta\": { \"name\": \"array-section\", \"version\": \"1.0.0\" }, \"filesystem\": [] }\n"; + std::fs::write(&path, array_section).expect("rewrite profile"); + let message = write_profile(&prepared) + .expect_err("non-object section") + .to_string(); + + assert!( + message.contains(&path.display().to_string()), + "message: {message}" + ); + assert_eq!( + message.matches("Profile save error:").count(), + 1, + "message: {message}" + ); + assert_eq!( + std::fs::read_to_string(&path).expect("read profile"), + array_section + ); + } + + #[test] + fn write_profile_update_rejects_invalid_profile_left_unchanged() { + let _env_lock = ENV_LOCK.lock().expect("env lock"); + let temp_home = TempDir::new().expect("temp home"); + let temp_config = TempDir::new().expect("temp config"); + let _env = EnvVarGuard::set_all(&[ + ("HOME", temp_home.path().to_str().expect("home path")), + ( + "XDG_CONFIG_HOME", + temp_config.path().to_str().expect("config path"), + ), + ]); + + let path = write_jsonc_user_profile( + "unknown-field", + "{ \"meta\": { \"name\": \"unknown-field\", \"version\": \"1.0.0\" } }\n", + ); + let mut patch = profile::Profile::default(); + patch.filesystem.read = vec!["/new".to_string()]; + let prepared = prepare_profile_save_from_patch(&patch, "claude", "unknown-field", None) + .expect("prepare"); + + let unknown_field = "{ \"meta\": { \"name\": \"unknown-field\", \"version\": \"1.0.0\" }, \"bogus_field\": 1 }\n"; + std::fs::write(&path, unknown_field).expect("rewrite profile"); + let result = write_profile(&prepared); + + assert!(result.is_err(), "expected error, got {:?}", result.err()); + assert_eq!( + std::fs::read_to_string(&path).expect("read profile"), + unknown_field + ); + } + #[test] fn prepare_profile_save_from_suppression_patch_uses_suppression_description() { let _env_lock = ENV_LOCK.lock().expect("env lock"); @@ -2926,11 +3853,30 @@ mod tests { .any(|e| { e.file_name() .to_string_lossy() - .starts_with(".profile.json.tmp.") + .starts_with(".profile.json.") }); assert!(!leftover, "temp file should be renamed into place"); } + #[cfg(unix)] + #[test] + fn atomic_write_does_not_follow_a_planted_temp_symlink() { + let dir = TempDir::new().expect("temp dir"); + let target = dir.path().join("agent.json"); + std::fs::write(&target, b"original\n").expect("seed"); + let victim = dir.path().join("victim"); + std::fs::write(&victim, b"victim\n").expect("seed victim"); + let planted = dir + .path() + .join(format!(".agent.json.tmp.{}", std::process::id())); + std::os::unix::fs::symlink(&victim, &planted).expect("plant symlink"); + + atomic_write(&target, b"updated\n").expect("atomic write"); + + assert_eq!(std::fs::read(&victim).expect("read victim"), b"victim\n"); + assert_eq!(std::fs::read(&target).expect("read target"), b"updated\n"); + } + // ─── URL denial patch tests ─────────────────────────────────────────── fn url_record(origin: &str, reason: UrlDenialReason) -> UrlDenialRecord { diff --git a/crates/nono-cli/src/proxy_runtime.rs b/crates/nono-cli/src/proxy_runtime.rs index 43b8f7f1e..4fc84039e 100644 --- a/crates/nono-cli/src/proxy_runtime.rs +++ b/crates/nono-cli/src/proxy_runtime.rs @@ -4156,6 +4156,7 @@ mod tests { open_url_allow_localhost: false, bypass_protection_paths: Vec::new(), ignored_denial_paths: Vec::new(), + profile_save_files: Vec::new(), suppressed_system_service_operations: Vec::new(), redaction_extra_env_vars: Vec::new(), network_denial_audit: Default::default(), @@ -4233,6 +4234,7 @@ mod tests { open_url_allow_localhost: false, bypass_protection_paths: Vec::new(), ignored_denial_paths: Vec::new(), + profile_save_files: Vec::new(), suppressed_system_service_operations: Vec::new(), redaction_extra_env_vars: Vec::new(), network_denial_audit: Default::default(), @@ -4305,6 +4307,7 @@ mod tests { open_url_allow_localhost: false, bypass_protection_paths: Vec::new(), ignored_denial_paths: Vec::new(), + profile_save_files: Vec::new(), suppressed_system_service_operations: Vec::new(), redaction_extra_env_vars: Vec::new(), network_denial_audit: Default::default(), diff --git a/crates/nono-cli/src/sandbox_prepare.rs b/crates/nono-cli/src/sandbox_prepare.rs index 26b7da931..d43603cb7 100644 --- a/crates/nono-cli/src/sandbox_prepare.rs +++ b/crates/nono-cli/src/sandbox_prepare.rs @@ -557,6 +557,8 @@ pub(crate) struct PreparedSandbox { pub(crate) open_url_allow_localhost: bool, pub(crate) bypass_protection_paths: Vec, pub(crate) ignored_denial_paths: Vec, + /// Writable files the loaded profile came from, highest precedence first. + pub(crate) profile_save_files: Vec, pub(crate) suppressed_system_service_operations: Vec, /// `diagnostics.redaction.extra_env_vars` from the profile: extra /// environment-variable name globs to redact in diagnostics and audit @@ -1550,6 +1552,7 @@ pub(crate) fn prepare_sandbox(args: &SandboxArgs, silent: bool) -> Result Result Result Output { + t.hermetic_command() + .args(args) + .current_dir(cwd) + .output() + .expect("cargo builds CARGO_BIN_EXE_nono before running this test binary") +} + +fn combined(out: &Output) -> String { + format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ) +} + +fn assert_ok(out: &Output) -> String { + let text = combined(out); + assert!(out.status.success(), "expected success, got:\n{text}"); + text +} + +fn assert_err_contains(out: &Output, needle: &str) { + let text = combined(out); + assert!(!out.status.success(), "expected failure, got:\n{text}"); + assert!(text.contains(needle), "expected {needle:?} in:\n{text}"); +} + +/// Canonical test root: macOS tempdirs sit behind symlinks, and resolved paths +/// in error messages are canonical. +fn root(t: &NonoTest) -> PathBuf { + t.root().canonicalize().expect("root exists") +} + +fn write(path: &Path, body: &str) { + fs::create_dir_all(path.parent().expect("test paths have parents")).expect("mkdir"); + fs::write(path, body).expect("write"); +} + +/// A profile body granting read on `dir`, created so the grant is valid. +fn reads(dir: &Path, extends: Option<&str>) -> String { + fs::create_dir_all(dir).expect("mkdir grant dir"); + let extends = extends + .map(|e| format!(r#""extends": "{e}", "#)) + .unwrap_or_default(); + format!( + r#"{{ {extends}"filesystem": {{ "read": ["{}"] }} }}"#, + dir.display() + ) +} + +#[test] +fn profile_path_extends_parent_relative_base() { + let t = nono_test!("relext-parent"); + let r = root(&t); + let base_dir = r.join("data").join("from-base"); + let child_dir = r.join("data").join("from-child"); + write(&r.join("shared/base.json"), &reads(&base_dir, None)); + write( + &r.join("proj/.nono/agent.json"), + &reads(&child_dir, Some("../../shared/base.json")), + ); + + let out = nono( + &t, + &r, + &[ + "run", + "--dry-run", + "--profile", + "./proj/.nono/agent.json", + "--", + "true", + ], + ); + let text = assert_ok(&out); + assert!(text.contains(&base_dir.display().to_string()), "{text}"); + assert!(text.contains(&child_dir.display().to_string()), "{text}"); +} + +#[test] +fn path_base_bare_name_resolves_beside_the_base() { + let t = nono_test!("relext-sibling"); + let r = root(&t); + let common_dir = r.join("data").join("shared-common"); + let decoy_dir = r.join("data").join("decoy-common"); + write(&r.join("shared/common.json"), &reads(&common_dir, None)); + write(&r.join("shared/base.json"), r#"{ "extends": "common" }"#); + write(&r.join("proj/common.json"), &reads(&decoy_dir, None)); + write( + &r.join("proj/agent.json"), + r#"{ "extends": "../shared/base.json" }"#, + ); + + let out = nono(&t, &r, &["profile", "show", "./proj/agent.json"]); + let text = assert_ok(&out); + assert!(text.contains(&common_dir.display().to_string()), "{text}"); + assert!(!text.contains(&decoy_dir.display().to_string()), "{text}"); +} + +fn show_with_extends(t: &NonoTest, extends: &str) -> Output { + let r = root(t); + write( + &r.join("proj/agent.json"), + &format!(r#"{{ "extends": "{extends}" }}"#), + ); + nono(t, &r, &["profile", "show", "./proj/agent.json"]) +} + +#[test] +fn profile_extends_absolute_path_rejected() { + let t = nono_test!("relext-abs"); + let out = show_with_extends(&t, "/abs.json"); + assert_err_contains(&out, &format!("'/abs.json': {ERR_ABSOLUTE_PATH}")); +} + +#[test] +fn profile_extends_home_path_rejected() { + let t = nono_test!("relext-home"); + let out = show_with_extends(&t, "~/x.json"); + assert_err_contains(&out, &format!("'~/x.json': {ERR_ABSOLUTE_PATH}")); +} + +#[test] +fn profile_extends_missing_file_names_resolved_path() { + let t = nono_test!("relext-missing"); + let out = show_with_extends(&t, "./nope.json"); + let resolved = root(&t).join("proj").join("./nope.json"); + assert_err_contains( + &out, + &format!( + "extends './nope.json' resolves to {}, which cannot be read", + resolved.display() + ), + ); +} + +#[test] +fn profile_extends_path_without_extension_rejected() { + let t = nono_test!("relext-noext"); + let out = show_with_extends(&t, "./base"); + assert_err_contains(&out, &format!("'./base': {ERR_NOT_JSON}")); +} + +#[test] +fn profile_extends_slash_name_without_dot_prefix_rejected() { + let t = nono_test!("relext-badname"); + let out = show_with_extends(&t, "shared/base.json"); + assert_err_contains(&out, "invalid base profile name 'shared/base.json'"); +} + +#[test] +fn profile_extends_into_pack_store_rejected() { + let t = nono_test!("relext-store"); + let r = root(&t); + let store_file = r.join("home/.config/nono/packages/acme/pack/profile.json"); + write(&store_file, "{}"); + // proj/agent.json -> ../home/.config/... reaches the store by path. + let out = show_with_extends(&t, "../home/.config/nono/packages/acme/pack/profile.json"); + assert_err_contains( + &out, + &format!("'../home/.config/nono/packages/acme/pack/profile.json': {ERR_PACK_STORE_TARGET}"), + ); +} + +#[test] +fn cli_extends_resolves_against_cwd_not_profile_dir() { + let t = nono_test!("relext-cli-cwd"); + let r = root(&t); + let cwd_dir = r.join("data").join("cwd-base"); + let decoy_dir = r.join("data").join("decoy-base"); + write(&r.join("cwd/x.json"), &reads(&cwd_dir, None)); + write(&r.join("proj/x.json"), &reads(&decoy_dir, None)); + write(&r.join("proj/agent.json"), "{}"); + let profile = r.join("proj/agent.json"); + + let out = nono( + &t, + &r.join("cwd"), + &[ + "run", + "--dry-run", + "--profile", + profile.to_str().expect("utf-8 path"), + "--extends", + "./x.json", + "--", + "true", + ], + ); + let text = assert_ok(&out); + assert!(text.contains(&cwd_dir.display().to_string()), "{text}"); + assert!(!text.contains(&decoy_dir.display().to_string()), "{text}"); +} + +fn run_with_cli_extends(t: &NonoTest, extends: &str) -> Output { + let r = root(t); + write(&r.join("proj/agent.json"), "{}"); + let profile = r.join("proj/agent.json"); + nono( + t, + &r, + &[ + "run", + "--dry-run", + "--profile", + profile.to_str().expect("utf-8 path"), + "--extends", + extends, + "--", + "true", + ], + ) +} + +#[test] +fn cli_extends_missing_file_names_cwd_resolved_path() { + let t = nono_test!("relext-cli-missing"); + let out = run_with_cli_extends(&t, "./missing.json"); + let resolved = root(&t).join("./missing.json"); + assert_err_contains( + &out, + &format!( + "extends './missing.json' resolves to {}, which cannot be read", + resolved.display() + ), + ); +} + +#[test] +fn cli_extends_non_json_rejected() { + let t = nono_test!("relext-cli-txt"); + write(&root(&t).join("x.txt"), "{}"); + let out = run_with_cli_extends(&t, "./x.txt"); + assert_err_contains(&out, &format!("'./x.txt': {ERR_NOT_JSON}")); +} + +#[test] +fn cli_extends_absolute_path_rejected() { + let t = nono_test!("relext-cli-abs"); + let out = run_with_cli_extends(&t, "/abs.json"); + assert_err_contains(&out, &format!("'/abs.json': {ERR_ABSOLUTE_PATH}")); +} + +fn init_with_extends(t: &NonoTest, extends: &str) -> (Output, PathBuf) { + let r = root(t); + let target = r.join("proj/.nono/new.json"); + fs::create_dir_all(target.parent().expect("has parent")).expect("mkdir"); + let out = nono( + t, + &r, + &[ + "profile", + "init", + "new", + "--output", + target.to_str().expect("utf-8 path"), + "--extends", + extends, + ], + ); + (out, target) +} + +#[test] +fn profile_init_writes_relative_extends_verbatim() { + let t = nono_test!("relext-init-ok"); + write(&root(&t).join("proj/base.json"), "{}"); + let (out, target) = init_with_extends(&t, "../base.json"); + assert_ok(&out); + let written: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&target).expect("profile written")) + .expect("valid JSON"); + let extends = &written["extends"]; + let entries: Vec<&str> = match extends { + serde_json::Value::String(s) => vec![s.as_str()], + serde_json::Value::Array(a) => a.iter().filter_map(|v| v.as_str()).collect(), + other => panic!("unexpected extends shape: {other}"), + }; + assert_eq!(entries, vec!["../base.json"], "{written}"); +} + +#[test] +fn profile_init_relative_extends_missing_base_errors() { + let t = nono_test!("relext-init-missing"); + let (out, target) = init_with_extends(&t, "../base.json"); + let resolved = root(&t).join("proj/.nono").join("../base.json"); + assert_err_contains( + &out, + &format!( + "extends '../base.json' resolves to {}, which cannot be read", + resolved.display() + ), + ); + assert!(!target.exists(), "no profile written on error"); +} + +#[test] +fn profile_init_absolute_extends_rejected() { + let t = nono_test!("relext-init-abs"); + let (out, target) = init_with_extends(&t, "/abs.json"); + assert_err_contains(&out, &format!("'/abs.json': {ERR_ABSOLUTE_PATH}")); + assert!(!target.exists(), "no profile written on error"); +} diff --git a/crates/nono-test-support/src/lib.rs b/crates/nono-test-support/src/lib.rs index 795a68e24..ad5aae884 100644 --- a/crates/nono-test-support/src/lib.rs +++ b/crates/nono-test-support/src/lib.rs @@ -106,7 +106,9 @@ impl NonoTest { } /// A `nono` invocation with a hermetic environment and no arguments. - fn hermetic_command(&self) -> Command { + /// + /// Public for subcommands that have no typed builder yet. + pub fn hermetic_command(&self) -> Command { let mut cmd = Command::new(&self.bin); // Fail secure: strip the whole `NONO_*` / `XDG_*` namespace. diff --git a/docs/cli/features/profile-authoring.mdx b/docs/cli/features/profile-authoring.mdx index 8429f8776..a04bbe474 100644 --- a/docs/cli/features/profile-authoring.mdx +++ b/docs/cli/features/profile-authoring.mdx @@ -47,6 +47,38 @@ Profile files use **JSONC** (JSON with Comments): standard JSON plus `//` line c This profile inherits everything from `default` and adds one extra directory. The base profile is unchanged. +### Extending a harness profile from a project + +The most common pattern is a project profile that builds on a harness profile such as `claude-code`, optionally with a shared base file kept next to it: + +```json +{ + "extends": ["claude-code", "../shared/base.json"], + "filesystem": { + "allow": ["$WORKDIR/build"] + } +} +``` + +Save it as `./.nono/agent.json` and run it with `nono run --profile ./.nono/agent.json -- claude`. + +An `extends` entry that starts with `./` or `../` is a file path, resolved against the directory of the file that declares it. Any other entry is a profile name, as before. + +- The file must end in `.json` or `.jsonc`. +- Entries starting with `/` or `~` are rejected: ``absolute and `~/` paths are not supported in `extends`; use a relative path``. Write `./shared/base.json`; `shared/base.json` without the `./` is read as a profile name and is invalid. +- A path that points into the pack store is rejected: ``extend a pack by name (`org/pack`), not by path``. Extend packs by name. +- Path entries are rejected in pack profiles, profile drafts, and built-in profiles. A draft is moved by `nono profile promote`, which would break its relative paths. +- A base file reached by path finds its own bare-name entries (such as `"extends": "team-base"`) among the files next to it. +- Cycles are detected by canonical path and fail the load. + +The `--extends` flag accepts the same entries. A path given on the command line is resolved against the current directory, and works with pack and built-in profiles selected by `--profile`: + +```bash +nono run --profile claude-code --extends ./team-base.json -- claude +``` + +`nono profile init --extends ../base.json` writes the entry as you gave it and checks it relative to the output file. + ### What happens when profiles are combined When nono loads a profile that extends a base, it merges the two together. Most fields fall into one of two patterns: @@ -464,6 +496,31 @@ selector ignores input for its first second and discards queued keypresses before enabling its controls, preventing input intended for the command that just exited from immediately confirming the default grants. +### Choosing which file the save prompt updates + +After a run, the save prompt can write the shown rules into any writable file the profile came from: the file given to `--profile`, a user profile, a draft, or any writable file in its `extends` chain. Files that come from a pack are never offered. + +When the only writable file is the top-level profile, the prompt names it: + +```text +Update existing profile '/work/app/.nono/agent.json' with the shown rules? [g] save / [Enter] skip: +``` + +With two or more, the prompt asks which one to update. The top-level profile is listed first and is the default; its bases follow in precedence order: + +```text +Save the selected rules to: + 1) /work/app/.nono/agent.json (this profile) + 2) /work/shared/base.json (base — applies to every profile that extends it) +Choice [1]: +``` + +The prompt also asks when the only writable file is a base, for example with `--profile claude-code --extends ./team-base.json`. That file is labelled as a base, so you confirm before the rules land in a shared file. Only the top-level profile is labelled "this profile". + +Press Enter for the default, type a number to pick a file, or type `skip` to cancel. Saving to a base applies the rules to every profile that extends it. "A new user profile" is offered only when no writable file exists. + +Updates keep your JSONC comments and layout and only add entries. A profile file that is a symlink is updated at its target. + ### Suppressing Expected macOS System-Service Diagnostics Use `diagnostics.suppress_system_services` for macOS Seatbelt operations you diff --git a/docs/superpowers/plans/2026-10-06-relative-extends.md b/docs/superpowers/plans/2026-10-06-relative-extends.md new file mode 100644 index 000000000..7f10105d3 --- /dev/null +++ b/docs/superpowers/plans/2026-10-06-relative-extends.md @@ -0,0 +1,319 @@ +# Relative `extends` paths and save-to-extended-profile Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let a profile `extends` another profile file by a `./` or `../` path, and let the save-on-exit prompt write grants into any writable profile file in the chain, keeping JSONC comments. + +**Architecture:** A new `profile/extends_ref.rs` owns classifying and resolving each `extends` entry; the resolver, CLI `--extends`, the official-pack chain walker and `profile init` all call it. Each loaded `Profile` records the files it was built from (`source_files`), which is threaded to the save offer. Profile updates go through a new comment-preserving writer built on `jsonc-parser`'s `cst` feature. + +**Tech Stack:** Rust (nono-cli binary crate), `jsonc-parser` 0.34 with `cst`, existing `test_env` helpers. + +**Spec:** `docs/superpowers/specs/2026-10-06-relative-extends-design.md` (issue nolabs-ai/nono#2065) + +## Global Constraints + +- Path entries start with `./` or `../` and must end in `.json` or `.jsonc`. +- Entries starting with `/` or `~` error: "absolute and `~/` paths are not supported in `extends`; use a relative path". +- Path target inside the pack store errors: "extend a pack by name (`org/pack`), not by path". +- Path entries are rejected in pack-store profiles, drafts (`profile-drafts/`) and built-ins. These rules never apply to CLI `--extends` entries. +- CLI Path entries resolve against the cwd; CLI Name entries keep today's resolution (including the selected profile's sibling context). +- Compare paths with `Path::starts_with` on canonical paths, never string prefixes. +- No production `.unwrap()`/`.expect()`; expected failures return `NonoError` (`ProfileInheritance` for extends errors, `LearnError` for save errors). +- A failed write leaves the profile file unchanged. +- Save menu: only when 2+ writable files; precedence order; "a new user profile" only when no writable file exists. +- Commits: `git commit -s`, conventional prefix, ending with `Co-Authored-By: Claude Opus 5.5 `. +- Focused test command: `cargo test -p nono-cli --bin nono `. Before handoff: `make ci` and `make clippy`. + +## Review Focus + +1. A Path entry that is a symlink to a file in another directory: its own bare-name siblings resolve next to the canonical target, not the link. (Task 2 test.) +2. The same base reached as `./a.json` from one file and `../x/a.json` from another, with no cycle: must load, not error. (Task 2 test.) +3. A `.jsonc` profile whose `filesystem` key is absent, or holds an empty array on one line: the writer creates/appends without breaking JSON. (Task 5 test.) +4. A profile file that changed on disk to invalid JSON before the save: the save errors and the file is unchanged. (Task 5 test.) +5. `--profile ./x.json` where `x.json` is a symlink into the pack store: no update offered. (Task 7 test.) + +--- + +### Task 1: Classify and resolve `extends` entries + +**Files:** +- Create: `crates/nono-cli/src/profile/extends_ref.rs` +- Modify: `crates/nono-cli/src/profile/mod.rs` (add `mod extends_ref;` + `pub(crate) use extends_ref::{...}` next to `mod credential_provider;` at line 8) + +**Interfaces:** +- Produces: + ```rust + pub(crate) enum ExtendsRef { Name(String), Registry(String), Path(PathBuf) } // Path is canonical + pub(crate) enum ExtendsOrigin<'a> { File(&'a Path), Cli(&'a Path), Builtin } + // File(p): entry written in the profile file at p (canonical when loaded; may not exist yet for `profile init`). + // Cli(dir): entry from `--extends`; paths resolve against dir. + pub(crate) fn classify_extends_entry(raw: &str, origin: ExtendsOrigin<'_>) -> Result; + impl ExtendsRef { pub(crate) fn visited_key(&self) -> String } // name, or canonical path string + pub(crate) fn is_under_pack_store(path: &Path) -> bool; + ``` + +- [ ] **Step 1: Write failing tests** in `extends_ref.rs` `#[cfg(test)] mod tests`, using `crate::test_env::with_isolated_config_home` and `tempfile::tempdir`: + - `bare_name_is_name` — `"default"` → `Name("default")`. + - `registry_ref_is_registry` — `"nolabs-ai/claude"` → `Registry`. + - `dot_slash_resolves_against_declaring_file_dir` — file `d/child.json`, sibling `d/base.json`; `"./base.json"` → `Path(canonical d/base.json)`. + - `dot_dot_resolves_to_parent` — `d/sub/child.json`, `"../base.json"` → `Path(d/base.json)`. + - `cli_origin_resolves_against_given_dir` — `Cli(dir)`, `"./x.json"` → `Path(dir/x.json)`. + - `missing_path_errors_with_resolved_path` — error string contains the entry and the joined path. + - `path_without_json_extension_errors` — `"./base"` errors. + - `absolute_and_tilde_rejected` — `"/x.json"`, `"~/x.json"` error containing `absolute and \`~/\` paths are not supported`. + - `path_into_pack_store_rejected` — target under `/nono/packages/...` errors containing `extend a pack by name`. + - `path_entry_in_pack_store_profile_rejected` — `File(/ns/p/profiles/x.json)` with `"./y.json"` errors. + - `path_entry_in_draft_rejected` — `File(/nono/profile-drafts/x.json)` errors. + - `path_entry_in_builtin_rejected` — `Builtin` origin errors. + - `cli_origin_skips_file_rules` — `Cli()` with an existing `./x.json` there → `Path`. + - `symlinked_target_returns_canonical` (`#[cfg(unix)]`) — link `d/base.json → e/real.json` returns `e/real.json`. + - `shared_slash_json_is_still_invalid` — `"shared/base.json"` errors with `invalid base profile name`. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile::extends_ref` — expect compile failure (module missing). + +- [ ] **Step 3: Implement.** Order of checks: `./`/`../` → Path; `/` or `~` prefix → error; `is_registry_ref` → Registry; `is_valid_profile_name` → Name; else the existing "invalid base profile name '{raw}'" message. For Path: origin rules first (File under `user_profile_draft_dir`/pack store, Builtin → error), base dir = `Cli(dir)` or `nono::try_canonicalize(file.parent())`, join, `canonicalize` (missing → error naming entry and joined path), then reject `is_under_pack_store(target)`. `is_under_pack_store` canonicalizes `package_store_dir()`; a missing store returns false. All errors are `NonoError::ProfileInheritance`. + +- [ ] **Step 4: Run** the same command — all pass. + +- [ ] **Step 5: Refactor.** Error messages are `const`s that the tests reference; classification reuses `is_registry_ref` / `is_valid_profile_name` rather than copying their logic. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(profile): classify relative-path extends entries (#2065)`. + +### Task 2: Resolve Path entries written in profile files + +**Files:** +- Modify: `crates/nono-cli/src/profile/mod.rs` — `resolve_extends` (~3593), `load_base_profile_raw` (~3699), `ResolvedBase` +- Test: `crates/nono-cli/src/profile/mod.rs` tests module (next to `test_transitive_extends_uses_symlinked_parent_referent`, ~7747) + +**Interfaces:** +- Consumes: Task 1 `classify_extends_entry`, `ExtendsOrigin::{File, Builtin}`, `ExtendsRef::visited_key`. +- Produces: `resolve_extends(child, visited, depth, context_dir, source_file, leading: &[ExtendsRef])` — `leading` is used by Task 3; this task passes `&[]` everywhere. `visited: &mut Vec` now holds `visited_key()` values. + +- [ ] **Step 1: Write failing tests** (temp dirs; `load_from_file(&child, &[])`): + - `test_extends_relative_path_sibling_dir` — `proj/.nono/agent.json` extends `"../shared/base.json"`; `filesystem.read` contains the base's entry then the child's. + - `test_extends_relative_path_base_finds_own_sibling` — `shared/base.json` extends `"common"`; `shared/common.json` is loaded (and a decoy `common.json` beside the child is not). + - `test_extends_relative_path_symlink_uses_target_dir` (`#[cfg(unix)]`) — Review Focus 1. + - `test_extends_relative_path_cycle_detected` — `a.json → ./b.json → ./a.json` errors with `circular dependency`. + - `test_extends_same_file_two_spellings_is_not_cycle` — child extends `["./x/a.json", "./y/b.json"]`, `y/b.json` extends `"../x/a.json"`; loads (Review Focus 2). + - `test_extends_path_entry_in_pack_profile_rejected` — `build_fake_pack_store` profile with `"extends": "./other.json"`; `load_profile("/")` errors. + - `test_extends_name_entries_unchanged` — existing `test_extends_*` tests still pass (run, don't edit). + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile::tests::test_extends` — new tests fail. + +- [ ] **Step 3: Implement.** In `resolve_extends`, classify each raw entry with `ExtendsOrigin::File(source_file)` when `source_file` is `Some`, else `Builtin`; keep the cycle check on `visited_key()`. For `ExtendsRef::Path(p)`: `parse_file_backed_profile(&p)` → `ResolvedBase::Sibling(profile, p)`. Name/Registry go to the existing `load_base_profile_raw` unchanged. Change `ResolvedBase::Global(Profile)` to `Global(Profile, Option)`: pack-store bases carry their profile path, built-ins carry `None`. Recurse into a pack base with `context_dir = None` (name lookup unchanged) but `source_file = Some(pack_path)`, so its path entries hit the pack-store rule and its message; built-in bases hit the Builtin rule. A pack-store top-level profile (`load_from_file` on a store path) already passes `Some(source_path)`. + +- [ ] **Step 4: Run** `cargo test -p nono-cli --bin nono profile::` — all pass. + +- [ ] **Step 5: Refactor.** The "invalid base profile name" check in `load_base_profile_raw` is now done by `classify_extends_entry`; remove the duplicate. Path and sibling bases share `parse_file_backed_profile`. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(profile): resolve relative-path extends entries (#2065)`. + +### Task 3: CLI `--extends` as a separate resolved list + +**Files:** +- Modify: `crates/nono-cli/src/profile/mod.rs` — `load_profile_impl`, `load_profile_inner`, `load_registry_profile`, `load_profile_from_path_impl`, `load_from_file`, built-in branch; remove `prepend_cli_extends`. +- Test: same tests module, next to `test_load_profile_with_extends_prepends_cli_bases` (~5066). + +**Interfaces:** +- Consumes: Task 1 `ExtendsOrigin::Cli`; Task 2 `resolve_extends(..., leading)`. +- Produces: internal `fn load_profile_with_cli_bases(name_or_path: &str, cli: &[ExtendsRef]) -> Result`; `load_profile_with_extends(name_or_path, cli_extends: &[String])` keeps its signature and classifies with `ExtendsOrigin::Cli(&std::env::current_dir()?)`. + +Note: CLI `Name` entries must still resolve with the selected profile's `context_dir` (pinned by `test_cli_extends_preserves_global_file_source_context`). Only `Path` entries are pre-resolved. + +- [ ] **Step 1: Write failing tests:** + - `test_cli_path_extends_with_pack_profile` — fake pack profile + `cli = [Path(/extra.json)]` via `load_profile_with_cli_bases("/", ...)`; result includes `extra.json`'s read entry. + - `test_cli_path_extends_with_builtin_profile` — `load_profile_with_cli_bases("default", [Path(..)])` includes the entry. + - `test_cli_path_resolves_against_cwd_not_profile_dir` — profile in `a/`, `extra.json` in both `a/` and `b/` with different reads; `Cli(b)` classification picks `b/extra.json`. + - Existing `test_load_profile_with_extends_*` and `test_cli_extends_preserves_global_file_source_context` keep passing unedited (ordering: CLI bases, then the profile's own bases, then the child). + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile::tests::test_cli` and `profile::tests::test_load_profile_with_extends` — new tests fail. + +- [ ] **Step 3: Implement.** Thread `cli: &[ExtendsRef]` through the load functions instead of `&[String]`. `resolve_extends` iterates `leading` before the child's own entries at depth 0 only (recursive calls pass `&[]`). The built-in branch calls `resolve_extends(def.to_raw_profile(), .., None, None, cli)` then `finalize_profile`. Delete `prepend_cli_extends`. + +- [ ] **Step 4: Run** `cargo test -p nono-cli --bin nono profile::` — all pass. + +- [ ] **Step 5: Refactor.** One place converts `&[String]` CLI bases into `Vec`; no leftover `cli_extends: &[String]` parameters below `load_profile_impl`. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(cli): resolve --extends paths against the cwd (#2065)`. + +### Task 4: Chain walker and `profile init --extends` + +**Files:** +- Modify: `crates/nono-cli/src/profile/mod.rs` — extract `fn locate_profile_file(name_or_path: &str) -> Option` from `load_profile_extends` (~2867); add `load_profile_extends_resolved`. +- Modify: `crates/nono-cli/src/package_status.rs:220` `walk_extends_chain` — call `load_profile_extends_resolved`. +- Modify: `crates/nono-cli/src/profile_cmd.rs:126-134` — `--extends` validation. + +**Interfaces:** +- Produces: `pub(crate) fn load_profile_extends_resolved(name_or_path: &str) -> Option>` — like `load_profile_extends`, but Path entries become canonical absolute path strings (entries that fail to classify are dropped; detection is best-effort and the real load reports the error). + +- [ ] **Step 1: Write failing tests:** + - `package_status.rs`: `claude_code_detection_follows_relative_path_extends` — `proj/.nono/agent.json` extends `"../shared/base.json"`, which extends `"nolabs-ai/claude"`; `selects_claude_code()` is true. + - `profile_cmd.rs`: `test_init_extends_relative_path_validates_against_output_dir` — `-o /proj/.nono/new.json --extends ../base.json` with `/proj/base.json` present succeeds and writes `"extends": "../base.json"` verbatim; with it absent, fails with the Task 1 missing-path message. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono package_status` and `profile_cmd::tests::test_init` — new tests fail. + +- [ ] **Step 3: Implement.** `locate_profile_file` mirrors `load_profile_extends`' lookup order (path, registry ref → pack store, user dir, pack store) and returns the canonical file; `load_profile_extends` uses it (built-ins keep their branch). In `cmd_init`, a base starting with `./`/`../` is checked with `classify_extends_entry(base, ExtendsOrigin::File(&output_path))`; other bases keep `profile_exists`. + +- [ ] **Step 4: Run** both commands — pass. + +- [ ] **Step 5: Refactor.** `load_profile_extends` and `load_profile_extends_resolved` share `locate_profile_file`; there is one copy of the lookup order. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(profile): follow relative extends in pack detection and profile init (#2065)`. + +### Task 5: Comment-preserving profile updates + +**Files:** +- Modify: `crates/nono-cli/Cargo.toml:98` → `features = ["serde", "cst"]` +- Create: `crates/nono-cli/src/profile_file_edit.rs` (+ `mod profile_file_edit;` in `main.rs`) +- Modify: `crates/nono-cli/src/profile_save_runtime.rs` — `PreparedProfileSave` (line 20), `write_profile` (730), `prepare_profile_save_from_patch` (1679) + +**Interfaces:** +- Produces: + ```rust + // profile_file_edit.rs + pub(crate) fn apply_patch_to_profile_text(text: &str, patch: &profile::Profile) -> Result; + // profile_save_runtime.rs + pub(crate) struct PreparedProfileSave { action, profile_name, profile_path, profile, patch: profile::Profile } + ``` + `write_profile`: `Created` → existing `serde_json` path; `Updated` → read file, `apply_patch_to_profile_text`, `profile::parse_profile_bytes` on the result (error → `LearnError`, file untouched), then `atomic_write`. + +- [ ] **Step 1: Write failing tests** in `profile_file_edit.rs`: + - `keeps_comments_and_appends_new_entries` — input with `// keep me` comments and `"read": ["/a"]`; patch read `["/a", "/b"]`; output contains `// keep me`, `"/b"` once, `"/a"` once. + - `creates_missing_sections` — `{ "meta": { "name": "x" } }` + patch on `filesystem.read_file` and `open_urls.allow_origins` → both created (Review Focus 3). + - `appends_to_empty_inline_array` — `"read": []` (Review Focus 3). + - `allow_localhost_only_flips_to_true` — existing `false` + patch `true` → `true`; existing `true` + patch `false` → stays `true`. + - `matches_merge_profile_patch` — for 4 representative patches (filesystem lists, `bypass_protection`, `unsafe_macos_seatbelt_rules`, `open_urls`): `serde_json::to_value(parse_profile_bytes(output))` equals `serde_json::to_value({ let mut p = parse_profile_bytes(input); merge_profile_patch(&mut p, &patch); p })`. + - `non_object_section_errors` — `"filesystem": []` returns `Err`. + In `profile_save_runtime.rs` tests: + - `write_profile_update_keeps_jsonc_comments` — `.jsonc` user profile with comments, update via `prepare_profile_save_from_patch` + `write_profile`; comments survive. + - `write_profile_update_invalid_file_left_unchanged` — file rewritten to `{ invalid` after prepare; `write_profile` errs; bytes unchanged (Review Focus 4). + - `write_profile_update_rejects_invalid_profile_left_unchanged` — file contains `"bogus_field": 1` (valid JSONC; `ProfileDeserialize` has `deny_unknown_fields`); the CST edit succeeds, the `parse_profile_bytes` re-check fails, `write_profile` errs, bytes unchanged. This pins the re-check itself. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile_file_edit` and `profile_save_runtime::tests::write_profile` — fail. + +- [ ] **Step 3: Implement** with `jsonc_parser::cst::CstRootNode::parse(text, &ParseOptions::default())`, `object_value_or_set`, `object_value_or_create(name)` / `array_value_or_create(name)` (`None` → `LearnError` naming the key), `CstArray::elements()` + `as_string_lit()?.decoded_value()` for de-dup, `CstArray::append(value.into())`, `root.to_string()`. Fields: `filesystem.{allow,read,write,allow_file,read_file,write_file,bypass_protection,suppress_save_prompt}`, top-level `unsafe_macos_seatbelt_rules`, `open_urls.allow_origins`, `open_urls.allow_localhost`. Add a comment in `merge_profile_patch` pointing at `apply_patch_to_profile_text` (the two field lists must match; `matches_merge_profile_patch` enforces it). + +- [ ] **Step 4: Run** both commands, then `cargo test -p nono-cli --bin nono profile_save_runtime` — all pass. + +- [ ] **Step 5: Refactor.** The patched fields are one table in `profile_file_edit.rs` (section, key, accessor), not repeated code per field; the comment in `merge_profile_patch` names it. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `fix(cli): keep comments when the save prompt updates a profile (#2065)`. + +### Task 6: Record profile source files and thread them to the save offer + +**Files:** +- Modify: `crates/nono-cli/src/profile/mod.rs` — `Profile` (~2576), `ProfileDeserialize` → `From` (~2805), `load_from_file`, `resolve_extends`, `load_base_profile_raw` pack branch, `merge_profiles` (~4103 next to `packs`) +- Modify: `crates/nono-cli/src/sandbox_prepare.rs` — `PreparedSandbox` (506) + its 3 constructors (~1552, ~1954, ~2938) +- Modify: `crates/nono-cli/src/launch_runtime.rs` — `ExecutionFlags` (229) + 307 +- Modify: `crates/nono-cli/src/exec_strategy.rs` — `ExecConfig` (~294), `ProfileSaveOffer` (~113), 1764 +- Modify: `crates/nono-cli/src/execution_runtime.rs:725` + +**Interfaces:** +- Produces: + ```rust + #[derive(Debug, Clone, PartialEq, Eq, Hash)] + pub(crate) struct ProfileSourceFile { pub(crate) path: PathBuf, pub(crate) kind: ProfileSourceKind } + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] + pub(crate) enum ProfileSourceKind { User, Draft, Project, Pack } + impl ProfileSourceFile { pub(crate) fn new(canonical: PathBuf) -> Self } // kind by Path::starts_with on canonical user/draft/pack dirs + impl ProfileSourceKind { pub(crate) fn is_writable(self) -> bool } // Pack → false + // Profile: #[serde(skip)] pub(crate) source_files: Vec (merge order: bases first, top-level last) + impl Profile { pub(crate) fn writable_source_files(&self) -> Vec } // writable only, reversed = precedence order + // ProfileSaveOffer / ExecConfig: profile_save_files: &'a [profile::ProfileSourceFile] + // ExecutionFlags / PreparedSandbox: profile_save_files: Vec + ``` + +- [ ] **Step 1: Write failing tests** (profile tests module): + - `test_source_files_record_kinds` — path profile `proj/agent.json` extends `["../shared/base.json", "", "default"]`; `source_files` kinds are `[Project(shared/base), Pack, Project(agent)]` in that order; no entry for `default`. + - `test_writable_source_files_precedence_order` — chain `top extends ["./a.json", "./b.json"]`, `a extends "./c.json"`; `writable_source_files()` paths are `[top, b, a, c]`. + - `test_source_files_draft_and_user_kinds` — a user profile loads as `User`; a draft path loads as `Draft`. + - `test_source_files_not_serialized` — `serde_json::to_value(&profile)` has no `source_files` key. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile::tests::test_source_files profile::tests::test_writable_source` — fail. + +- [ ] **Step 3: Implement.** Push `ProfileSourceFile::new(source_path)` in `load_from_file` after parsing, and onto each `ResolvedBase::Sibling` base before recursing; push a `Pack` entry (via `nono::try_canonicalize`) in the pack-store branches of `load_base_profile_raw`. `merge_profiles`: `source_files: dedup_append(&base.source_files, &child.source_files)`. Fix every `Profile { .. }` literal the compiler flags with `source_files: Vec::new()`. Set `PreparedSandbox.profile_save_files` from `loaded_profile.as_ref().map(Profile::writable_source_files).unwrap_or_default()` where `ignored_denial_paths` is set from the profile (~1954); `Vec::new()` at the other two constructors. Thread through exactly as `ignored_denial_paths` is threaded. + +- [ ] **Step 4: Run** `cargo test -p nono-cli --bin nono` — all pass (no behaviour change yet). + +- [ ] **Step 5: Refactor.** Kind detection lives only in `ProfileSourceFile::new`, reusing `is_under_pack_store` (Task 1) and `is_under_user_profile_draft_dir`. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(profile): record the files a loaded profile came from (#2065)`. + +### Task 7: Save to the profile file the session ran with + +**Files:** +- Modify: `crates/nono-cli/src/profile_save_runtime.rs` — `offer_save_run_profile` (260), `offer_url_only_save` (307), `offer_save_with_patch` (335), `offer_save_text_prompt` (382), `prompt_profile_save_choice` (547), `prepare_profile_save_from_patch` (1679) + +**Interfaces:** +- Consumes: Task 6 `offer.profile_save_files` (precedence order); Task 5 `PreparedProfileSave.patch`. +- Produces: + ```rust + enum SaveTarget { File(PathBuf), NewUserProfile } + fn save_targets(save_files: &[profile::ProfileSourceFile]) -> Vec; // files in order; [NewUserProfile] when empty + pub(crate) fn prepare_profile_save_to_file(patch: &profile::Profile, path: &Path, run_with: &str) -> Result; // action Updated + ``` + `run_with` is the `--profile` value to print in "Run with:" (the top-level `compared_profile`, not the base file). + +- [ ] **Step 1: Write failing tests:** + - `save_targets_empty_is_new_user_profile` — `[]` → `[NewUserProfile]`. + - `save_targets_files_never_include_new_user_profile` — two files → `[File(a), File(b)]`. + - `prepare_profile_save_to_file_updates_path_profile` — `proj/.nono/agent.json`; result `Updated`, path equals it, `profile_name == "./proj/.nono/agent.json"` as passed. + - `path_profile_symlinked_into_pack_store_not_offered` — a `ProfileSourceFile::new` for a symlink into the store has kind `Pack` and `writable_source_files` drops it (Review Focus 5). + - Existing `prepare_profile_save_from_patch_*` tests still pass. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile_save_runtime` — fail. + +- [ ] **Step 3: Implement.** Replace both `compared_profile.filter(... is_user_override)` branches with: `match save_targets(files).as_slice()` → `[SaveTarget::File(p)]` uses `prepare_profile_save_to_file` (selector: no extra question; text prompt: existing question with the full path in place of the name); `[NewUserProfile]` keeps the name prompt + `prepare_profile_save_from_patch`; 2+ files → for this task take the first (Task 8 adds the menu). Change the `prompt_profile_save_choice` wording from `user profile '{name}'` to `profile '{path}'`. + +- [ ] **Step 4: Run** `cargo test -p nono-cli --bin nono profile_save_runtime` — pass. + +- [ ] **Step 5: Refactor.** The selector and text-prompt flows share one helper that writes to a `SaveTarget`; no duplicated `prepare` + `write_profile` + `print_profile_save` sequences. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `fix(cli): let the save prompt update a --profile path (#2065)`. + +### Task 8: Save-target menu + +**Files:** +- Modify: `crates/nono-cli/src/profile_save_runtime.rs` + +**Interfaces:** +- Consumes: Task 7 `SaveTarget`, `save_targets`, `prepare_profile_save_to_file`. +- Produces: + ```rust + fn render_save_target_menu(files: &[PathBuf]) -> String; + fn parse_save_target_choice(input: &str, count: usize) -> Option>; // Some(Some(i)) pick, Some(None) skip, None invalid + fn chosen_save_target(files: &[PathBuf], input: &str) -> Option>; // parse + index into files + fn prompt_save_target(files: &[PathBuf]) -> Result>; // loops on invalid input; uses chosen_save_target + ``` + +- [ ] **Step 1: Write failing tests:** + - `render_menu_labels_top_and_bases` — two paths; output equals + ``` + Save the selected rules to: + 1) (this profile) + 2) (base — applies to every profile that extends it) + Choice [1]: + ``` + (column-align the labels; the test asserts each line's prefix, path and label). + - `parse_choice_enter_is_first` — `""` → `Some(Some(0))`; `"2"` → `Some(Some(1))`; `" skip "` → `Some(None)`; `"3"` with count 2 → `None`; `"x"` → `None`. + - `menu_choice_two_writes_base_only` — temp `top.json` and `base.json` (in precedence order); `chosen_save_target(&files, "2")` → `Some(Some(base))`; `prepare_profile_save_to_file(&patch, &base, "./top.json")` + `write_profile`; `base.json` gains the patch entry and `top.json`'s bytes are unchanged. + +- [ ] **Step 2: Run** `cargo test -p nono-cli --bin nono profile_save_runtime::tests::render_menu profile_save_runtime::tests::parse_choice profile_save_runtime::tests::menu_choice` — fail. + +- [ ] **Step 3: Implement.** In Task 7's 2+ files arm, call `prompt_save_target` after the selector/override confirmation and before writing, for both the selector and text-prompt flows (`prompt_print`/`read_input_line`; invalid input prints "Enter a number from 1 to N, press Enter for 1, or type skip." in red). `skip` writes nothing. + +- [ ] **Step 4: Run** `cargo test -p nono-cli --bin nono profile_save_runtime` — pass. + +- [ ] **Step 5: Refactor.** Both flows call `prompt_save_target`; `prompt_save_target` is a thin loop over `chosen_save_target`. Re-run the Step 4 command — still passes. + +- [ ] **Step 6: Commit** `feat(cli): choose which profile file the save prompt updates (#2065)`. + +### Task 9: Docs and full verification + +**Files:** +- Modify: `docs/cli/features/profile-authoring.mdx` — "How `extends` Works" (~35) and the post-run save prompt text (~460) + +- [ ] **Step 1:** Document, leading with a project profile (`--profile ./.nono/agent.json`) that has `"extends": "claude-code"` plus `"../shared/base.json"`: the `./`/`../` rule, extension requirement, the error cases from Global Constraints, CLI `--extends` resolving from the cwd. In the save section: which files can be updated, the menu, precedence order, packs never offered, comments kept. +- [ ] **Step 2: Run** `make fmt`, then `make ci` and `make clippy` — both end without errors (`CI checks passed`). +- [ ] **Step 3: Grep-verify** each task's claim on the current tree: `grep -n "fn classify_extends_entry" crates/nono-cli/src/profile/extends_ref.rs`, `grep -n "prepend_cli_extends" -r crates/nono-cli/src` (no hits), `grep -n '"cst"' crates/nono-cli/Cargo.toml`, `grep -n "source_files" crates/nono-cli/src/profile/mod.rs`, `grep -n "fn prompt_save_target" crates/nono-cli/src/profile_save_runtime.rs`. +- [ ] **Step 4: Commit** `docs(profile): document relative extends and save targets (#2065)`. diff --git a/docs/superpowers/specs/2026-10-06-relative-extends-design.md b/docs/superpowers/specs/2026-10-06-relative-extends-design.md new file mode 100644 index 000000000..273b86ae1 --- /dev/null +++ b/docs/superpowers/specs/2026-10-06-relative-extends-design.md @@ -0,0 +1,288 @@ +# Relative paths in `extends`, and save to an extended profile — design + +Issue: [nolabs-ai/nono#2065](https://github.com/nolabs-ai/nono/issues/2065). +Both phases ship in one PR. Phase 1 commits come first; phase 2 builds on the +phase 1 writer and path-profile save. + +## Goal + +A profile can extend another profile file by a path relative to itself, so a +project can keep a shared base profile anywhere in its tree. Also fix two bugs +in the save-on-exit flow that block path-based profiles: + +- Bug 3: `--profile ./x.json` cannot be updated on exit. +- Bug 4: updating a profile rewrites it with `serde_json` and strips comments. + +Phase 2: on exit, the user can save grants to any writable profile file in the +resolved `extends` chain, not only the top-level profile. Pack and built-in +profiles are never offered. + +## Entry syntax + +Each `extends` entry written in a profile file is classified once, in this +order. CLI `--extends` entries follow the same table, except that a Path entry +resolves against the cwd (see "CLI `--extends`" below). + +| Entry | Kind | Rule | +|---|---|---| +| starts with `./` or `../` | Path | Must end in `.json` or `.jsonc`. | +| starts with `/` or `~` | — | Error: "absolute and `~/` paths are not supported in `extends`; use a relative path". | +| `org/name[@ver]` (`is_registry_ref`) | Registry | Unchanged. | +| valid profile name | Name | Unchanged lookup: sibling `.json`, user dir, pack store, built-in. | +| anything else | — | Error: "invalid base profile name" (unchanged). | + +`shared/base.json` (no `./`) is not a path. It stays an invalid name, as today. +This keeps the rule to one check and stops it colliding with registry refs. + +## Path resolution + +1. Base directory: + - For an entry in a file: the parent of the canonical path of that file. + `load_from_file` already canonicalizes, so this is the existing + `context_dir`. + - For a CLI `--extends` entry: the current working directory. +2. Join, then `canonicalize`. A missing file is an error naming the entry and + the resolved path. There is no fallback to name lookup. +3. Reject when the canonical target is inside the pack store + (`package_store_dir()`, canonicalized; `Path::starts_with`). Message: + "extend a pack by name (`org/pack`), not by path". Reason: a path load skips + the pack bookkeeping (verification list, session hook provenance, + `$PACK_DIR`). +4. Load the file with `parse_file_backed_profile`. Return it as + `ResolvedBase::Sibling(profile, canonical_path)`. Its own entries then + resolve relative to its directory, and its bare names find siblings there. + +### Where path entries are rejected + +These rules apply to entries written in a profile file. They never apply to CLI +`--extends` entries. + +- In a profile loaded from the pack store: any Path entry is an error. A pack + must not load files outside itself. +- In a profile under `profile-drafts/`: any Path entry is an error, because + `profile promote` moves the file and changes what the path means. +- In a built-in profile: no file context exists. Treat as an error (cannot + happen with shipped policy, but fail closed). + +### Cycle detection + +`visited` holds a key per entry: the name for Name/Registry entries, and the +canonical path for Path entries. So `./a.json` and `../x/a.json` that point at +the same file are one node. `MAX_INHERITANCE_DEPTH` is unchanged. + +## Callers that must understand Path entries + +- `resolve_extends` / `load_base_profile_raw` — the main change. +- `prepend_cli_extends` — see "CLI `--extends`" below. +- `package_status::walk_extends_chain` (Claude Code / official pack detection) + — it walks raw strings without a directory. It must resolve Path entries + relative to the declaring file, using the same classify-and-resolve function. + It must not duplicate the rules. +- `profile init --extends ` — write the entry as given. Validate it + relative to the output file's directory, because that is what it means when + loaded. +- `profile show` / `diff` / `validate` / `why` — show the raw entry as written. + They resolve through the shared loader, so no extra logic. + +### CLI `--extends` + +Today `prepend_cli_extends` inserts CLI bases into the selected profile's own +`extends` list (`load_from_file`, the pack-store branch, and the built-in +branch of `load_profile_inner`). That would put a CLI path inside a pack or +built-in profile's list, where path entries are rejected, and the resolved +absolute path would hit the "no absolute paths" rule. + +Change: CLI bases do not go into `profile.extends`. Classify and resolve them +first (Path entries against the cwd, to a canonical path), then pass the +resolved entries to the resolver as a separate list that is merged ahead of the +profile's own bases, in the same position they take today. The pack-store, +built-in and "no absolute paths" rules check only entries written in a file. + +So `nono run --profile claude-code --extends ./project-extra.json` works. The +main pattern is still a project profile with `"extends": "claude-code"`, run +as `--profile ./.nono/agent.json`; that needs no CLI change. + +One function owns classification and resolution +(`classify_extends_entry(raw, origin: ExtendsOrigin) -> Result`, where +`ExtendsOrigin` is `File(declaring_file)`, `Cli(cwd)` or `Builtin`; the origin +selects the base directory and which rejection rules apply). +Every caller above uses it. + +## Bug 3: update a path profile on exit + +When `profile_save_base` is a file path (`is_file_path_ref`): + +- Canonicalize it. If it is inside the pack store, do not offer an update; keep + the current new-user-profile flow. +- Otherwise update that file, exactly like the existing named-user-profile + branch: the interactive selector writes without a further question (the item + selection is the confirmation), and the text prompt asks its existing + "Update …? [g/s/Enter]" question. In both, the prompt and the + `print_profile_save` output show the full path, because the file can be in a + git repo. + +`prepare_profile_save_from_patch` takes a name today. Add a path-based entry +point; keep the name-based one for user profiles. + +## Bug 4: keep comments when updating + +Enable the `cst` feature of `jsonc-parser` (already a dependency, v0.34). For +the `Updated` action only: + +1. Read the original text. Parse with `CstRootNode::parse`. +2. For each field that `merge_profile_patch` touches, append values that are + not already present as plain strings: + `filesystem.{allow,read,write,allow_file,read_file,write_file,bypass_protection,suppress_save_prompt}`, + `unsafe_macos_seatbelt_rules`, `open_urls.allow_origins`. Use + `object_value_or_create` / `array_value_or_create` / `CstArray::append`. +3. `open_urls.allow_localhost`: set to `true` only if the patch sets it (same + monotonic rule). +4. Re-parse the output with `parse_profile_bytes` before the atomic write. If + it fails, return an error and leave the file unchanged. + +New profiles (`Created`) keep the `serde_json` writer; they have no comments. + +The CST write is the only writer for updates, so the set of patched fields +lives in two places (`merge_profile_patch` and the CST glue). A test pins them +together (below). + +## Phase 2: choose the save target on exit + +### Recording where each layer came from + +Add `source_files: Vec` to `Profile` (`#[serde(skip)]`, not +part of the file format). It is filled at load and merged in `merge_profiles` +the same way as `packs`. Each entry has the canonical path and a kind: + +| Kind | How it is detected | Writable | +|---|---|---| +| User | under `user_profile_dir()` | yes | +| Draft | under `user_profile_draft_dir()` | yes | +| Project | any other file (path entry, `--profile `, sibling) | yes | +| Pack | under `package_store_dir()` | no — verification fails on change and `nono pull` replaces it | +| Built-in | no file | not listed | + +Detection uses `Path::starts_with` on canonical paths. The list is recorded at +load, so the save offer uses the files the session actually ran with. It is +not re-walked at exit. + +The loaded profile's `source_files` (writable entries only) is passed to +`ProfileSaveOffer` next to `profile_save_base`. + +### Save flow + +1. The session ends with denials. nono builds the patch (no change). +2. The user selects items in the selector or the text prompt (no change). + The `override` confirmation stays where it is. +3. Build the target list: + - Writable source files in precedence order: the reverse of the recorded + merge order. The top-level profile is first; then the base whose values + win, down to the base that wins least. For `extends: ["a", "b"]` where + `a` extends `c`, the merge records `[c, a, b, top]` and the menu shows + `top, b, a, c`. Duplicates are already removed by the merge. + - "a new user profile", only when the list of writable files is empty + (today's rule: a pack, built-in or no profile). So a named user profile + with no writable bases saves exactly as today, with no menu. A new user + profile is never offered for a path profile: it cannot extend one + (absolute paths are out of scope), so it would drop the user's base. +4. If the list has one entry, behave as today: update that file, or prompt for + a new user profile name. +5. If the list has two or more entries, show a numbered menu. The same menu is + used after the interactive selector and in the text prompt (the selector + has already left raw mode). + + ``` + Save the selected rules to: + 1) /path/proj/.nono/agent.json (this profile) + 2) /path/proj/shared/base.json (base — applies to every profile that extends it) + Choice [1]: + ``` + + - Enter selects 1. `skip` cancels. Invalid input prints help and asks again. + - Paths are shown in full: a project file can be in a git repo. +6. Write to the chosen file with the phase 1 CST writer, or run the existing + new-user-profile flow. + +No terminal (`terminal_prompts_available()` is false): no prompt, as today. + +### Phase 2 tests + +- Source recording: user, draft, project, pack, built-in layers get the right + kind; a pack layer is never in the writable list; merge keeps all layers. +- Target list order: precedence order (`top, b, a, c` for the example in the + save flow); duplicates removed. +- "a new user profile" is the only target when no writable file exists, and is + never listed next to writable files. +- One target: no menu, current behaviour. +- Menu: Enter picks the top-level profile; `2` writes to the base and leaves the + top-level file unchanged; `skip` writes nothing; invalid input re-prompts. +- Writing to a base keeps its comments (phase 1 writer). + +## Out of scope + +- Absolute and `~/` paths (reasons in the issue). +- `.json` vs `.jsonc` precedence in bare-name sibling lookup. Today sibling + lookup checks only `.json`, and the user dir prefers `.jsonc`. Changing that + alters existing resolution; it needs its own issue. +- Choosing a different save target per item. + +## Security notes + +- A path can reach any file the user can read. This adds no trust: the + top-level profile can already grant the same access directly. +- Packs cannot use paths, and paths cannot reach into packs, so pack + verification and hook provenance stay intact. +- Paths are compared with `Path::starts_with` on canonical paths, never string + prefixes. Canonicalizing at load also removes `..` and symlink tricks from + cycle detection. +- TOCTOU: the file is canonicalized and read once per resolution, the same as + sibling lookup today. +- A grant saved to a shared base applies to every profile that extends it. The + menu says so, and the default is the top-level profile. +- Pack files are never write targets, so a save cannot break pack + verification. +- Linux and macOS: no change to enforcement. The resolved profile goes through + the same merge and validation. + +## Tests + +Resolution (`profile/mod.rs` tests, using temp dirs and `with_config_env`): + +- `./base.json` and `../shared/base.json` resolve and merge. +- A path base with a bare-name `extends` finds its own sibling. +- A symlinked directory resolves via the canonical path. +- Cycle through paths (`a -> ./b.json -> ./a.json`) errors; the same file + reached by two spellings is detected as a cycle. +- Missing file, missing extension, `/abs.json`, `~/x.json` each error with the + documented message. +- Path entry in a pack-store profile errors; path entry in a draft errors; + path target inside the pack store errors. +- CLI `--extends ./x.json` resolves against the cwd, not the profile's + directory. +- CLI `--extends ./x.json` works with a pack-store top-level profile and with a + built-in top-level profile. +- `walk_extends_chain` follows a Path entry to the Claude pack. +- `profile init --extends ../base.json` validates relative to the output file. + +Save: + +- `--profile ./x.json` run updates that file: no extra question after the + interactive selector; the existing question in the text prompt; the full path + is printed. +- A path profile inside the pack store is not offered for update. +- Updating a `.jsonc` profile keeps comments and layout; only new entries are + added. +- Equivalence: for a set of patches, `parse(cst_write(original))` equals + `merge_profile_patch(parse(original), patch)`. This pins the CST field list + to `merge_profile_patch`. +- A file that is not valid JSON at save time: error, file unchanged. +- A file that is valid JSONC but not a valid profile (unknown field): the edit + succeeds, the re-parse in step 4 fails, error, file unchanged. + +## Docs + +Update `docs/cli/features/profile-authoring.mdx` ("How `extends` Works") with +path entries, the rules, and the error cases. Lead with the main pattern: a +project profile that extends the harness profile (`"extends": "claude-code"`). Update the post-run save prompt +text near line 460 of the same page with the save-target menu and which files +it offers.