From f48a83a500d7d5cb6fd34b0b87e7fe347b081cbc Mon Sep 17 00:00:00 2001 From: David Abram Date: Mon, 3 Aug 2026 18:52:50 +0200 Subject: [PATCH 1/2] setup: Replace directory-level install with per-asset merge-aware install sce setup previously staged the full catalog into a temp directory and swapped it over the whole .claude/, .opencode/, or .pi/ target directory, destroying any user-owned files (skills, settings.local.json, CLAUDE.md, etc.) living alongside SCE's own assets. The two generated JSON configs (.claude/settings.json, .opencode/opencode.json) had the same problem one level down: written whole, they clobbered a user's permissions, env, model, mcp, or non-SCE hook/plugin entries. Installation now happens per asset: each embedded file is staged and atomically renamed into place individually, and assets the current selection or catalog no longer owns are pruned by relative path instead of the whole directory being rebuilt. The two config files are merged via a new config_merge module that replaces only the SCE-owned fragment (hook entries by marker, plugin paths by prefix) and leaves every other key and entry untouched, idempotently across repeated installs. sce doctor's integration inspection is updated to match: the two merge-target configs are checked by whether their SCE-owned fragment is current rather than by byte-exact sha256, and `--fix` gained a repair path that reinstalls just a drifted merge-target asset. Co-authored-by: SCE --- cli/src/services/doctor/inspect.rs | 376 +++++++++- cli/src/services/doctor/mod.rs | 3 +- cli/src/services/setup/config_merge.rs | 530 ++++++++++++++ cli/src/services/setup/mod.rs | 667 +++++++++++++++--- context/architecture.md | 2 +- context/cli/cli-command-surface.md | 2 +- context/context-map.md | 2 +- context/glossary.md | 6 +- context/overview.md | 2 +- context/patterns.md | 5 +- .../plans/non-destructive-setup-install.md | 242 +++++++ context/sce/doctor-human-text-contract.md | 5 +- context/sce/setup-no-backup-policy-seam.md | 39 +- .../sce/setup-repo-local-config-bootstrap.md | 2 +- 14 files changed, 1736 insertions(+), 147 deletions(-) create mode 100644 cli/src/services/setup/config_merge.rs create mode 100644 context/plans/non-destructive-setup-install.md diff --git a/cli/src/services/doctor/inspect.rs b/cli/src/services/doctor/inspect.rs index f8a83bd2..a2860344 100644 --- a/cli/src/services/doctor/inspect.rs +++ b/cli/src/services/doctor/inspect.rs @@ -15,18 +15,18 @@ use crate::services::repository_identity::resolve::{ resolve_repository_identity, RepositoryIdentitySource, }; use crate::services::setup::{ - iter_embedded_assets_for_setup_target_with_selection, iter_required_hook_assets, - persisted_optional_workflows, EmbeddedAsset, SetupTarget, + config_merge, iter_embedded_assets_for_setup_target_with_selection, iter_required_hook_assets, + persisted_optional_workflows, repair_merge_target_asset, EmbeddedAsset, SetupTarget, }; use super::types::{ - AgentTraceDbHealth, CheckoutIdentityHealth, DoctorProblem, FileLocationHealth, - GlobalStateHealth, HookContentState, HookDoctorReport, HookFileHealth, HookPathSource, - IntegrationChildHealth, IntegrationContentState, IntegrationGroupHealth, ProblemCategory, - ProblemFixability, ProblemKind, ProblemSeverity, Readiness, CLAUDE_AGENTS_LABEL, - CLAUDE_COMMANDS_LABEL, CLAUDE_PLUGINS_LABEL, CLAUDE_SKILLS_LABEL, OPENCODE_AGENTS_LABEL, - OPENCODE_COMMANDS_LABEL, OPENCODE_PLUGINS_LABEL, OPENCODE_SKILLS_LABEL, PI_EXTENSIONS_LABEL, - PI_PROMPTS_LABEL, PI_SKILLS_LABEL, + AgentTraceDbHealth, CheckoutIdentityHealth, DoctorFixResultRecord, DoctorProblem, + FileLocationHealth, FixResult, GlobalStateHealth, HookContentState, HookDoctorReport, + HookFileHealth, HookPathSource, IntegrationChildHealth, IntegrationContentState, + IntegrationGroupHealth, ProblemCategory, ProblemFixability, ProblemKind, ProblemSeverity, + Readiness, CLAUDE_AGENTS_LABEL, CLAUDE_COMMANDS_LABEL, CLAUDE_PLUGINS_LABEL, + CLAUDE_SKILLS_LABEL, OPENCODE_AGENTS_LABEL, OPENCODE_COMMANDS_LABEL, OPENCODE_PLUGINS_LABEL, + OPENCODE_SKILLS_LABEL, PI_EXTENSIONS_LABEL, PI_PROMPTS_LABEL, PI_SKILLS_LABEL, }; use super::{is_executable, DoctorDependencies, DoctorMode, REQUIRED_HOOKS}; @@ -515,6 +515,81 @@ fn inspect_repository_integrations( integration_groups } +/// Repairs each merge-target asset (`.claude/settings.json`, +/// `.opencode/opencode.json`) whose SCE-owned fragment is currently missing or +/// stale, by reinstalling just that asset through the same merge-install path +/// `sce setup` uses. Assets whose fragment is already current are left +/// untouched, and a fully missing integration is left to the existing +/// "reinstall assets" guidance rather than being created here. +pub(super) fn repair_merge_target_configs(repository_root: &Path) -> Vec { + let targets = resolve_doctor_integration_targets(repository_root); + let selected_optional_workflows = persisted_optional_workflows(repository_root); + let mut results = Vec::new(); + + if targets.contains(&IntegrationTargetId::Claude) { + let claude_groups = + collect_claude_integration_groups(repository_root, &selected_optional_workflows); + if let Some(result) = repair_merge_target_if_mismatched( + repository_root, + SetupTarget::Claude, + claude_asset::SETTINGS_FILE, + &claude_groups, + ) { + results.push(result); + } + } + + if targets.contains(&IntegrationTargetId::Opencode) { + let opencode_groups = + collect_opencode_integration_groups(repository_root, &selected_optional_workflows); + if let Some(result) = repair_merge_target_if_mismatched( + repository_root, + SetupTarget::OpenCode, + OPENCODE_CONFIG_RELATIVE_PATH, + &opencode_groups, + ) { + results.push(result); + } + } + + results +} + +fn repair_merge_target_if_mismatched( + repository_root: &Path, + target: SetupTarget, + relative_path: &str, + groups: &[IntegrationGroupHealth], +) -> Option { + let is_mismatched = groups + .iter() + .flat_map(|group| &group.children) + .any(|child| { + child.relative_path == relative_path + && matches!(child.content_state, IntegrationContentState::Mismatch) + }); + if !is_mismatched { + return None; + } + + Some( + match repair_merge_target_asset(repository_root, target, relative_path) { + Ok(()) => DoctorFixResultRecord { + category: ProblemCategory::RepoAssets, + outcome: FixResult::Fixed, + detail: format!("Merged canonical SCE fragments into '{relative_path}'."), + }, + Err(error) => DoctorFixResultRecord { + category: ProblemCategory::RepoAssets, + outcome: FixResult::Failed, + detail: format!( + "Failed to merge canonical SCE fragments into '{relative_path}': {error}" + ), + }, + }, + ) +} + #[allow(dead_code)] fn collect_global_state_health( repository_root: &Path, @@ -1167,18 +1242,24 @@ fn collect_opencode_integration_groups( let manifest_child = embedded_assets .iter() - .find(|asset| asset.relative_path == "opencode.json") + .find(|asset| asset.relative_path == OPENCODE_CONFIG_RELATIVE_PATH) .map_or_else( || build_integration_child_presence_only("opencode.json", &manifest_path), - |asset| build_integration_child_from_asset(&opencode_root, asset), + |asset| { + build_integration_child_from_asset( + &opencode_root, + asset, + Some(&MergeTargetAsset::OpenCodeConfig), + ) + }, ); plugin_children.push(manifest_child); for asset in embedded_assets { - if asset.relative_path == "opencode.json" { + if asset.relative_path == OPENCODE_CONFIG_RELATIVE_PATH { continue; } - let child = build_integration_child_from_asset(&opencode_root, asset); + let child = build_integration_child_from_asset(&opencode_root, asset, None); if child .relative_path @@ -1248,7 +1329,12 @@ fn collect_claude_integration_groups( let mut skill_children = Vec::new(); for asset in embedded_assets { - let child = build_integration_child_from_asset(&claude_root, asset); + let merge_target = if asset.relative_path == claude_asset::SETTINGS_FILE { + Some(&MergeTargetAsset::ClaudeSettings) + } else { + None + }; + let child = build_integration_child_from_asset(&claude_root, asset, merge_target); if child.relative_path == claude_asset::SETTINGS_FILE || child @@ -1315,7 +1401,7 @@ fn collect_pi_integration_groups( let mut extension_children = Vec::new(); for asset in embedded_assets { - let child = build_integration_child_from_asset(&pi_root, asset); + let child = build_integration_child_from_asset(&pi_root, asset, None); if child .relative_path @@ -1359,12 +1445,36 @@ fn sort_integration_children(children: &mut [IntegrationChildHealth]) { children.sort_by(|left, right| left.relative_path.cmp(&right.relative_path)); } +/// The relative path of the `OpenCode` merge-target asset within `.opencode/`. +const OPENCODE_CONFIG_RELATIVE_PATH: &str = "opencode.json"; + +/// Identifies the two setup assets that are installed by JSON merge +/// (`config_merge`) rather than whole-file replacement, and therefore need +/// SCE-fragment-based content inspection instead of byte-exact `sha256`. +enum MergeTargetAsset { + ClaudeSettings, + OpenCodeConfig, +} + fn build_integration_child_from_asset( integration_root: &Path, asset: &EmbeddedAsset, + merge_target: Option<&MergeTargetAsset>, ) -> IntegrationChildHealth { let path = integration_root.join(asset.relative_path); - let content_state = inspect_integration_asset_state(&path, &asset.sha256); + let content_state = match merge_target { + Some(MergeTargetAsset::ClaudeSettings) => inspect_merge_target_asset_state( + &path, + asset.bytes, + config_merge::claude_settings_fragment_is_current, + ), + Some(MergeTargetAsset::OpenCodeConfig) => inspect_merge_target_asset_state( + &path, + asset.bytes, + config_merge::opencode_config_fragment_is_current, + ), + None => inspect_integration_asset_state(&path, &asset.sha256), + }; IntegrationChildHealth { relative_path: asset.relative_path.to_string(), path, @@ -1372,6 +1482,33 @@ fn build_integration_child_from_asset( } } +/// Content state for a merge-target asset: `Match` when the existing file +/// already carries a current, complete copy of the SCE-owned fragment +/// alongside whatever else it holds; `Mismatch` when that fragment is absent +/// or stale, or when the existing file cannot be parsed as JSON (a merge +/// cannot succeed either way, so both drift and hard-error surface the same +/// remediation: reinstall/`sce doctor --fix`). +fn inspect_merge_target_asset_state( + path: &Path, + generated_bytes: &[u8], + fragment_is_current: fn(&[u8], &[u8]) -> anyhow::Result, +) -> IntegrationContentState { + if !path_is_file(path) { + return IntegrationContentState::Missing; + } + + match fs::read(path) { + Ok(existing_bytes) => { + if fragment_is_current(&existing_bytes, generated_bytes).unwrap_or(false) { + IntegrationContentState::Match + } else { + IntegrationContentState::Mismatch + } + } + Err(error) => IntegrationContentState::ReadFailed(error.to_string()), + } +} + fn build_integration_child_presence_only( relative_path: &str, path: &Path, @@ -1598,4 +1735,211 @@ mod tests { "a selected optional workflow's missing file was not reported" ); } + + fn unique_temp_repository_root(label: &str) -> PathBuf { + let nonce = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system time should be after Unix epoch") + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "sce-doctor-merge-target-{label}-{}-{nonce}", + std::process::id() + )); + std::fs::create_dir_all(&dir).expect("create temp repository root"); + dir + } + + fn embedded_claude_settings_bytes() -> &'static [u8] { + crate::services::setup::iter_embedded_assets_for_setup_target_with_selection( + crate::services::setup::SetupTarget::Claude, + &[] as &[String], + ) + .find(|asset| { + asset.relative_path == crate::services::default_paths::claude_asset::SETTINGS_FILE + }) + .expect("embedded Claude catalog carries settings.json") + .bytes + } + + fn embedded_opencode_config_bytes() -> &'static [u8] { + crate::services::setup::iter_embedded_assets_for_setup_target_with_selection( + crate::services::setup::SetupTarget::OpenCode, + &[] as &[String], + ) + .find(|asset| asset.relative_path == "opencode.json") + .expect("embedded OpenCode catalog carries opencode.json") + .bytes + } + + #[test] + fn claude_settings_reports_match_despite_extra_user_permissions() { + let root = unique_temp_repository_root("claude-pass"); + let claude_dir = root.join(".claude"); + std::fs::create_dir_all(&claude_dir).unwrap(); + + let generated_bytes = embedded_claude_settings_bytes(); + let installed_bytes = + crate::services::setup::config_merge::merge_or_create_claude_settings( + None, + generated_bytes, + "settings.json", + ) + .unwrap(); + let mut installed: serde_json::Value = serde_json::from_slice(&installed_bytes).unwrap(); + installed["permissions"] = serde_json::json!({"allow": ["Bash(git *)"]}); + std::fs::write( + claude_dir.join("settings.json"), + serde_json::to_vec_pretty(&installed).unwrap(), + ) + .unwrap(); + + let groups = collect_claude_integration_groups(&root, &[]); + let settings_child = groups + .iter() + .flat_map(|group| &group.children) + .find(|child| child.relative_path == "settings.json") + .expect("settings.json child present"); + assert!(matches!( + settings_child.content_state, + IntegrationContentState::Match + )); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn claude_settings_reports_mismatch_when_sce_hook_entry_deleted_then_fix_repairs_it() { + let root = unique_temp_repository_root("claude-fix"); + let claude_dir = root.join(".claude"); + std::fs::create_dir_all(&claude_dir).unwrap(); + + let generated_bytes = embedded_claude_settings_bytes(); + let installed_bytes = + crate::services::setup::config_merge::merge_or_create_claude_settings( + None, + generated_bytes, + "settings.json", + ) + .unwrap(); + let mut drifted: serde_json::Value = serde_json::from_slice(&installed_bytes).unwrap(); + drifted["permissions"] = serde_json::json!({"allow": ["Bash(git *)"]}); + // Drop every hook event's entries to simulate a deleted SCE hook entry. + for (_, entries) in drifted["hooks"].as_object_mut().unwrap() { + *entries = serde_json::json!([]); + } + let settings_path = claude_dir.join("settings.json"); + std::fs::write(&settings_path, serde_json::to_vec_pretty(&drifted).unwrap()).unwrap(); + + let groups = collect_claude_integration_groups(&root, &[]); + let settings_child = groups + .iter() + .flat_map(|group| &group.children) + .find(|child| child.relative_path == "settings.json") + .expect("settings.json child present"); + assert!(matches!( + settings_child.content_state, + IntegrationContentState::Mismatch + )); + + let fix_results = super::repair_merge_target_configs(&root); + assert!( + fix_results + .iter() + .any(|result| matches!(result.outcome, super::FixResult::Fixed)), + "expected the drifted settings.json to be repaired" + ); + + let repaired: serde_json::Value = + serde_json::from_slice(&std::fs::read(&settings_path).unwrap()).unwrap(); + assert_eq!(repaired["permissions"]["allow"][0], "Bash(git *)"); + + let groups_after_fix = collect_claude_integration_groups(&root, &[]); + let settings_child_after_fix = groups_after_fix + .iter() + .flat_map(|group| &group.children) + .find(|child| child.relative_path == "settings.json") + .expect("settings.json child present"); + assert!(matches!( + settings_child_after_fix.content_state, + IntegrationContentState::Match + )); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn opencode_config_reports_match_despite_extra_user_plugin_then_drift_and_fix() { + let root = unique_temp_repository_root("opencode-fix"); + let opencode_dir = root.join(".opencode"); + std::fs::create_dir_all(&opencode_dir).unwrap(); + + let generated_bytes = embedded_opencode_config_bytes(); + let installed_bytes = + crate::services::setup::config_merge::merge_or_create_opencode_config( + None, + generated_bytes, + "opencode.json", + ) + .unwrap(); + let mut installed: serde_json::Value = serde_json::from_slice(&installed_bytes).unwrap(); + installed["model"] = serde_json::json!("anthropic/claude"); + installed["plugin"] + .as_array_mut() + .unwrap() + .insert(0, serde_json::json!("./plugins/my-plugin.ts")); + let manifest_path = opencode_dir.join("opencode.json"); + std::fs::write( + &manifest_path, + serde_json::to_vec_pretty(&installed).unwrap(), + ) + .unwrap(); + + let groups = collect_opencode_integration_groups(&root, &[]); + let manifest_child = groups + .iter() + .flat_map(|group| &group.children) + .find(|child| child.relative_path == "opencode.json") + .expect("opencode.json child present"); + assert!(matches!( + manifest_child.content_state, + IntegrationContentState::Match + )); + + // Drop the plugin array entirely to simulate a stale/removed SCE registration. + installed["plugin"] = serde_json::json!(["./plugins/my-plugin.ts"]); + std::fs::write( + &manifest_path, + serde_json::to_vec_pretty(&installed).unwrap(), + ) + .unwrap(); + + let drifted_groups = collect_opencode_integration_groups(&root, &[]); + let drifted_child = drifted_groups + .iter() + .flat_map(|group| &group.children) + .find(|child| child.relative_path == "opencode.json") + .expect("opencode.json child present"); + assert!(matches!( + drifted_child.content_state, + IntegrationContentState::Mismatch + )); + + let fix_results = super::repair_merge_target_configs(&root); + assert!( + fix_results + .iter() + .any(|result| matches!(result.outcome, super::FixResult::Fixed)), + "expected the drifted opencode.json to be repaired" + ); + + let repaired: serde_json::Value = + serde_json::from_slice(&std::fs::read(&manifest_path).unwrap()).unwrap(); + assert_eq!(repaired["model"], "anthropic/claude"); + let plugin = repaired["plugin"].as_array().unwrap(); + assert!(plugin.contains(&serde_json::json!("./plugins/my-plugin.ts"))); + assert!(plugin.contains(&serde_json::json!("./plugins/sce-bash-policy.ts"))); + assert!(plugin.contains(&serde_json::json!("./plugins/sce-agent-trace.ts"))); + + std::fs::remove_dir_all(&root).ok(); + } } diff --git a/cli/src/services/doctor/mod.rs b/cli/src/services/doctor/mod.rs index 16a877da..30a2ca85 100644 --- a/cli/src/services/doctor/mod.rs +++ b/cli/src/services/doctor/mod.rs @@ -21,7 +21,7 @@ pub(crate) mod types; pub mod command; use fixes::build_manual_fix_results; -use inspect::build_report_with_lifecycle_problems; +use inspect::{build_report_with_lifecycle_problems, repair_merge_target_configs}; use render::render_report; use types::{ DoctorFixResultRecord, DoctorProblem, FixResult, HookDoctorReport, ProblemCategory, @@ -129,6 +129,7 @@ fn execute_doctor_with_lifecycle_providers( } let mut fix_results = fix_lifecycle_providers(context, &providers, &initial_problems); + fix_results.extend(repair_merge_target_configs(repository_root)); let final_problems = diagnose_lifecycle_providers(context, &providers); let final_doctor_problems = final_problems .into_iter() diff --git a/cli/src/services/setup/config_merge.rs b/cli/src/services/setup/config_merge.rs new file mode 100644 index 00000000..b2340342 --- /dev/null +++ b/cli/src/services/setup/config_merge.rs @@ -0,0 +1,530 @@ +//! Pure JSON merge for setup-installed config files that a user may already own +//! and extend. Two known shapes today: Claude's `.claude/settings.json` hook +//! registry and `OpenCode`'s `.opencode/opencode.json` plugin registry. Each +//! merge keeps every non-SCE key and entry untouched, and replaces SCE-owned +//! content wholesale so repeated installs stay idempotent. + +use anyhow::{Context, Result}; +use serde_json::Value; + +/// Substring identifying an SCE-authored Claude hook command +/// (`config/pkl/renderers/claude-content.pkl`). +const CLAUDE_SCE_HOOK_MARKER: &str = "run-sce-or-show-install-guidance.sh"; + +/// Path prefix identifying an SCE-authored `OpenCode` plugin registration +/// (`config/pkl/base/opencode.pkl`), matched structurally so a plugin path an +/// older or renamed catalog installed is still recognized as SCE-owned even +/// though the current generated document no longer declares it. +const OPENCODE_SCE_PLUGIN_PREFIX: &str = "./plugins/sce-"; + +/// Merges `generated` (the freshly rendered SCE settings document) into +/// `existing_bytes` (the user's current `.claude/settings.json`, if any) and +/// returns the merged document's bytes, pretty-printed with a trailing +/// newline. When `existing_bytes` is `None`, returns `generated` verbatim. +/// +/// `source_path` is used only to name the offending file in a parse error. +pub fn merge_or_create_claude_settings( + existing_bytes: Option<&[u8]>, + generated_bytes: &[u8], + source_path: &str, +) -> Result> { + let Some(existing_bytes) = existing_bytes else { + return Ok(generated_bytes.to_vec()); + }; + + let existing: Value = serde_json::from_slice(existing_bytes).with_context(|| { + format!("Existing config file '{source_path}' must contain valid JSON.") + })?; + let generated: Value = serde_json::from_slice(generated_bytes) + .context("Generated settings payload must be valid JSON")?; + + let merged = merge_claude_settings(&existing, &generated, source_path)?; + + let mut serialized = serde_json::to_string_pretty(&merged) + .context("Failed to serialize merged Claude settings")?; + serialized.push('\n'); + Ok(serialized.into_bytes()) +} + +/// Merges `generated` into `existing` for the Claude settings shape: +/// - `$schema` is SCE-owned and taken from `generated`. +/// - `hooks` is merged event-by-event: for each event key `generated.hooks` +/// declares, entries in `existing.hooks[event]` whose command contains the +/// SCE marker are dropped, and `generated.hooks[event]`'s entries are +/// appended after the surviving (non-SCE) entries. Event keys `existing` +/// holds that `generated` does not declare are left untouched. +/// - Every other top-level key in `existing` is left untouched. +fn merge_claude_settings(existing: &Value, generated: &Value, source_path: &str) -> Result { + let mut existing_obj = existing.as_object().cloned().with_context(|| { + format!("Existing config file '{source_path}' must contain a top-level JSON object.") + })?; + let generated_obj = generated + .as_object() + .context("Generated settings payload must contain a top-level JSON object")?; + + if let Some(schema) = generated_obj.get("$schema") { + existing_obj.insert("$schema".to_string(), schema.clone()); + } + + if let Some(generated_hooks) = generated_obj.get("hooks") { + let generated_hooks = generated_hooks + .as_object() + .context("Generated settings 'hooks' must be a JSON object")?; + + let mut existing_hooks = match existing_obj.get("hooks") { + Some(value) => value.as_object().cloned().with_context(|| { + format!("Existing config file '{source_path}' key 'hooks' must be a JSON object.") + })?, + None => serde_json::Map::new(), + }; + + for (event, generated_entries) in generated_hooks { + let generated_entries = generated_entries.as_array().with_context(|| { + format!("Generated settings 'hooks.{event}' must be a JSON array") + })?; + + let existing_entries = match existing_hooks.get(event) { + Some(value) => value.as_array().with_context(|| { + format!("Existing config file '{source_path}' key 'hooks.{event}' must be a JSON array.") + })?, + None => &Vec::new(), + }; + + let mut merged_entries: Vec = existing_entries + .iter() + .filter(|entry| !hook_entry_is_sce_owned(entry)) + .cloned() + .collect(); + merged_entries.extend(generated_entries.iter().cloned()); + + existing_hooks.insert(event.clone(), Value::Array(merged_entries)); + } + + existing_obj.insert("hooks".to_string(), Value::Object(existing_hooks)); + } + + Ok(Value::Object(existing_obj)) +} + +/// True when a Claude hook-matcher entry (`{"matcher": ..., "hooks": [{"type", +/// "command"}, ...]}`) carries at least one command routed through the SCE +/// hook script. +fn hook_entry_is_sce_owned(entry: &Value) -> bool { + entry + .get("hooks") + .and_then(Value::as_array) + .is_some_and(|hooks| { + hooks.iter().any(|hook| { + hook.get("command") + .and_then(Value::as_str) + .is_some_and(|command| command.contains(CLAUDE_SCE_HOOK_MARKER)) + }) + }) +} + +/// Merges `generated` (the freshly rendered SCE `OpenCode` config) into +/// `existing_bytes` (the user's current `.opencode/opencode.json`, if any) and +/// returns the merged document's bytes, pretty-printed with a trailing +/// newline. When `existing_bytes` is `None`, returns `generated` verbatim. +/// +/// `source_path` is used only to name the offending file in a parse error. +pub fn merge_or_create_opencode_config( + existing_bytes: Option<&[u8]>, + generated_bytes: &[u8], + source_path: &str, +) -> Result> { + let Some(existing_bytes) = existing_bytes else { + return Ok(generated_bytes.to_vec()); + }; + + let existing: Value = serde_json::from_slice(existing_bytes).with_context(|| { + format!("Existing config file '{source_path}' must contain valid JSON.") + })?; + let generated: Value = serde_json::from_slice(generated_bytes) + .context("Generated OpenCode config payload must be valid JSON")?; + + let merged = merge_opencode_config(&existing, &generated, source_path)?; + + let mut serialized = serde_json::to_string_pretty(&merged) + .context("Failed to serialize merged OpenCode config")?; + serialized.push('\n'); + Ok(serialized.into_bytes()) +} + +/// Merges `generated` into `existing` for the `OpenCode` config shape: +/// - `$schema` is SCE-owned and taken from `generated`. +/// - `plugin` is merged as a set: entries in `existing.plugin` shaped like an +/// SCE plugin path are dropped (whether or not `generated.plugin` still +/// declares them), and `generated.plugin`'s entries are appended after the +/// surviving (non-SCE) entries. +/// - Every other top-level key in `existing` is left untouched. +fn merge_opencode_config(existing: &Value, generated: &Value, source_path: &str) -> Result { + let mut existing_obj = existing.as_object().cloned().with_context(|| { + format!("Existing config file '{source_path}' must contain a top-level JSON object.") + })?; + let generated_obj = generated + .as_object() + .context("Generated OpenCode config payload must contain a top-level JSON object")?; + + if let Some(schema) = generated_obj.get("$schema") { + existing_obj.insert("$schema".to_string(), schema.clone()); + } + + if let Some(generated_plugin) = generated_obj.get("plugin") { + let generated_plugin = generated_plugin + .as_array() + .context("Generated OpenCode config 'plugin' must be a JSON array")?; + + let existing_plugin = match existing_obj.get("plugin") { + Some(value) => value.as_array().cloned().with_context(|| { + format!("Existing config file '{source_path}' key 'plugin' must be a JSON array.") + })?, + None => Vec::new(), + }; + + let mut merged_plugin: Vec = existing_plugin + .into_iter() + .filter(|entry| !plugin_entry_is_sce_owned(entry)) + .collect(); + merged_plugin.extend(generated_plugin.iter().cloned()); + + existing_obj.insert("plugin".to_string(), Value::Array(merged_plugin)); + } + + Ok(Value::Object(existing_obj)) +} + +/// True when a `plugin` array entry is a string shaped like an SCE plugin +/// registration path (`./plugins/sce-*`). +fn plugin_entry_is_sce_owned(entry: &Value) -> bool { + entry + .as_str() + .is_some_and(|path| path.starts_with(OPENCODE_SCE_PLUGIN_PREFIX)) +} + +/// True when merging `generated` into `existing_bytes` would be a no-op, i.e. +/// `existing_bytes` already carries a current, complete copy of every +/// SCE-owned hook entry the generated document declares. Used by `sce doctor` +/// to tell a merged file that legitimately carries extra user content apart +/// from an SCE-owned fragment that is missing or stale. +pub(crate) fn claude_settings_fragment_is_current( + existing_bytes: &[u8], + generated_bytes: &[u8], +) -> Result { + let existing: Value = serde_json::from_slice(existing_bytes) + .context("Existing Claude settings file must contain valid JSON.")?; + let generated: Value = serde_json::from_slice(generated_bytes) + .context("Generated settings payload must be valid JSON")?; + + let merged = merge_claude_settings(&existing, &generated, "existing")?; + Ok(merged == existing) +} + +/// True when merging `generated` into `existing_bytes` would be a no-op, i.e. +/// `existing_bytes` already carries every canonical SCE plugin path the +/// generated document declares and no stale SCE-shaped plugin path. Used by +/// `sce doctor` for the same purpose as `claude_settings_fragment_is_current`. +pub(crate) fn opencode_config_fragment_is_current( + existing_bytes: &[u8], + generated_bytes: &[u8], +) -> Result { + let existing: Value = serde_json::from_slice(existing_bytes) + .context("Existing OpenCode config file must contain valid JSON.")?; + let generated: Value = serde_json::from_slice(generated_bytes) + .context("Generated OpenCode config payload must be valid JSON")?; + + let merged = merge_opencode_config(&existing, &generated, "existing")?; + Ok(merged == existing) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn sce_hook_entry(command: &str) -> Value { + json!({ + "hooks": [ + {"type": "command", "command": format!("bash \"$CLAUDE_PROJECT_DIR/.claude/hooks/{}\" {}", CLAUDE_SCE_HOOK_MARKER, command)} + ] + }) + } + + fn user_hook_entry() -> Value { + json!({ + "matcher": "Bash", + "hooks": [ + {"type": "command", "command": "echo user-hook"} + ] + }) + } + + fn generated_settings() -> Value { + json!({ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "hooks": { + "PreToolUse": [sce_hook_entry("sce policy bash")], + "Stop": [sce_hook_entry("sce hooks conversation-trace")] + } + }) + } + + #[test] + fn preserves_user_keys_and_non_sce_hook_entries() { + let existing = json!({ + "permissions": {"allow": ["Bash(git *)"]}, + "env": {"FOO": "bar"}, + "hooks": { + "PreToolUse": [user_hook_entry()] + } + }); + + let merged = + merge_claude_settings(&existing, &generated_settings(), "settings.json").unwrap(); + + assert_eq!(merged["permissions"]["allow"][0], "Bash(git *)"); + assert_eq!(merged["env"]["FOO"], "bar"); + + let pre_tool_use = merged["hooks"]["PreToolUse"].as_array().unwrap(); + assert_eq!(pre_tool_use.len(), 2); + assert_eq!(pre_tool_use[0], user_hook_entry()); + assert!(hook_entry_is_sce_owned(&pre_tool_use[1])); + } + + #[test] + fn replaces_sce_entries_instead_of_duplicating_them_across_two_merges() { + let existing = json!({}); + + let once = + merge_claude_settings(&existing, &generated_settings(), "settings.json").unwrap(); + let twice = merge_claude_settings(&once, &generated_settings(), "settings.json").unwrap(); + + assert_eq!(once, twice); + assert_eq!(twice["hooks"]["PreToolUse"].as_array().unwrap().len(), 1); + assert_eq!(twice["hooks"]["Stop"].as_array().unwrap().len(), 1); + } + + #[test] + fn drops_sce_entry_the_generated_document_no_longer_declares() { + let existing = json!({ + "hooks": { + "PreToolUse": [sce_hook_entry("sce policy bash"), user_hook_entry()], + "Stop": [sce_hook_entry("stale command")] + } + }); + + let generated = json!({ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "hooks": { + "PreToolUse": [sce_hook_entry("sce policy bash")], + "Stop": [] + } + }); + + let merged = merge_claude_settings(&existing, &generated, "settings.json").unwrap(); + + let stop = merged["hooks"]["Stop"].as_array().unwrap(); + assert!(stop.is_empty()); + let pre_tool_use = merged["hooks"]["PreToolUse"].as_array().unwrap(); + assert_eq!(pre_tool_use.len(), 2); + assert_eq!(pre_tool_use[0], user_hook_entry()); + } + + #[test] + fn leaves_event_keys_generated_does_not_declare_untouched() { + let existing = json!({ + "hooks": { + "Notification": [user_hook_entry()] + } + }); + + let merged = + merge_claude_settings(&existing, &generated_settings(), "settings.json").unwrap(); + + assert_eq!( + merged["hooks"]["Notification"].as_array().unwrap()[0], + user_hook_entry() + ); + } + + #[test] + fn missing_file_returns_generated_bytes_verbatim() { + let generated_bytes = b"{\"$schema\":\"x\"}"; + let result = + merge_or_create_claude_settings(None, generated_bytes, "settings.json").unwrap(); + assert_eq!(result, generated_bytes); + } + + #[test] + fn unparseable_existing_file_fails_naming_the_path_and_does_not_write() { + let generated_bytes = serde_json::to_vec(&generated_settings()).unwrap(); + let error = merge_or_create_claude_settings( + Some(b"{ not valid json"), + &generated_bytes, + ".claude/settings.json", + ) + .unwrap_err(); + + assert!(error.to_string().contains(".claude/settings.json")); + } + + #[test] + fn missing_existing_hooks_key_is_populated_from_generated() { + let existing = json!({"permissions": {"allow": []}}); + + let merged = + merge_claude_settings(&existing, &generated_settings(), "settings.json").unwrap(); + + assert_eq!(merged["hooks"]["PreToolUse"].as_array().unwrap().len(), 1); + } + + fn generated_opencode_config() -> Value { + json!({ + "$schema": "https://opencode.ai/config.json", + "plugin": ["./plugins/sce-bash-policy.ts", "./plugins/sce-agent-trace.ts"] + }) + } + + #[test] + fn opencode_merge_preserves_user_keys_and_user_plugin() { + let existing = json!({ + "model": "anthropic/claude", + "mcp": {"my-server": {"command": "my-server"}}, + "plugin": ["./plugins/my-plugin.ts"] + }); + + let merged = + merge_opencode_config(&existing, &generated_opencode_config(), "opencode.json") + .unwrap(); + + assert_eq!(merged["model"], "anthropic/claude"); + assert_eq!(merged["mcp"]["my-server"]["command"], "my-server"); + + let plugin = merged["plugin"].as_array().unwrap(); + assert_eq!(plugin.len(), 3); + assert_eq!(plugin[0], "./plugins/my-plugin.ts"); + assert!(plugin.contains(&json!("./plugins/sce-bash-policy.ts"))); + assert!(plugin.contains(&json!("./plugins/sce-agent-trace.ts"))); + } + + #[test] + fn opencode_merge_is_idempotent_across_two_merges() { + let existing = json!({}); + + let once = merge_opencode_config(&existing, &generated_opencode_config(), "opencode.json") + .unwrap(); + let twice = + merge_opencode_config(&once, &generated_opencode_config(), "opencode.json").unwrap(); + + assert_eq!(once, twice); + assert_eq!(twice["plugin"].as_array().unwrap().len(), 2); + } + + #[test] + fn opencode_merge_drops_stale_sce_shaped_plugin_path_current_catalog_no_longer_declares() { + let existing = json!({ + "plugin": ["./plugins/sce-old-feature.ts", "./plugins/my-plugin.ts"] + }); + + let merged = + merge_opencode_config(&existing, &generated_opencode_config(), "opencode.json") + .unwrap(); + + let plugin = merged["plugin"].as_array().unwrap(); + assert!(!plugin.contains(&json!("./plugins/sce-old-feature.ts"))); + assert!(plugin.contains(&json!("./plugins/my-plugin.ts"))); + assert!(plugin.contains(&json!("./plugins/sce-bash-policy.ts"))); + assert!(plugin.contains(&json!("./plugins/sce-agent-trace.ts"))); + } + + #[test] + fn opencode_missing_file_returns_generated_bytes_verbatim() { + let generated_bytes = b"{\"$schema\":\"x\"}"; + let result = + merge_or_create_opencode_config(None, generated_bytes, "opencode.json").unwrap(); + assert_eq!(result, generated_bytes); + } + + #[test] + fn opencode_unparseable_existing_file_fails_naming_the_path_and_does_not_write() { + let generated_bytes = serde_json::to_vec(&generated_opencode_config()).unwrap(); + let error = merge_or_create_opencode_config( + Some(b"{ not valid json"), + &generated_bytes, + ".opencode/opencode.json", + ) + .unwrap_err(); + + assert!(error.to_string().contains(".opencode/opencode.json")); + } + + #[test] + fn opencode_missing_existing_plugin_key_is_populated_from_generated() { + let existing = json!({"model": "anthropic/claude"}); + + let merged = + merge_opencode_config(&existing, &generated_opencode_config(), "opencode.json") + .unwrap(); + + assert_eq!(merged["plugin"].as_array().unwrap().len(), 2); + } + + #[test] + fn claude_fragment_is_current_when_merged_settings_already_match() { + let generated_bytes = serde_json::to_vec(&generated_settings()).unwrap(); + let installed = + merge_or_create_claude_settings(None, &generated_bytes, "settings.json").unwrap(); + let with_user_keys = + merge_or_create_claude_settings(Some(&installed), &generated_bytes, "settings.json") + .unwrap(); + + assert!(claude_settings_fragment_is_current(&with_user_keys, &generated_bytes).unwrap()); + } + + #[test] + fn claude_fragment_is_not_current_when_sce_hook_entry_is_deleted() { + let generated_bytes = serde_json::to_vec(&generated_settings()).unwrap(); + let installed_bytes = + merge_or_create_claude_settings(None, &generated_bytes, "settings.json").unwrap(); + let mut installed: Value = serde_json::from_slice(&installed_bytes).unwrap(); + installed["permissions"] = json!({"allow": ["Bash(git *)"]}); + + assert!(claude_settings_fragment_is_current( + &serde_json::to_vec(&installed).unwrap(), + &generated_bytes + ) + .unwrap()); + + installed["hooks"]["PreToolUse"] = json!([]); + let drifted_bytes = serde_json::to_vec(&installed).unwrap(); + + assert!(!claude_settings_fragment_is_current(&drifted_bytes, &generated_bytes).unwrap()); + } + + #[test] + fn opencode_fragment_is_current_when_merged_plugins_already_match() { + let generated_bytes = serde_json::to_vec(&generated_opencode_config()).unwrap(); + let installed_bytes = + merge_or_create_opencode_config(None, &generated_bytes, "opencode.json").unwrap(); + let mut installed: Value = serde_json::from_slice(&installed_bytes).unwrap(); + installed["model"] = json!("anthropic/claude"); + installed["plugin"] + .as_array_mut() + .unwrap() + .insert(0, json!("./plugins/my-plugin.ts")); + let existing_bytes = serde_json::to_vec(&installed).unwrap(); + + assert!(opencode_config_fragment_is_current(&existing_bytes, &generated_bytes).unwrap()); + } + + #[test] + fn opencode_fragment_is_not_current_when_sce_plugin_path_is_stale() { + let generated_bytes = serde_json::to_vec(&generated_opencode_config()).unwrap(); + let existing = json!({ + "plugin": ["./plugins/sce-old-feature.ts"] + }); + let existing_bytes = serde_json::to_vec(&existing).unwrap(); + + assert!(!opencode_config_fragment_is_current(&existing_bytes, &generated_bytes).unwrap()); + } +} diff --git a/cli/src/services/setup/mod.rs b/cli/src/services/setup/mod.rs index 86c67cb7..cfbef3ea 100644 --- a/cli/src/services/setup/mod.rs +++ b/cli/src/services/setup/mod.rs @@ -9,6 +9,7 @@ use crate::services::style::{label, success, value}; use crate::services::{default_paths, default_paths::RepoPaths}; pub mod command; +pub(crate) mod config_merge; /// Canonical JSON payload for a newly bootstrapped repo-local `.sce/config.json`. /// Contains only the `$schema` declaration pointing to the SCE config JSON Schema. @@ -647,6 +648,18 @@ pub fn install_embedded_setup_assets( install::install_embedded_setup_assets(repository_root, target, selected_optional_workflows) } +/// Repairs a single merge-target asset (`.claude/settings.json` or +/// `.opencode/opencode.json`) by reinstalling just that asset through the same +/// per-asset merge-install path `sce setup` uses, so `sce doctor --fix` can +/// restore a drifted SCE fragment without touching any other asset. +pub(crate) fn repair_merge_target_asset( + repository_root: &Path, + target: SetupTarget, + relative_path: &str, +) -> Result<()> { + install::repair_merge_target_asset(repository_root, target, relative_path) +} + pub(crate) fn setup_install_recovery_guidance( target: SetupTarget, destination_root: &Path, @@ -800,13 +813,16 @@ mod install { use crate::services::default_paths::InstallTargetPaths; use crate::services::security::{ensure_directory_is_writable, redact_sensitive_text}; + use super::config_merge; use super::{ - cleanup_path_if_exists, concrete_targets_for, hook_install_recovery_guidance, - iter_embedded_assets_for_setup_target_with_selection, iter_required_hook_assets, - setup_install_recovery_guidance, EmbeddedAsset, RequiredHookInstallResult, - RequiredHookInstallStatus, RequiredHooksInstallOutcome, SetupInstallOutcome, - SetupInstallTargetResult, SetupTarget, + cleanup_path_if_exists, concrete_targets_for, embedded_assets_for_concrete_target, + hook_install_recovery_guidance, iter_embedded_assets_for_setup_target_with_selection, + iter_required_hook_assets, setup_install_recovery_guidance, EmbeddedAsset, + RequiredHookInstallResult, RequiredHookInstallStatus, RequiredHooksInstallOutcome, + SetupInstallOutcome, SetupInstallTargetResult, SetupTarget, }; + use crate::services::default_paths; + use crate::services::default_paths::claude_asset; pub(super) fn prepare_setup_hooks_repository(repository_root: &Path) -> Result { let normalized_repository_root = normalize_user_repository_path(repository_root)?; @@ -839,6 +855,31 @@ mod install { ) } + pub(super) fn repair_merge_target_asset( + repository_root: &Path, + target: SetupTarget, + relative_path: &str, + ) -> Result<()> { + let asset = embedded_assets_for_concrete_target(target) + .iter() + .find(|asset| asset.relative_path == relative_path) + .with_context(|| { + format!("No embedded asset named '{relative_path}' for target {target:?}") + })?; + + let install_targets = InstallTargetPaths::new(repository_root); + let destination_root = match target { + SetupTarget::OpenCode => install_targets.opencode_target_dir(), + SetupTarget::Claude => install_targets.claude_target_dir(), + SetupTarget::Pi => install_targets.pi_target_dir(), + SetupTarget::All => unreachable!("meta targets are expanded into concrete targets"), + }; + + install_single_asset_with_rename(target, &destination_root, asset, &mut |from, to| { + fs::rename(from, to) + }) + } + fn install_required_git_hooks_in_resolved_repository( resolved_repository_root: &Path, mut rename_fn: F, @@ -1156,7 +1197,7 @@ mod install { Ok(metadata.is_file()) } - fn install_embedded_setup_assets_with_rename( + pub(super) fn install_embedded_setup_assets_with_rename( repository_root: &Path, target: SetupTarget, selected_optional_workflows: &[String], @@ -1207,38 +1248,221 @@ mod install { unreachable!("meta targets are expanded into concrete targets") } }; - let staging_root = create_staging_root(repository_root, target)?; - if let Err(error) = write_assets_to_staging(&staging_root, assets) { - cleanup_path_if_exists(&staging_root); - return Err(error); + for asset in assets { + install_single_asset_with_rename(target, &destination_root, asset, rename_fn)?; } - if destination_root.exists() { - remove_existing_install_target(&destination_root).with_context(|| { + prune_stale_assets_for_concrete_target(&destination_root, target, assets)?; + + Ok(SetupInstallTargetResult { + target, + destination_root, + installed_file_count: assets.len(), + }) + } + + /// Deletes every catalog asset for `target` that this run did not install + /// (deselected, or dropped by a newer catalog), then removes any SCE-owned + /// skill directory left empty by that deletion. A directory still holding a + /// user file fails to remove and is left in place. + fn prune_stale_assets_for_concrete_target( + destination_root: &Path, + target: SetupTarget, + installed_assets: &[&'static EmbeddedAsset], + ) -> Result<()> { + let installed_paths: std::collections::HashSet<&'static str> = installed_assets + .iter() + .map(|asset| asset.relative_path) + .collect(); + + for asset in embedded_assets_for_concrete_target(target) { + if installed_paths.contains(asset.relative_path) { + continue; + } + + let destination = destination_root.join(asset.relative_path); + if !destination.is_file() { + continue; + } + + fs::remove_file(&destination).with_context(|| { format!( - "Failed to replace existing setup target '{}' without creating a backup", - destination_root.display() + "Failed to prune unselected setup asset '{}'", + destination.display() ) })?; + + remove_empty_ancestor_directories(destination_root, &destination); + } + + Ok(()) + } + + /// Removes now-empty parent directories of a pruned file, walking upward + /// until reaching `destination_root` or a directory that still has content + /// (removal fails and stops the walk). + fn remove_empty_ancestor_directories(destination_root: &Path, removed_file: &Path) { + let mut current = removed_file.parent(); + while let Some(directory) = current { + if directory == destination_root || !directory.starts_with(destination_root) { + break; + } + if fs::remove_dir(directory).is_err() { + break; + } + current = directory.parent(); } + } + + /// True for the one asset the Claude install path merges into an existing + /// document instead of overwriting: `.claude/settings.json`. + fn is_claude_settings_merge_target(target: SetupTarget, relative_path: &str) -> bool { + target == SetupTarget::Claude && relative_path == claude_asset::SETTINGS_FILE + } + + /// True for the one asset the `OpenCode` install path merges into an existing + /// document instead of overwriting: `.opencode/opencode.json`. + fn is_opencode_config_merge_target(target: SetupTarget, relative_path: &str) -> bool { + target == SetupTarget::OpenCode + && relative_path == default_paths::repo_file::OPENCODE_MANIFEST + } - if let Err(error) = rename_fn(&staging_root, &destination_root).with_context(|| { + fn install_single_asset_with_rename( + target: SetupTarget, + destination_root: &Path, + asset: &'static EmbeddedAsset, + rename_fn: &mut F, + ) -> Result<()> + where + F: FnMut(&Path, &Path) -> io::Result<()>, + { + validate_embedded_relative_path(asset.relative_path)?; + let destination = destination_root.join(asset.relative_path); + let parent = destination + .parent() + .context("Embedded asset destination should have a parent directory")?; + + fs::create_dir_all(parent).with_context(|| { format!( - "Failed to swap staged install '{}' into destination '{}'", - staging_root.display(), - destination_root.display() + "Failed to create parent directory '{}' for setup asset", + parent.display() + ) + })?; + + if destination.is_dir() { + bail!( + "Setup asset destination '{}' is an existing directory, not a file. Try: remove or rename the directory and rerun 'sce setup'.", + destination.display() + ); + } + + let install_bytes: Vec = if is_claude_settings_merge_target(target, asset.relative_path) + { + let existing_bytes = if destination.is_file() { + Some(fs::read(&destination).with_context(|| { + format!( + "Failed to read existing setup asset '{}' for merge", + destination.display() + ) + })?) + } else { + None + }; + config_merge::merge_or_create_claude_settings( + existing_bytes.as_deref(), + asset.bytes, + &destination.display().to_string(), + )? + } else if is_opencode_config_merge_target(target, asset.relative_path) { + let existing_bytes = if destination.is_file() { + Some(fs::read(&destination).with_context(|| { + format!( + "Failed to read existing setup asset '{}' for merge", + destination.display() + ) + })?) + } else { + None + }; + config_merge::merge_or_create_opencode_config( + existing_bytes.as_deref(), + asset.bytes, + &destination.display().to_string(), + )? + } else { + asset.bytes.to_vec() + }; + + let staging_path = create_asset_staging_path(parent, asset.relative_path)?; + if let Err(error) = fs::write(&staging_path, &install_bytes).with_context(|| { + format!( + "Failed to write staged embedded asset '{}'", + staging_path.display() ) }) { - cleanup_path_if_exists(&staging_root); - return Err(error.context(setup_install_recovery_guidance(target, &destination_root))); + cleanup_path_if_exists(&staging_path); + return Err(error); } - Ok(SetupInstallTargetResult { - target, - destination_root, - installed_file_count: assets.len(), - }) + if destination.exists() { + if let Err(error) = fs::remove_file(&destination).with_context(|| { + format!( + "Failed to replace existing setup asset '{}' without creating a backup", + destination.display() + ) + }) { + cleanup_path_if_exists(&staging_path); + return Err(error); + } + } + + if let Err(error) = rename_fn(&staging_path, &destination).with_context(|| { + format!( + "Failed to install staged asset '{}' into destination '{}'", + staging_path.display(), + destination.display() + ) + }) { + cleanup_path_if_exists(&staging_path); + return Err(error.context(setup_install_recovery_guidance(target, &destination))); + } + + Ok(()) + } + + fn create_asset_staging_path(parent: &Path, relative_path: &str) -> Result { + let epoch_nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .context("System clock is before UNIX_EPOCH")? + .as_nanos(); + let sanitized_name = relative_path.replace(['/', '\\'], "-"); + + for attempt in 0..1000_u16 { + let candidate = parent.join(format!( + ".sce-setup-staging-{sanitized_name}-{epoch_nanos}-{}-{attempt}", + std::process::id() + )); + + match fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&candidate) + { + Ok(_) => return Ok(candidate), + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => {} + Err(error) => { + return Err(error).with_context(|| { + format!("Failed to allocate staging file '{}'", candidate.display()) + }); + } + } + } + + bail!( + "Could not allocate a unique staging file under '{}'", + parent.display() + ) } fn remove_existing_install_target(destination_root: &Path) -> Result<()> { @@ -1268,35 +1492,6 @@ mod install { Ok(()) } - fn write_assets_to_staging( - staging_root: &Path, - assets: &[&'static EmbeddedAsset], - ) -> Result<()> { - for asset in assets { - validate_embedded_relative_path(asset.relative_path)?; - let destination = staging_root.join(asset.relative_path); - let parent = destination - .parent() - .context("Embedded asset destination should have a parent directory")?; - - fs::create_dir_all(parent).with_context(|| { - format!( - "Failed to create staged parent directory '{}'", - parent.display() - ) - })?; - - fs::write(&destination, asset.bytes).with_context(|| { - format!( - "Failed to write staged embedded asset '{}'", - destination.display() - ) - })?; - } - - Ok(()) - } - fn validate_embedded_relative_path(relative_path: &str) -> Result<()> { let path = Path::new(relative_path); @@ -1315,52 +1510,6 @@ mod install { Ok(()) } - - fn create_staging_root(repository_root: &Path, target: SetupTarget) -> Result { - let install_targets = InstallTargetPaths::new(repository_root); - let target_dir = match target { - SetupTarget::OpenCode => install_targets.opencode_target_dir(), - SetupTarget::Claude => install_targets.claude_target_dir(), - SetupTarget::Pi => install_targets.pi_target_dir(), - SetupTarget::All => { - unreachable!("meta targets are expanded into concrete targets") - } - }; - let target_label = target_dir - .file_name() - .and_then(|name| name.to_str()) - .context("Setup target directory should have a valid UTF-8 file name")? - .trim_start_matches('.'); - let epoch_nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .context("System clock is before UNIX_EPOCH")? - .as_nanos(); - - for attempt in 0..1000_u16 { - let candidate = repository_root.join(format!( - ".sce-setup-staging-{target_label}-{epoch_nanos}-{}-{attempt}", - std::process::id() - )); - - match fs::create_dir(&candidate) { - Ok(()) => return Ok(candidate), - Err(error) if error.kind() == io::ErrorKind::AlreadyExists => {} - Err(error) => { - return Err(error).with_context(|| { - format!( - "Failed to create staging directory '{}'", - candidate.display() - ) - }); - } - } - } - - bail!( - "Could not allocate a unique staging directory under '{}'", - repository_root.display() - ) - } } pub trait SetupTargetPrompter { @@ -1935,4 +2084,322 @@ mod tests { assert!(contains(SetupTarget::Pi, "extensions/sce/index.ts")); assert!(iter_required_hook_assets().all(|asset| !asset.bytes.is_empty())); } + + #[test] + fn install_preserves_user_owned_files_and_writes_sce_assets() { + let repo = init_git_repo("install-preserves-user-files"); + let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); + + fs::create_dir_all(claude_dir.join("skills/my-own-skill")).expect("create user skill dir"); + fs::create_dir_all(claude_dir.join("commands")).expect("create commands dir"); + + fs::write(claude_dir.join("MY_NOTES.md"), "top level user notes\n") + .expect("seed top-level user file"); + fs::write( + claude_dir.join("skills/my-own-skill/SKILL.md"), + "user skill content\n", + ) + .expect("seed user skill file"); + fs::write( + claude_dir.join("commands/my-command.md"), + "user command content\n", + ) + .expect("seed user command file"); + + let selection: Vec = every_optional_workflow() + .into_iter() + .map(str::to_string) + .collect(); + + install_embedded_setup_assets(&repo, SetupTarget::Claude, &selection) + .expect("install should succeed"); + + assert_eq!( + fs::read_to_string(claude_dir.join("MY_NOTES.md")).expect("read top-level user file"), + "top level user notes\n" + ); + assert_eq!( + fs::read_to_string(claude_dir.join("skills/my-own-skill/SKILL.md")) + .expect("read user skill file"), + "user skill content\n" + ); + assert_eq!( + fs::read_to_string(claude_dir.join("commands/my-command.md")) + .expect("read user command file"), + "user command content\n" + ); + + let expected_next_task_bytes = + iter_embedded_assets_for_setup_target_with_selection(SetupTarget::Claude, &selection) + .find(|asset| asset.relative_path == "commands/next-task.md") + .expect("next-task asset should be in the catalog") + .bytes; + assert_eq!( + fs::read(claude_dir.join("commands/next-task.md")).expect("read installed sce asset"), + expected_next_task_bytes + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn install_merges_into_existing_claude_settings_json_and_stays_idempotent() { + let repo = init_git_repo("install-merges-claude-settings"); + let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); + + fs::create_dir_all(&claude_dir).expect("create claude dir"); + fs::write( + claude_dir.join("settings.json"), + serde_json::to_string_pretty(&json!({ + "permissions": {"allow": ["Bash(git *)"]}, + "env": {"FOO": "bar"}, + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo user-hook"}] + } + ] + } + })) + .expect("serialize seeded settings"), + ) + .expect("seed existing settings.json"); + + let selection: Vec = every_optional_workflow() + .into_iter() + .map(str::to_string) + .collect(); + + install_embedded_setup_assets(&repo, SetupTarget::Claude, &selection) + .expect("first install should succeed"); + + let after_first = + fs::read_to_string(claude_dir.join("settings.json")).expect("read merged settings"); + let merged: serde_json::Value = + serde_json::from_str(&after_first).expect("merged settings should be valid JSON"); + + assert_eq!(merged["permissions"]["allow"][0], "Bash(git *)"); + assert_eq!(merged["env"]["FOO"], "bar"); + let pre_tool_use = merged["hooks"]["PreToolUse"] + .as_array() + .expect("PreToolUse should be an array"); + assert!(pre_tool_use + .iter() + .any(|entry| entry["hooks"][0]["command"] == "echo user-hook")); + assert!(pre_tool_use + .iter() + .any(|entry| entry["hooks"] + .as_array() + .unwrap() + .iter() + .any(|hook| hook["command"] + .as_str() + .unwrap() + .contains("run-sce-or-show-install-guidance.sh")))); + + install_embedded_setup_assets(&repo, SetupTarget::Claude, &selection) + .expect("second install should succeed"); + + let after_second = + fs::read_to_string(claude_dir.join("settings.json")).expect("read re-merged settings"); + assert_eq!( + after_first, after_second, + "two consecutive installs should merge to byte-identical output" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn install_merges_into_existing_opencode_config_json_and_stays_idempotent() { + let repo = init_git_repo("install-merges-opencode-config"); + let opencode_dir = default_paths::InstallTargetPaths::new(&repo).opencode_target_dir(); + + fs::create_dir_all(&opencode_dir).expect("create opencode dir"); + fs::write( + opencode_dir.join("opencode.json"), + serde_json::to_string_pretty(&json!({ + "model": "anthropic/claude", + "mcp": {"my-server": {"command": "my-server"}}, + "plugin": ["./plugins/my-plugin.ts", "./plugins/sce-old-feature.ts"] + })) + .expect("serialize seeded opencode config"), + ) + .expect("seed existing opencode.json"); + + let selection: Vec = every_optional_workflow() + .into_iter() + .map(str::to_string) + .collect(); + + install_embedded_setup_assets(&repo, SetupTarget::OpenCode, &selection) + .expect("first install should succeed"); + + let after_first = fs::read_to_string(opencode_dir.join("opencode.json")) + .expect("read merged opencode config"); + let merged: serde_json::Value = serde_json::from_str(&after_first) + .expect("merged opencode config should be valid JSON"); + + assert_eq!(merged["model"], "anthropic/claude"); + assert_eq!(merged["mcp"]["my-server"]["command"], "my-server"); + + let plugin = merged["plugin"] + .as_array() + .expect("plugin should be an array"); + assert!(plugin.contains(&json!("./plugins/my-plugin.ts"))); + assert!(plugin.contains(&json!("./plugins/sce-bash-policy.ts"))); + assert!(plugin.contains(&json!("./plugins/sce-agent-trace.ts"))); + assert!(!plugin.contains(&json!("./plugins/sce-old-feature.ts"))); + + install_embedded_setup_assets(&repo, SetupTarget::OpenCode, &selection) + .expect("second install should succeed"); + + let after_second = fs::read_to_string(opencode_dir.join("opencode.json")) + .expect("read re-merged opencode config"); + assert_eq!( + after_first, after_second, + "two consecutive installs should merge to byte-identical output" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn reinstall_with_empty_selection_prunes_deselected_workflow_without_touching_sibling_skill() { + let repo = init_git_repo("install-prunes-deselected-workflow"); + let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); + + let brownfield_selection = vec!["brownfield".to_string()]; + install_embedded_setup_assets(&repo, SetupTarget::Claude, &brownfield_selection) + .expect("initial install with brownfield selected should succeed"); + + let brownfield_command = claude_dir.join("commands/brownfield.md"); + let brownfield_skill_dir = claude_dir.join("skills/sce-brownfield"); + assert!( + brownfield_command.is_file(), + "brownfield command should be installed" + ); + assert!( + brownfield_skill_dir.is_dir(), + "brownfield skill dir should be installed" + ); + + fs::create_dir_all(claude_dir.join("skills/my-skill")).expect("create user skill dir"); + fs::write( + claude_dir.join("skills/my-skill/SKILL.md"), + "sibling user skill\n", + ) + .expect("seed sibling user skill file"); + + install_embedded_setup_assets(&repo, SetupTarget::Claude, &[]) + .expect("reinstall with empty selection should succeed"); + + assert!( + !brownfield_command.exists(), + "deselected workflow command should be pruned" + ); + assert!( + !brownfield_skill_dir.exists(), + "deselected workflow skill dir should be pruned entirely once empty" + ); + assert_eq!( + fs::read_to_string(claude_dir.join("skills/my-skill/SKILL.md")) + .expect("read sibling user skill file"), + "sibling user skill\n" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn reinstall_with_empty_selection_keeps_pruned_skill_dir_holding_a_user_file() { + let repo = init_git_repo("install-prunes-but-keeps-user-file"); + let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); + + let brownfield_selection = vec!["brownfield".to_string()]; + install_embedded_setup_assets(&repo, SetupTarget::Claude, &brownfield_selection) + .expect("initial install with brownfield selected should succeed"); + + let brownfield_skill_dir = claude_dir.join("skills/sce-brownfield"); + fs::write( + brownfield_skill_dir.join("MY_OVERRIDE.md"), + "user file inside sce skill dir\n", + ) + .expect("seed user file inside sce-owned skill dir"); + + install_embedded_setup_assets(&repo, SetupTarget::Claude, &[]) + .expect("reinstall with empty selection should succeed"); + + assert!( + !brownfield_skill_dir.join("SKILL.md").exists(), + "deselected workflow skill file should be pruned" + ); + assert!( + brownfield_skill_dir.is_dir(), + "sce-owned skill dir should survive because it still holds a user file" + ); + assert_eq!( + fs::read_to_string(brownfield_skill_dir.join("MY_OVERRIDE.md")) + .expect("read user file inside pruned skill dir"), + "user file inside sce skill dir\n" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn install_cleans_up_staging_and_reports_asset_path_on_rename_failure() { + let repo = init_git_repo("install-rename-failure"); + let selection: Vec = every_optional_workflow() + .into_iter() + .map(str::to_string) + .collect(); + + let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); + let failing_destination = claude_dir.join("commands/next-task.md"); + + let result = install::install_embedded_setup_assets_with_rename( + &repo, + SetupTarget::Claude, + &selection, + |from, to| { + if to == failing_destination { + Err(std::io::Error::other("simulated rename failure")) + } else { + fs::rename(from, to) + } + }, + ); + + let error = result.expect_err("rename failure should surface as an error"); + let message = format!("{error:#}"); + assert!( + message.contains(&failing_destination.display().to_string()), + "error should name the failing asset path: {message}" + ); + assert!( + message.contains("does not create backups"), + "error should include recovery guidance: {message}" + ); + + let commands_staging_dir = claude_dir.join("commands"); + if commands_staging_dir.exists() { + let leftover_staging_files = fs::read_dir(&commands_staging_dir) + .expect("read commands staging dir") + .filter_map(Result::ok) + .any(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with(".sce-setup-staging-") + }); + assert!( + !leftover_staging_files, + "staging artifact for the failed asset should be cleaned up" + ); + } + + let _ = fs::remove_dir_all(&repo); + } } diff --git a/context/architecture.md b/context/architecture.md index e90d231e..57751b1c 100644 --- a/context/architecture.md +++ b/context/architecture.md @@ -123,7 +123,7 @@ The repository includes a new placeholder Rust binary crate at `cli/`. - `cli/src/services/local_db/mod.rs` provides the concrete local DB spec and `LocalDb` type alias over the shared generic `TursoDb` adapter. `LocalDbSpec` resolves the deterministic persistent runtime DB target through the shared default-path seam and declares no local migrations; `TursoDb` supplies retry-backed blocking `execute`/`query`, parent-directory creation, Turso connection setup, tokio current-thread runtime bridging, and generic migration execution. - `cli/src/services/auth_db/mod.rs` provides the encrypted auth DB spec and `AuthDb` type alias over `EncryptedTursoDb`. `AuthDbSpec` resolves `/sce/auth.db` through the shared default-path seam and embeds ordered auth migrations. Auth DB lifecycle setup/doctor integration is wired through `AuthDbLifecycle`; auth command/token-storage reads/writes are directed through `token_storage.rs`. - `cli/src/services/agent_trace_db/mod.rs` owns the shared Agent Trace insert payloads, SQL constants, and typed row helpers (diff-trace/intersection/Agent Trace/message/part) plus `ensure_schema_ready_for_hooks()` consumed by the repository adapter. `cli/src/services/agent_trace_db/repository.rs` defines the sole `RepositoryAgentTraceDb` adapter over `TursoDb` with one fresh `agent-trace-repository/001_repository_schema.sql` baseline for `diff_traces` (including `payload_type`), `post_commit_patch_intersections`, `agent_traces`, `messages`, `parts`, indexes, and triggers, `repository_metadata` validation, no trace-table `checkout_id` columns, `agent_traces.agent_trace_id NOT NULL UNIQUE`, and `recent_diff_trace_patches(cutoff_time_ms, end_time_ms)` using the inclusive chronological parser without checkout filtering. Active hook runtime, setup/lifecycle storage, and trace status/list/shell flows resolve through `agent_trace_storage` and use `RepositoryAgentTraceDb`. The checkout-scoped `AgentTraceDb`/`AgentTraceDbSpec` adapter, its `agent_trace_db_path()`/`agent_trace_db_path_for_checkout()` helpers, the 15-file `cli/migrations/agent-trace/` chain, and the `sce trace --legacy` surface were removed by the `retire-legacy-agent-trace-db` plan. -- `cli/src/services/setup/mod.rs` defines the setup command contract (`SetupMode`, `SetupTarget`, `SetupRequest`, CLI flag parser/validator), an `inquire`-backed interactive target prompter (`InquireSetupTargetPrompter`), setup dispatch outcomes (proceed/cancelled), additive durable-context bootstrap (`bootstrap_context_baseline` for standalone `--bootstrap-context` and every normal successful setup path), and compile-time embedded asset access (`EmbeddedAsset`, target-scoped iterators, required-hook asset iterators/lookups). It also owns the install-time optional-workflow seam: the `OptionalWorkflow` type plus the build-generated `OPTIONAL_WORKFLOWS` catalog, a per-target `WorkflowAssetLayout` built from the existing `default_paths` command/skill directory constants (`command`/`commands`/`prompts` plus `skills`), and `iter_embedded_assets_for_setup_target_with_selection(target, selection)`, which yields every embedded asset except the `{command_dir}/{command_slug}.md` file and `{skills_dir}/{skill_slug}/` subtree of each optional workflow the selection omits. Membership is derived from the catalog's slugs rather than an enumerated file list, so a new optional workflow needs no Rust change. This filtered iterator is the only way embedded assets are enumerated; setup installs through it and doctor inspects through it, so there is no unfiltered enumeration path that could reintroduce an unselected workflow. The non-interactive selection flows through the repeatable `--workflow ` flag into `SetupRequest.optional_workflows: Option>` (`None` meaning the flag was absent); `validate_optional_workflow_slugs` checks each slug against `OPTIONAL_WORKFLOWS` during request resolution, before any write, and `run_setup_for_mode` resolves `None` to the persisted `integrations.optional_workflows` (exported as `persisted_optional_workflows`) before installing through the filtered iterator and persisting the resolved selection. The interactive selection flows through the same seam: `SetupTargetPrompter` carries `prompt_target` plus `prompt_optional_workflows(defaults)` (returning `None` for a cancelled prompt), `SetupDispatch::Proceed { mode, optional_workflows }` carries a prompted selection alongside the resolved mode, and `resolve_setup_dispatch(mode, prompter, defaults)` runs the workflow prompt only after an interactive target prompt, mapping either cancellation to `SetupDispatch::Cancelled`. The prompt module builds its `inquire::MultiSelect` from `optional_workflow_prompt_inputs(catalog, defaults)`, which returns `None` for an empty catalog (skipping the prompt) and otherwise catalog-ordered rows plus the indices to pre-check, ignoring ids absent from the catalog. `setup/command.rs` therefore resolves the repository root before dispatch, so a non-git directory fails before any prompt. For repository builds, `cli/build.rs` validates the `SCE_CLI_GENERATED_INPUT_DIR` payload and canonical-input inventories, copies the payload into Cargo `OUT_DIR/pkl-generated`, stages `cli/assets/hooks/**` under `OUT_DIR/static`, requires the staged `config/optional-workflows.json`, and generates both the setup manifest and the optional-workflow catalog (`optional_workflows.rs`, rejecting a manifest whose `schemaVersion` is not 1 or whose entries lack a non-empty `id`/`title`/`description`/`commandSlug`/`skillSlug`) in `OUT_DIR`; focused internal seams separate install-flow from prompt-flow logic; `cli/src/services/setup/command.rs` owns the `SetupCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Its install engine/orchestrator stages embedded files and uses a unified remove-and-replace policy (removing existing targets before swapping staged content, with deterministic recovery guidance on swap failure and no backup artifact creation), and formats deterministic completion messaging; required-hook install orchestration (`install_required_git_hooks`) follows the same remove-and-replace policy (removing existing hooks before swapping staged content, with deterministic recovery guidance on swap failure). After the Git gate, setup always ensures the context baseline; context-only requests return there, while normal modes derive a repo-root-scoped context before aggregating static lifecycle provider `setup` dispatch across providers (config → local_db → auth_db → agent_trace_db → hooks when requested), so setup providers consume only repo-root access from the scoped context. +- `cli/src/services/setup/mod.rs` defines the setup command contract (`SetupMode`, `SetupTarget`, `SetupRequest`, CLI flag parser/validator), an `inquire`-backed interactive target prompter (`InquireSetupTargetPrompter`), setup dispatch outcomes (proceed/cancelled), additive durable-context bootstrap (`bootstrap_context_baseline` for standalone `--bootstrap-context` and every normal successful setup path), and compile-time embedded asset access (`EmbeddedAsset`, target-scoped iterators, required-hook asset iterators/lookups). It also owns the install-time optional-workflow seam: the `OptionalWorkflow` type plus the build-generated `OPTIONAL_WORKFLOWS` catalog, a per-target `WorkflowAssetLayout` built from the existing `default_paths` command/skill directory constants (`command`/`commands`/`prompts` plus `skills`), and `iter_embedded_assets_for_setup_target_with_selection(target, selection)`, which yields every embedded asset except the `{command_dir}/{command_slug}.md` file and `{skills_dir}/{skill_slug}/` subtree of each optional workflow the selection omits. Membership is derived from the catalog's slugs rather than an enumerated file list, so a new optional workflow needs no Rust change. This filtered iterator is the only way embedded assets are enumerated; setup installs through it and doctor inspects through it, so there is no unfiltered enumeration path that could reintroduce an unselected workflow. The non-interactive selection flows through the repeatable `--workflow ` flag into `SetupRequest.optional_workflows: Option>` (`None` meaning the flag was absent); `validate_optional_workflow_slugs` checks each slug against `OPTIONAL_WORKFLOWS` during request resolution, before any write, and `run_setup_for_mode` resolves `None` to the persisted `integrations.optional_workflows` (exported as `persisted_optional_workflows`) before installing through the filtered iterator and persisting the resolved selection. The interactive selection flows through the same seam: `SetupTargetPrompter` carries `prompt_target` plus `prompt_optional_workflows(defaults)` (returning `None` for a cancelled prompt), `SetupDispatch::Proceed { mode, optional_workflows }` carries a prompted selection alongside the resolved mode, and `resolve_setup_dispatch(mode, prompter, defaults)` runs the workflow prompt only after an interactive target prompt, mapping either cancellation to `SetupDispatch::Cancelled`. The prompt module builds its `inquire::MultiSelect` from `optional_workflow_prompt_inputs(catalog, defaults)`, which returns `None` for an empty catalog (skipping the prompt) and otherwise catalog-ordered rows plus the indices to pre-check, ignoring ids absent from the catalog. `setup/command.rs` therefore resolves the repository root before dispatch, so a non-git directory fails before any prompt. For repository builds, `cli/build.rs` validates the `SCE_CLI_GENERATED_INPUT_DIR` payload and canonical-input inventories, copies the payload into Cargo `OUT_DIR/pkl-generated`, stages `cli/assets/hooks/**` under `OUT_DIR/static`, requires the staged `config/optional-workflows.json`, and generates both the setup manifest and the optional-workflow catalog (`optional_workflows.rs`, rejecting a manifest whose `schemaVersion` is not 1 or whose entries lack a non-empty `id`/`title`/`description`/`commandSlug`/`skillSlug`) in `OUT_DIR`; focused internal seams separate install-flow from prompt-flow logic; `cli/src/services/setup/command.rs` owns the `SetupCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Its install engine/orchestrator (`install_assets_for_concrete_target_with_rename`) installs each embedded asset individually: it stages the asset's content next to its final destination, removes only that exact destination file if one already exists, and swaps the staged content into place, with deterministic recovery guidance naming the failing asset's path on swap failure and no backup artifact creation; it never removes an integration target directory as a whole, so files a repository owns inside `.opencode`/`.claude`/`.pi` — including nested inside an SCE-owned subdirectory such as `skills/` or `commands/` — survive a setup run. For the two assets that are merge targets — the Claude target's `settings.json`, detected by `is_claude_settings_merge_target`, and the OpenCode target's `opencode.json`, detected by `is_opencode_config_merge_target` — the content staged is not the embedded asset's bytes but the result of `cli/src/services/setup/config_merge.rs::merge_or_create_claude_settings`/`merge_or_create_opencode_config(existing_bytes, generated_bytes, source_path)`: each returns the generated document verbatim when no file exists yet, otherwise parses the existing file as JSON (a parse failure is a hard error naming `source_path`, and nothing is written) and merges it with a pure per-shape function that copies `$schema` from the generated document and preserves every other key from the existing file untouched. `merge_claude_settings` replaces, per hook event key the generated document declares, only the entries whose command contains the ownership marker `run-sce-or-show-install-guidance.sh`, preserving every event key and non-SCE hook entry from the existing file. `merge_opencode_config` merges the `plugin` array as a set: existing entries whose path starts with the ownership marker `./plugins/sce-` are dropped structurally — so a stale plugin path an older or renamed catalog once installed is still recognized and pruned even after the current generated document stops declaring it — and the generated document's `plugin` entries are appended after the surviving entries. After that per-asset install loop, `prune_stale_assets_for_concrete_target` deletes every path the full embedded catalog for the concrete target claims but the resolved selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), and `remove_empty_ancestor_directories` removes any parent directory left empty by that deletion, stopping at the target root or at a directory that still holds something such as a user file. It formats deterministic completion messaging; required-hook install orchestration (`install_required_git_hooks`) follows the same per-file stage/swap choreography (removing an existing hook file before swapping staged content, with deterministic recovery guidance on swap failure). After the Git gate, setup always ensures the context baseline; context-only requests return there, while normal modes derive a repo-root-scoped context before aggregating static lifecycle provider `setup` dispatch across providers (config → local_db → auth_db → agent_trace_db → hooks when requested), so setup providers consume only repo-root access from the scoped context. - `cli/src/services/setup/mod.rs` keeps those responsibilities inside one file for now, but the current ownership split is explicit: the inline `install` module owns repository-path normalization, staging/swap install behavior, required-hook installation, and filesystem safety guards, while the inline `prompt` module owns interactive target selection and prompt styling. - `cli/src/services/security.rs` provides shared security utilities for deterministic secret redaction (`redact_sensitive_text`) and directory write-permission probes (`ensure_directory_is_writable`) used by app/setup/observability surfaces. - `cli/src/services/doctor/mod.rs` owns the current doctor request/report surface while focused submodules (`doctor/inspect.rs`, `doctor/render.rs`, `doctor/fixes.rs`, `doctor/types.rs`) split report fact collection, rendering, manual fix reporting, and doctor-owned domain types into smaller seams; `cli/src/services/doctor/command.rs` owns the `DoctorCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Runtime doctor execution resolves a repository root, derives a scoped context, requests the shared static lifecycle provider catalog with hooks included for service-owned `diagnose` and `fix` behavior, adapts lifecycle-owned health/fix records into doctor-owned problem/fix records, and then renders stable text/JSON problem records with category/severity/fixability/remediation fields plus deterministic fix-result reporting in fix mode. Checkout DB discovery no longer lives in `doctor`; it moved to the `trace` group (`sce trace db list`) in `cli/src/services/trace/`. Report fact collection preserves environment/repository/hook/integration display data and checkout identity facts, while service-owned lifecycle providers own config validation, local DB and repository-scoped Agent Trace DB readiness/bootstrap, and hook rollout diagnosis/repair. Integration inspection in `doctor/inspect.rs` is scoped twice over: `resolve_doctor_integration_targets` picks which targets to inspect, and `persisted_optional_workflows` (reused from setup) resolves which optional workflows the repository selected, which the OpenCode/Claude/Pi child collectors apply through `iter_embedded_assets_for_setup_target_with_selection`. An unselected optional workflow therefore contributes no expected children at all, so no row and no missing/mismatch problem can be produced for it, while a selected one keeps the unchanged presence and content-hash checks. diff --git a/context/cli/cli-command-surface.md b/context/cli/cli-command-surface.md index df7970e9..4cb9ee51 100644 --- a/context/cli/cli-command-surface.md +++ b/context/cli/cli-command-surface.md @@ -62,7 +62,7 @@ An interactive `setup` run instead resolves the selection through an `inquire` m `auth` now emits auth-local guidance for bare `sce auth` and `sce auth --help`, listing `login`, `logout`, and `status` plus copy-ready next steps. `setup`, `doctor`, `hooks`, `version`, and `completion` all support command-local `--help`/`-h` usage output via top-level parser routing in `cli/src/app.rs`. `setup` now also exposes compile-time embedded config assets for OpenCode/Claude/Pi targets, sourced from the generated `config/.opencode/**`, `config/.claude/**`, and `config/.pi/**` trees via `cli/build.rs` with normalized forward-slash relative paths and target-scoped iteration APIs; the embedded asset set includes the OpenCode bash-policy plugin wrapper plus Claude settings `PreToolUse` Bash policy hook, both delegating to the Rust `sce policy bash` path. -`setup` additionally includes a repository-root install engine (`install_embedded_setup_assets`) that stages embedded files and uses a unified remove-and-replace policy for `.opencode/`/`.claude/`/`.pi/` (removing existing targets before swapping staged content, with deterministic recovery guidance on swap failure) while treating bash-policy enforcement files as first-class SCE-managed assets. +`setup` additionally includes a repository-root install engine (`install_embedded_setup_assets`) that installs each embedded asset individually into `.opencode/`/`.claude/`/`.pi/` — stage next to the final destination, remove only that destination file if present, swap into place, with deterministic recovery guidance naming the failing asset's path on swap failure — never removing an integration target directory as a whole, while treating bash-policy enforcement files as first-class SCE-managed assets. See [setup non-destructive per-asset install policy](../sce/setup-no-backup-policy-seam.md) for the full contract, including the pending pruning gap for deselected/stale assets. `setup` now executes end-to-end and prints deterministic completion details including selected target(s) and per-target install count. `doctor` now executes end-to-end with explicit diagnosis and repair-intent surfaces: `sce doctor` stays read-only and `sce doctor --fix` selects repair-intent mode. Agent Trace DB checkout discovery has moved out of `doctor`; `sce trace db list`, `sce trace status`, and `sce trace status --all` operate only on repository-scoped DBs (the checkout-scoped `--legacy` surface was removed by the `retire-legacy-agent-trace-db` plan); see [trace-command.md](trace-command.md). The current `doctor` runtime aggregates `ServiceLifecycle::diagnose` and `ServiceLifecycle::fix` calls across all registered service providers (`config`, `local_db`, `auth_db`, `agent_trace_db`, `hooks`) plus integration checks, covering state-root resolution, global and repo-local `sce/config.json` readability/schema validation, local DB and repository-scoped Agent Trace DB path/health, DB-parent readiness barriers, the repo hook rollout slice when a repository target is detected, and repo-root installed OpenCode, Claude, and Pi integration presence/content health for their embedded setup assets. Fix mode delegates to each provider's `fix` implementation, which reuses the canonical setup hook install flow to repair missing/stale/non-executable required hooks and missing hooks directories, and it can bootstrap missing canonical database parent directories when the resolved paths match canonical owned locations. A user-invocable `sync` command is not wired in the current CLI surface; local DB and Agent Trace DB bootstrap currently happen through `setup`, and DB health/repair currently happens through `doctor`. Command wiring for `sce sync` is deferred to `0.4.0`. diff --git a/context/context-map.md b/context/context-map.md index 5cef3cca..793f9139 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -50,7 +50,7 @@ Feature/domain context: - `context/sce/agent-trace-hook-doctor.md` (approved operator-environment contract for broadening `sce doctor` into the canonical health-and-repair entrypoint, including stable problem taxonomy, `--fix` semantics, checkout-aware Agent Trace DB reporting, setup-to-doctor alignment rules, canonical Git-hook payload restoration, and the approved downstream human text-mode layout/status/integration contract) - `context/sce/doctor-human-text-contract.md` (implemented `sce doctor` human text layout contract: section order, `[PASS]`/`[FAIL]`/`[MISS]` status vocabulary, simplified hook rows, target-scoped integration checks with configured/detected/empty target resolution, selection-scoped optional-workflow inventory read from `integrations.optional_workflows`, no-installed-integrations guidance, and OpenCode, Claude, plus Pi integration group rendering rules including the `Pi extensions` group) - `context/sce/setup-githooks-install-contract.md` (canonical `sce setup --hooks` install contract for target-path resolution, all-hook non-blocking missing-CLI bootstrap behavior, idempotent outcomes, remove-and-replace behavior, and doctor-readiness alignment) -- `context/sce/setup-no-backup-policy-seam.md` (implemented unified remove-and-replace install policy for both config-install and required-hook install flows, with no backup creation and deterministic recovery guidance on swap failure) +- `context/sce/setup-no-backup-policy-seam.md` (non-destructive per-asset install policy: config install writes/swaps each embedded asset individually and never removes an integration target directory as a whole, then prunes catalog-derived stale/deselected asset paths and any parent directory left empty by that pruning; required-hook install keeps the prior per-file remove-and-replace choreography; `.claude/settings.json` and `.opencode/opencode.json` are merge targets whose staged content is computed by JSON-merging the generated document into the user's existing one before the shared stage/swap step; no backup creation; deterministic recovery guidance naming the failing asset on swap failure) - `context/sce/setup-githooks-hook-asset-packaging.md` (compile-time `sce setup --hooks` required-hook template packaging contract, including all-hook non-blocking missing-`sce` install guidance, available-CLI argument forwarding, post-commit-only origin remote lookup plus remote-URL forwarding/fallback behavior, setup-service accessor surface, and current validation posture) - `context/sce/setup-githooks-install-flow.md` (setup-service required-hook install orchestration with git-truth hooks-path resolution, canonical missing-CLI payload installation, per-hook installed/updated/skipped outcomes, and remove-and-replace behavior with recovery guidance) - `context/sce/setup-githooks-cli-ux.md` (T04 composable `sce setup` target+`--hooks` / `--repo` command-surface contract, option compatibility validation, and deterministic setup/hook output semantics) diff --git a/context/glossary.md b/context/glossary.md index 099d5092..d6a960c4 100644 --- a/context/glossary.md +++ b/context/glossary.md @@ -155,8 +155,10 @@ - `sce policy command adapter`: Hidden/internal `sce policy bash` command in `cli/src/services/bash_policy.rs` that exposes the Rust bash-policy evaluator to hook callers. It reads JSON from STDIN, resolves bash-policy config from the project root (git root with current-directory fallback), evaluates the command against active policies, and emits hook-safe output: Claude Code deny JSON (`hookSpecificOutput` with `permissionDecision: "deny"`) or empty string for allowed commands in `--output claude-hook` mode (default), and structured `{"status","decision","command","normalized_argv","reason","policy_id"}` JSON in `--output json` mode. Input modes are `--input claude-pre-tool-use` (default, parses Claude `PreToolUse` event JSON with `tool_name`/`tool_input.command`) and `--input normalized` (parses `{"command":...}` for OpenCode delegation). The command uses explicit `--input`/`--output` flags rather than auto-detection; Claude Code hooks invoke `sce policy bash` with defaults, while OpenCode plugin delegation passes `--input normalized --output json`. Invalid invocation/input returns deterministic validation diagnostics without executing target commands. - `bash policy redundancy warning`: Non-fatal config validation output emitted when `forbid-git-all` and `forbid-git-commit` are enabled together; the config remains valid, but `sce config show|validate` reports the overlap deterministically as a warning instead of an error. - `auth config baked default`: Optional key-declared fallback in `cli/src/services/config/mod.rs` (with schema/parsing in `schema.rs`) used only after env and config-file inputs are absent; the first implemented case is `workos_client_id`, which currently falls back to `client_sce_default`. -- `setup install engine`: Installer in `cli/src/services/setup/mod.rs` (`install_embedded_setup_assets`) that writes embedded setup assets into per-target staging directories and swaps them into repository-root `.opencode/`/`.claude/` destinations, using a unified remove-and-replace policy that removes existing targets before swapping staged content. -- `setup remove-and-replace`: Replacement choreography in `cli/src/services/setup/mod.rs` where existing install targets are removed before staged content is promoted; on swap failure, the engine cleans temporary staging paths and returns deterministic recovery guidance (recover from version control). No backup artifacts are created. +- `setup install engine`: Installer in `cli/src/services/setup/mod.rs` (`install_embedded_setup_assets`) that writes each embedded setup asset into its own staged file next to its final destination under repository-root `.opencode/`/`.claude/`/`.pi/`, removing only that exact destination file if present, then swapping the staged content into place. It never removes an integration target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. After the per-asset install loop, it prunes stale SCE-owned paths (see `setup catalog-derived pruning`). Two assets, the Claude target's `settings.json` and the OpenCode target's `opencode.json`, stage merged content instead of the embedded asset's bytes verbatim (see `setup config-merge seam`). +- `setup catalog-derived pruning`: `prune_stale_assets_for_concrete_target` in `cli/src/services/setup/mod.rs`, run after per-asset install, deletes every path the full embedded catalog for a concrete target claims but the resolved selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), then `remove_empty_ancestor_directories` removes any parent directory left empty by that deletion, walking upward until it reaches the target root or a directory that still holds something (a user file inside an SCE-owned skill directory keeps that directory from being removed). Pruning is stateless and derives only from the compiled-in catalog; no install manifest is persisted, so an orphaned file installed by an older `sce` binary under a name the current catalog no longer knows is left alone. +- `setup config-merge seam`: Pure module `cli/src/services/setup/config_merge.rs`, covering `.claude/settings.json` and `.opencode/opencode.json`. Both `merge_or_create_claude_settings` and `merge_or_create_opencode_config` share the shape `(existing_bytes: Option<&[u8]>, generated_bytes: &[u8], source_path: &str) -> Result>`: each returns `generated_bytes` verbatim when no existing file is present; otherwise each parses both as JSON (an existing-file parse failure is a hard error naming `source_path`, with no write) and delegates to a pure merge function that copies the SCE-owned `$schema` key from the generated document and preserves every other top-level key from the existing document untouched. `merge_claude_settings` additionally replaces, per hook event key the generated `hooks` object declares, only the entries whose `hooks[].command` contains the SCE ownership marker `run-sce-or-show-install-guidance.sh`, appended after the surviving non-SCE entries; event keys the generated document does not declare are left untouched. `merge_opencode_config` additionally merges the `plugin` array as a set: existing entries whose path starts with the SCE ownership marker `./plugins/sce-` are dropped structurally (so a stale plugin path an older or renamed catalog installed is recognized and pruned even after the current generated document stops declaring it), then the generated document's `plugin` entries are appended. `install_single_asset_with_rename` (`mod install` in `cli/src/services/setup/mod.rs`) detects each merge target via `is_claude_settings_merge_target` / `is_opencode_config_merge_target` and stages the merged bytes instead of the embedded asset's bytes before the shared stage/swap step. The same module also exposes `claude_settings_fragment_is_current` / `opencode_config_fragment_is_current`, which merge `existing_bytes` into `generated_bytes` and compare the merged `Value` against the existing one: a no-op merge means the existing file already carries a current copy of the SCE-owned fragment, whatever else it holds. `cli/src/services/doctor/inspect.rs` uses these two functions (instead of byte-exact `sha256`) to inspect `.claude/settings.json` and `.opencode/opencode.json`, and `crate::services::setup::repair_merge_target_asset` lets `sce doctor --fix` repair a drifted merge target by reinstalling just that one asset through the same merge-install path. +- `setup remove-and-replace`: Per-file replacement choreography in `cli/src/services/setup/mod.rs` where an existing destination file is removed before staged content is swapped into its place; on swap failure, the engine cleans that file's staging artifact and returns deterministic recovery guidance naming its path (recover from version control). No backup artifacts are created. Config install (`install_embedded_setup_assets`) applies this at individual-asset granularity and never removes an integration target directory as a whole; required-hook install applies it per hook file. - `hooks command routing contract`: Current hook command parser/dispatcher plus runtime wiring in `cli/src/services/hooks/mod.rs` (`HookSubcommand`, `run_hooks_subcommand`) supports `pre-commit`, `commit-msg `, `post-commit`, `post-rewrite `, `diff-trace`, and `conversation-trace` with deterministic invocation validation/usage errors; `session-model` is no longer supported. `commit-msg` is the only active attribution path behind the attribution hooks gate and staged-diff AI-overlap preflight; `pre-commit`/`post-rewrite` are no-ops; `post-commit` persists intersections and built Agent Trace payloads; `diff-trace` persists DB-only AgentTraceDb rows using tool-prefixed `session_id` values plus direct `model_id` and `tool_version` values (session-model fallback was removed in the `remove-session-models-direct-claude-model-id` plan). - `Claude raw hook capture (removed)`: Former hidden/internal `sce hooks claude-capture ` intake path removed in T05 of the `claude-typescript-model-cache-remove-rust-capture` plan. Rust now exposes `diff-trace` and `conversation-trace` intakes for active Claude/OpenCode editor runtimes; `session-model` is also removed from the supported hook command surface. The removed route previously wrote pretty-printed JSON artifacts under `context/tmp/claude/` without AgentTraceDb writes. See `context/sce/claude-raw-hook-capture.md`. - `deferred sync command`: `sce sync` has no command wiring and no `cli/src/services/sync.rs` module in the current runtime. Local DB initialization and health ownership are split between setup and doctor instead. diff --git a/context/overview.md b/context/overview.md index 8863c65f..0cbed150 100644 --- a/context/overview.md +++ b/context/overview.md @@ -25,7 +25,7 @@ Agent Trace lifecycle setup now resolves repository storage, creates/reuses chec The CLI now also includes a shared text styling service in `cli/src/services/style.rs` that provides deterministic color enablement via `owo-colors`, automatic TTY detection, and `NO_COLOR` compliance for human-facing text output; stdout help/text surfaces, stderr diagnostics, and interactive prompt-adjacent text now reuse that shared styling policy while JSON, completion, and other non-interactive/machine-readable flows remain unstyled. The service exports color-detection, conditional styling, help/diagnostic/label/prompt styling, and `banner_with_gradient()` helpers for use across command surfaces while preserving pipe-safe output for non-interactive environments. The `setup` command includes an `inquire`-backed target-selection flow: default interactive selection for OpenCode/Claude/Pi/All with required-hook installation in the same run, explicit non-interactive target flags (`--opencode`, `--claude`, `--pi`, `--all`), standalone `--bootstrap-context` for additive durable-context baseline creation without integration installs, deterministic mutually-exclusive validation, and non-destructive cancellation exits; the former `--both` flag was removed in favor of `--all` (opencode+claude+pi). Every normal successful setup path also ensures the same context baseline after the Git gate. Workflows the catalog marks optional are installed only when a repository opts in: the repeatable `sce setup --workflow ` flag names the selection for a run, an omitted flag reuses the selection persisted in `integrations.optional_workflows`, and the resolved selection filters the installed target assets and is written back to repo-local config. Interactive runs ask for the selection instead: a multi-select prompt follows target selection with every row unchecked on a first run and pre-checked from the persisted selection afterwards, cancelling it exits non-destructively like the target prompt, and the prompt is skipped when the catalog marks no workflow optional. `brownfield` is currently the only optional workflow, so a default run installs the five core workflows and no brownfield assets. `sce doctor` scopes its integration checks to that same recorded selection, so it never reports an unselected optional workflow's files as missing. For repository generation consumers, `config/pkl/generator-inputs.txt` declares the canonical Pkl/plugin input set and `scripts/produce-cli-generated-input.sh` owns its discovery, two-pass `config/pkl/generate.pkl` evaluation, determinism comparison, payload/input inventories, in-flight input-mutation rejection, atomic handoff publication, and staging cleanup. `scripts/run-cli-cargo.sh` creates a fresh temporary destination, delegates generation to that producer, invokes the requested Cargo workflow with `SCE_CLI_GENERATED_INPUT_DIR`, and removes the handoff after Cargo success, failure, or handled signals. `config/pkl/check-generated.sh` delegates the same production mechanics while retaining contract and path assertions. `scripts/prepare-cli-generated-assets.sh` moves the producer-validated Pkl payload and checksums into the unchanged package fallback, adds hooks, migrations, and the Agent Trace schema, and appends only those static checksums to the combined inventory. The root flake's pre-Cargo `cliGeneratedInput` derivation invokes the same producer from a declarative source containing the producer plus its declared inputs. `cli/build.rs` rejects missing, incomplete, modified, or stale repository handoffs, copies the validated payload into Cargo `OUT_DIR/pkl-generated`, stages static inputs under `OUT_DIR/static`, and writes setup-asset, optional-workflow-catalog, and migration Rust manifests into `OUT_DIR`; it never invokes Pkl. Published crates carry the ignored packaging-only fallback, and unpacked downstream builds validate and copy it into their own `OUT_DIR` without requiring Pkl or parent repository paths. -The setup service also provides repository-root install orchestration: it resolves the repository root, ensures the additive durable-context baseline, then for normal modes derives a repo-root-scoped `AppContext` from the runtime command context, aggregates `ServiceLifecycle::setup` calls across lifecycle providers (config → local_db → auth_db → agent_trace_db → hooks when requested), handles interactive or flag-based target selection for config asset installation, and reports deterministic completion details (selected target(s) and installed file counts). Setup uses a unified remove-and-replace policy for all write flows — it removes existing targets before swapping staged content and returns deterministic recovery guidance (recover from version control) on swap failure, without creating backup artifacts. The setup command gates all modes on an existing git repository before any writes. Internally, `cli/src/services/setup/mod.rs` now separates install-flow logic from interactive prompt logic through focused support seams. +The setup service also provides repository-root install orchestration: it resolves the repository root, ensures the additive durable-context baseline, then for normal modes derives a repo-root-scoped `AppContext` from the runtime command context, aggregates `ServiceLifecycle::setup` calls across lifecycle providers (config → local_db → auth_db → agent_trace_db → hooks when requested), handles interactive or flag-based target selection for config asset installation, and reports deterministic completion details (selected target(s) and installed file counts). Setup installs config assets (`.opencode`/`.claude`/`.pi`) per file: each embedded asset is staged and swapped into its own destination path, creating parent directories as needed, without removing or recreating the target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. Two assets are merge targets rather than verbatim writes: Claude's `.claude/settings.json` and OpenCode's `.opencode/opencode.json`. For each, setup JSON-merges the generated document into the user's existing file rather than overwriting it, and fails deterministically without writing if the existing file is not valid JSON; a missing file is still created from the generated document verbatim. Claude's merge replaces only SCE-owned hook entries (identified by a command containing `run-sce-or-show-install-guidance.sh`) and the `$schema` key while preserving every other key and hook entry untouched. OpenCode's merge replaces the `$schema` key and merges the `plugin` array as a set: any entry shaped like an SCE plugin path (`./plugins/sce-*`) is dropped, structurally, so a path an older or renamed catalog once installed is still recognized and pruned, and the generated document's canonical plugin entries are appended after the surviving user entries. Required-hook install keeps the prior remove-and-replace choreography at file granularity — it removes an existing hook file before swapping staged content. Both flows return deterministic recovery guidance (recover from version control) on swap failure, without creating backup artifacts. After installing, config install prunes stale SCE-owned assets: it deletes every path the full embedded catalog for the target claims but the current selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), then removes any parent directory left empty by that deletion, leaving a directory intact if a user file still lives inside it. The setup command gates all modes on an existing git repository before any writes. Internally, `cli/src/services/setup/mod.rs` now separates install-flow logic from interactive prompt logic through focused support seams. The CLI now also applies baseline security hardening for reliability-driven automation: diagnostics/logging paths use deterministic secret redaction, `sce setup --hooks --repo ` canonicalizes and validates repository paths before execution, and setup write flows run explicit directory write-permission probes before staging/swap operations. The config service now provides deterministic runtime config resolution with explicit precedence (`flags > env > config file > defaults`), strict config-file validation (`$schema`, `log_level`, `log_format`, `log_dir`, `timeout_ms`, `workos_client_id`, and nested `policies.bash`, `policies.attribution_hooks.enabled`, plus `policies.database_retry` with per-DB `connection_open`/`query` retry policy specs), deterministic default discovery/merge of global+local config files (`${config_root}/sce/config.json` then `.sce/config.json` with local override, where `config_root` comes from the shared default-path seam with XDG/`dirs::config_dir()` config-root resolution), defaults for the resolved observability value set (`log_level=error`, `log_format=text`, `log_dir=/sce/logs`), shared auth-key resolution with optional baked defaults starting at `workos_client_id`, first-class bash-policy preset/custom parsing with deterministic conflict and duplicate-prefix validation, custom-policy `satisfied_by` wrapper exemption (a policy does not fire when the matched command was unwrapped from a declared wrapper such as `nix shell nixpkgs#ripgrep`), and a canonical Pkl-authored `sce/config.json` JSON Schema generated beneath Cargo `OUT_DIR` and embedded by `cli/src/services/config/mod.rs` for both `sce config validate` and doctor-time config checks. Runtime startup config loading keeps parity with that schema by accepting its `$schema` declaration in repo-local and global config files, so startup commands such as `sce version` no longer fail before dispatch on that field; the canonical declaration is `"https://sce.crocoder.dev/config.json"`. App-runtime observability now consumes flat logging keys through the shared resolver, so env values still override config-file values while config files provide deterministic fallback for `log_dir`; positive-integer `log_file_retention_limit` uses config-file/default precedence, defaults to `10`, and controls creation-triggered cleanup for primary and v2 log files; `sce config show` reports resolved observability/auth/policy values with provenance, while `sce config validate` is now a trimmed validation surface that reports only pass/fail plus validation errors or warnings in text and JSON modes. The canonical preset catalog and matching contract live in `config/pkl/base/bash-policy-presets.pkl` and `context/sce/bash-tool-policy-enforcement-contract.md`. Invalid default-discovered config files now also degrade gracefully at startup: `sce` keeps running with degraded observability defaults, logs `sce.config.invalid_config` warnings, and reserves hard failures for explicit `--config` / `SCE_CONFIG_FILE` targets or other truly invalid runtime observability inputs. diff --git a/context/patterns.md b/context/patterns.md index a5bda2c9..bdf860f9 100644 --- a/context/patterns.md +++ b/context/patterns.md @@ -51,7 +51,7 @@ - Derive accepted ids, error text, and asset membership from the catalog rather than enumerating workflows in code, so marking another workflow optional in Pkl needs no new branch. - Reject an unknown id during request resolution, before any file or config write. - Let health checks derive their expectations from the persisted selection through the same filter installation uses, so what a run installs and what a later check requires cannot drift apart. -- Treat an unrecorded selection as "nothing selected" for inspection, and do not report a deselected workflow's leftover files as stray; remove-and-replace installs already clear them. +- Treat an unrecorded selection as "nothing selected" for inspection, and do not report a deselected workflow's leftover files as stray; catalog-derived pruning after per-asset install already clears them. ## Dev-shell fallback shims for unavailable nixpkgs tools @@ -136,7 +136,8 @@ - Treat setup prompt cancellation/interrupt as a non-destructive exit path with explicit user messaging (no file mutations and no partial side effects). - For repository setup-asset build prep, declare canonical generator inputs in `config/pkl/generator-inputs.txt` and route input discovery, two-pass Pkl evaluation, determinism comparison, payload/input inventory creation, in-flight input checks, atomic publication, and private staging cleanup through `scripts/produce-cli-generated-input.sh`. The Cargo wrapper, generated-output check, package-fallback helper, and Nix `cliGeneratedInput` derivation must consume that producer rather than implement those mechanics independently. Keep each consumer's domain checks separate: the generated-output check owns metadata/contract/negative/path assertions; packaging owns static hook/schema/migration staging and the combined Pkl-plus-static checksum inventory; Nix owns declarative producer/input source selection and pre-Cargo handoff wiring. Route build, run, targeted-test, Clippy, and local-install Cargo workflows through `scripts/run-cli-cargo.sh`, which passes the producer handoff through `SCE_CLI_GENERATED_INPUT_DIR` and owns cleanup around Cargo. Keep `cli/build.rs` free of Pkl subprocesses and source-tree generated mirrors. - For CLI database migration prep, keep SQL files under immediate `cli/migrations//` directories named `NNN_description.sql`; `cli/build.rs` stages those files under `OUT_DIR/static/migrations`, sorts by the numeric prefix before `_`, and writes deterministic `OUT_DIR/generated_migrations.rs` constants with `include_str!` references for service `DbSpec` consumers. -- For setup install execution, write selected embedded assets into a per-target staging directory first, then remove the existing target and swap staged content into place; on swap failure, clean temporary staging paths and return deterministic recovery guidance (recover from version control). No backup artifacts are created. +- For setup install execution, write each selected embedded asset into its own staging file next to its final destination, remove only that destination file if one already exists, then swap the staged content into place; never remove or recreate the integration target directory as a whole. On swap failure, clean the failing asset's staging path and return deterministic recovery guidance naming that asset's destination (recover from version control). No backup artifacts are created. After the install loop, prune stale SCE-owned paths by diffing the full embedded catalog for the target against the assets actually installed, deleting each catalog path not installed, then removing any parent directory left empty by that deletion (a directory still holding a user file fails to remove and survives). +- For a config asset a user may already own and extend (`.claude/settings.json`, `.opencode/opencode.json`), do not write the embedded asset's bytes verbatim: compute the bytes to stage with a pure `serde_json`-based merge (`cli/src/services/setup/config_merge.rs`) that copies SCE-owned keys/entries from the generated document — identified by a fixed ownership marker, such as a hook command substring for Claude hooks or a plugin path prefix for OpenCode plugins — over the existing file, and preserves every other key and entry untouched. A parse failure on the existing file is a hard, deterministic error naming the file's path with no write; a missing file still gets the generated document verbatim. Keep this pure and filesystem-free per "Unit testing in Nix sandbox" below; the install seam reads the existing file and calls the merge before staging. - For required-hook setup execution, resolve repository root and effective hooks directory from git (`rev-parse --show-toplevel`, `rev-parse --git-path hooks`), then apply deterministic per-hook outcomes (`Installed`, `Updated`, `Skipped`) with staged writes, executable-bit enforcement, and remove-and-replace behavior that removes existing hooks before swapping staged content. - For hook setup CLI UX, allow `--hooks` as both hooks-only and composable target+hooks execution (optional `--repo `), enforce deterministic option compatibility (`--repo` requires `--hooks`; target flags stay mutually exclusive), and emit stable section-ordered setup/hook status lines for automation-friendly logs. - For setup command messaging, emit deterministic completion output that includes selected target(s) and per-target install counts. diff --git a/context/plans/non-destructive-setup-install.md b/context/plans/non-destructive-setup-install.md new file mode 100644 index 00000000..e4dc7029 --- /dev/null +++ b/context/plans/non-destructive-setup-install.md @@ -0,0 +1,242 @@ +# Plan: non-destructive-setup-install + +## Change summary + +`sce setup --claude|--opencode|--pi|--all` currently destroys everything in the +target integration directory. `install_assets_for_concrete_target_with_rename` +(`cli/src/services/setup/mod.rs:1192`) stages the embedded SCE assets into a +temporary root, calls `remove_existing_install_target` on the whole `.claude/`, +`.opencode/`, or `.pi/` directory, then renames staging into place. A repository +whose `.claude/` holds the user's own skills, agents, commands, +`settings.local.json`, or `CLAUDE.md` loses all of it on a routine setup run. The +two generated JSON configs (`.claude/settings.json`, `.opencode/opencode.json`) +are the same problem one level down: they are written whole, so a user's +`permissions`, `env`, `model`, `mcp`, or non-SCE hook entries are replaced by the +SCE-only document. + +This plan replaces the directory-level remove-and-replace policy with per-asset +installation plus catalog-derived pruning of SCE-owned paths, and adds JSON-aware +merging for the two generated config files so SCE-owned fragments are installed +into the user's document instead of over it. It preserves the existing swap +choreography (stage, then atomic rename) at file granularity, the existing +no-backup policy, and the existing optional-workflow deselection semantics — the +latter moves from "the whole tree is rebuilt" to "unselected catalog paths are +pruned". `sce doctor` integration checks are realigned in the same change, since +byte-exact `sha256` comparison stops being the right check for a merged file. + +## Acceptance criteria + +How this plan is proven complete. Each criterion is observable and names the +check that proves it. `/validate` runs these checks; no task in the stack +performs final validation. + +- [x] AC1: Running setup for a target leaves every file in that target directory + that SCE does not own exactly as it was — contents, mode, and mtime — including + files nested inside SCE-owned parent directories such as + `.claude/skills/my-own-skill/SKILL.md`. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — the setup integration tests seed a target directory with user-owned files at top level, inside `skills/`, and inside `commands/`, run install, and assert every seeded file survives byte-identical. +- [x] AC2: Running setup twice with an optional workflow selected and then + deselected leaves no file of the deselected workflow on disk, and still leaves + every unrelated file intact. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — a test installs with `brownfield` selected, reinstalls with an empty selection, and asserts the brownfield command file and skill directory are gone while a sibling user-owned skill directory remains. +- [x] AC3: Installing into an existing `.claude/settings.json` that carries user + keys (`permissions`, `env`) and a user-authored hook entry yields a document + that still carries those keys and that hook entry, plus exactly one current copy + of each SCE hook entry, with no duplicate SCE entries after repeated runs. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — merge tests assert key preservation, SCE-entry replacement, and idempotence across two consecutive installs. +- [x] AC4: Installing into an existing `.opencode/opencode.json` that carries user + keys and a user plugin path yields a document retaining both, with the canonical + SCE plugin paths present exactly once and no stale SCE plugin path left behind. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — merge tests assert user-key and user-plugin preservation plus SCE plugin path reconciliation. +- [x] AC5: `sce doctor` reports `[PASS]` for a target whose merged JSON configs + carry extra user content, and reports drift only when an SCE-owned fragment is + missing or stale. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` plus a manual run of `sce doctor` in a checkout whose `.claude/settings.json` has a user `permissions` block — the `Claude` integration group shows `[PASS]`. + +### Full validation + +- `nix flake check` +- `nix run .#pkl-check-generated` + +### Context sync + +- `context/sce/setup-no-backup-policy-seam.md` — this file currently states + directory-level remove-and-replace as the unified policy for both config + install and hook install. It must describe per-asset install, catalog-derived + pruning, JSON merge targets, and the fact that the policy now differs between + config install and required-hook install. +- `context/sce/setup-repo-local-config-bootstrap.md` — the optional-workflow + section states deselection is expressed "under the existing remove-and-replace + policy"; it must state catalog-derived pruning instead. +- `context/patterns.md` — the "For setup install execution" bullet and the + optional-workflow "remove-and-replace installs already clear them" bullet both + encode the old policy. +- `context/overview.md` — the setup paragraph describing the unified + remove-and-replace policy. +- `context/sce/doctor-human-text-contract.md` — if the drift vocabulary for + merge-target files changes in T05. +- `context/sce/generated-opencode-plugin-registration.md` — the generated + `opencode.json` is now a merge fragment, not a whole-file payload. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/setup/mod.rs` (install flow, staging, pruning, + merge seam), a new JSON-merge module under `cli/src/services/setup/`, + `cli/src/services/doctor/inspect.rs` integration-asset inspection, and the + durable-context files named under Context sync. +- **Out of scope:** `install_required_git_hooks` and the `.git/hooks/*` payloads. + Those still remove and replace an existing `pre-commit`/`commit-msg`/`post-commit` + file wholesale; chaining shell hooks is a different problem (see Open questions). +- **Out of scope:** `.sce/config.json`, which is already create-if-missing with + additive key writes and needs no change. +- **Out of scope:** Pkl authoring and the generated payload. Generation stays + byte-identical; `nix run .#pkl-check-generated` must keep passing unchanged. +- **Constraints:** No backup artifacts (`context/sce/setup-no-backup-policy-seam.md`). + No new crate dependency — `serde_json` is already a CLI dependency and is + sufficient for the merge work. Unit tests must stay filesystem-free + (`context/patterns.md`, "Unit testing in Nix sandbox"): merge logic is pure and + unit-tested; install/prune behavior belongs in integration tests. +- **Non-goal:** A general-purpose declarative config-merge engine. Two files, two + known shapes, one shared ownership marker. +- **Non-goal:** A persisted install manifest. Pruning is derived from the + compiled-in asset catalog (see Open questions for the residue this leaves). + +## Assumptions + +- SCE-owned files are overwritten without asking. A user who edited + `.claude/skills/sce-commit/SKILL.md` loses that edit, exactly as today. "Unrelated" + in the change request means files SCE never authored, not SCE files a user + modified. +- On a merge conflict inside a JSON config, the SCE-owned value wins for + SCE-owned keys and entries; every other key and entry is preserved untouched. + Setup cannot do its job otherwise. +- SCE ownership inside `.claude/settings.json` is identified by the hook command + string containing `run-sce-or-show-install-guidance.sh`, which every generated + Claude hook entry routes through (`config/pkl/renderers/claude-content.pkl:8`). +- SCE ownership inside `.opencode/opencode.json` is identified by a `plugin` entry + matching a canonical SCE plugin path (`./plugins/sce-bash-policy.ts`, + `./plugins/sce-agent-trace.ts`), authored in `config/pkl/base/opencode.pkl`. +- A malformed pre-existing JSON config is a hard error with actionable guidance, + not a silent overwrite. Silently replacing an unparseable user file is the same + data loss this plan exists to remove. +- Pruning stays stateless and catalog-derived, with no persisted install + manifest. Orphan files installed by an older `sce` under names the current + binary no longer knows are accepted residue — decided by the user when this was + raised as an open question. + +## Task stack + +- [x] T01: `Install setup assets per file instead of replacing the target directory` (status:done) + - Task ID: T01 + - Goal: `sce setup` writes each embedded asset into its own path under the target directory, creating parent directories as needed, and never removes the target root or any path it did not author. + - Boundaries (in/out of scope): In — `install_assets_for_concrete_target_with_rename` and its staging/swap helpers in `cli/src/services/setup/mod.rs`; per-file stage-then-rename with cleanup of the staging file on failure; the existing writability probe and recovery guidance retargeted to the individual asset path. Out — pruning stale SCE paths (T02), JSON merging (T03/T04), doctor (T05), git hooks. + - Dependencies: none + - Done when: installing into a target directory seeded with user-owned files at the top level, inside `skills/`, and inside `commands/` leaves every seeded file byte-identical while every embedded asset for the selected set is present with correct content; `remove_existing_install_target` is no longer called on an integration root; swap failure on one asset still cleans its staging artifact and returns recovery guidance naming that asset path. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml`. + - Implementation evidence: `install_assets_for_concrete_target_with_rename` (`cli/src/services/setup/mod.rs`) no longer stages a whole directory and swaps it over `destination_root`. It now loops over each embedded asset and calls new `install_single_asset_with_rename`, which stages a per-asset temp file next to the asset's real destination (`create_asset_staging_path`, mirroring the existing hook-install staging pattern), removes only that single existing destination file if present (bailing instead of deleting if the destination is unexpectedly a directory), then renames the staged file into place. Staging cleanup and `setup_install_recovery_guidance` are retargeted to the individual asset path on failure. Dead whole-directory helpers `create_staging_root` and `write_assets_to_staging` were removed; `remove_existing_install_target` is retained only for the untouched, out-of-scope git-hooks path. `install_embedded_setup_assets_with_rename` was widened from private to `pub(super)` so tests can inject a failing `rename_fn`. + - Verification evidence: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 16 passed, including two new tests: `install_preserves_user_owned_files_and_writes_sce_assets` (seeds top-level, `skills/`, and `commands/` user files in `.claude`, installs, asserts all three survive byte-identical and `commands/next-task.md` matches the embedded catalog bytes) and `install_cleans_up_staging_and_reports_asset_path_on_rename_failure` (forces a rename failure for `commands/next-task.md` via the injected `rename_fn`, asserts the error names that destination path and includes "does not create backups", and asserts no leftover `.sce-setup-staging-` file in that asset's parent directory). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` — clean. + - Deviations/assumptions: None beyond the review assumptions already recorded in the plan. + +- [x] T02: `Prune unselected and stale SCE-owned asset paths after install` (status:done) + - Task ID: T02 + - Goal: Restore deselection and stale-asset cleanup, which T01 removed, by deleting exactly those paths the full embedded catalog for the target claims but the resolved selection does not install. + - Boundaries (in/out of scope): In — a prune step in `cli/src/services/setup/mod.rs` computing `full catalog for target` minus `installed set`, deleting each such file, and removing SCE-owned skill directories left empty by that deletion. Out — deleting any path outside the compiled-in catalog; persisted install manifests; merge targets, which are never pruned because the file is shared with the user. + - Dependencies: T01 + - Done when: installing with `brownfield` selected and then reinstalling with an empty selection removes `.claude/commands/brownfield.md` and `.claude/skills/sce-brownfield/` entirely, leaves a sibling user-owned `.claude/skills/my-skill/` untouched, and leaves a user file placed inside an SCE-owned skill directory intact (so that directory is not removed as empty). + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`. + - Implementation evidence: `install_assets_for_concrete_target_with_rename` (`cli/src/services/setup/mod.rs`) now calls new `prune_stale_assets_for_concrete_target` after the per-asset install loop. It diffs `embedded_assets_for_concrete_target` (the full unfiltered catalog for the concrete target) against the just-installed `assets` slice by `relative_path`, and removes the destination file for every catalog path not in that installed set (a no-op when the file is already absent, covering assets an older or renamed catalog left behind). Each successful removal calls new `remove_empty_ancestor_directories`, which walks upward from the removed file's parent directory calling `fs::remove_dir` until it reaches `destination_root` or a directory removal fails (a non-empty directory, such as one still holding a user file, fails `fs::remove_dir` and stops the walk, so it survives). `embedded_assets_for_concrete_target` was added to the `install` submodule's `use super::{...}` import list; no other signature changed. + - Verification evidence: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 18 passed, including two new tests: `reinstall_with_empty_selection_prunes_deselected_workflow_without_touching_sibling_skill` (installs Claude with `brownfield` selected, seeds a sibling `.claude/skills/my-skill/SKILL.md`, reinstalls with an empty selection, asserts `.claude/commands/brownfield.md` and `.claude/skills/sce-brownfield/` are both gone entirely and the sibling skill file is untouched) and `reinstall_with_empty_selection_keeps_pruned_skill_dir_holding_a_user_file` (same flow but seeds `.claude/skills/sce-brownfield/MY_OVERRIDE.md` before reinstalling, asserts the SCE `SKILL.md` is pruned, the directory survives because it still holds the user file, and that file's content is intact). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` — clean. + - Deviations/assumptions: None beyond the review assumptions already recorded in the plan. + +- [x] T03: `Merge SCE hook entries into an existing .claude/settings.json` (status:done) + - Task ID: T03 + - Goal: Install `.claude/settings.json` by merging the generated document into the user's existing one rather than replacing it, preserving every non-SCE key and hook entry. + - Boundaries (in/out of scope): In — a new pure merge module under `cli/src/services/setup/` (for example `config_merge.rs`) exposing a `serde_json`-based merge for the Claude settings shape; a merge-target classification for asset relative path `settings.json` in the Claude install path; deterministic error on an unparseable existing file. Out — OpenCode (T04); doctor (T05); any change to the generated Pkl payload. + - Done when: merging the generated document into a settings file carrying `permissions`, `env`, and a user `PreToolUse` hook entry yields a document retaining all three; SCE hook entries (identified by `run-sce-or-show-install-guidance.sh` in the command) are replaced rather than appended, so two consecutive installs produce byte-identical output; an SCE hook entry the current generated document no longer contains is removed; a missing file is created from the generated document verbatim; an unparseable existing file fails with a message naming the path and does not write. + - Dependencies: T01 + - Verification notes (commands or checks): pure merge unit tests in the new module (no filesystem); `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::config_merge`; `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`. + - Implementation evidence: New pure module `cli/src/services/setup/config_merge.rs` (declared via `mod config_merge;` in `cli/src/services/setup/mod.rs`) exposes `merge_or_create_claude_settings(existing_bytes: Option<&[u8]>, generated_bytes: &[u8], source_path: &str) -> Result>`. It returns `generated_bytes` verbatim when `existing_bytes` is `None`; otherwise it parses both as JSON (a parse failure on the existing document is a hard error naming `source_path`) and calls pure `merge_claude_settings(&Value, &Value, &str) -> Result`, which copies `$schema` from the generated document (SCE-owned), and for each event key the generated `hooks` object declares (currently `PreToolUse`, `PostToolUse`, `UserPromptSubmit`, `Stop`), replaces only the SCE-owned entries in `existing.hooks[event]` — identified via `hook_entry_is_sce_owned`, which checks whether any `hooks[].command` contains the marker `run-sce-or-show-install-guidance.sh` — with the generated entries for that event, appended after the surviving non-SCE entries; every other top-level key and every hook event key the generated document does not declare are left untouched. The result is re-serialized with `serde_json::to_string_pretty` plus a trailing newline. In `mod install` (`cli/src/services/setup/mod.rs`), `install_single_asset_with_rename` gained a new `is_claude_settings_merge_target(target, relative_path)` check (true only for `SetupTarget::Claude` + `claude_asset::SETTINGS_FILE`); when true it reads the existing destination bytes (if the file exists) before staging, computes `install_bytes` via `config_merge::merge_or_create_claude_settings`, and stages/renames those bytes instead of `asset.bytes` directly — the rest of the stage-then-rename, cleanup, and `setup_install_recovery_guidance` behavior is unchanged. + - Verification evidence: `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — clean. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::config_merge` — 7 passed, covering: user-key/non-SCE-hook-entry preservation, SCE-entry replacement producing byte-identical output across two merges, an SCE hook entry the generated document no longer declares being dropped, a user-owned event key absent from the generated document being left untouched, a missing file returning the generated bytes verbatim, an unparseable existing file failing with an error naming the path, and a missing `hooks` key in the existing document being populated from generated. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 26 passed, including new integration test `install_merges_into_existing_claude_settings_json_and_stays_idempotent` (seeds `.claude/settings.json` with `permissions`, `env`, and a user `PreToolUse` hook entry, installs, asserts the user keys and hook entry survive alongside an SCE-owned `PreToolUse` entry, reinstalls, and asserts the two installs produce byte-identical `settings.json` content). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` — clean. + - Deviations/assumptions: Scoped the merge to only the four hook event keys the generator currently emits (`PreToolUse`, `PostToolUse`, `UserPromptSubmit`, `Stop`); a user-owned event key the generated document never declares (e.g. `Notification`) is left completely untouched by every merge, consistent with the plan's non-goal of a general-purpose config-merge engine. No other deviations beyond the review assumptions already recorded in the plan. + +- [x] T04: `Merge SCE plugin registrations into an existing .opencode/opencode.json` (status:done) + - Task ID: T04 + - Goal: Install `.opencode/opencode.json` by merging the canonical SCE `plugin` entries into the user's existing document, preserving every other key and plugin. + - Boundaries (in/out of scope): In — an OpenCode merge in the T03 module; classifying asset relative path `opencode.json` as a merge target in the OpenCode install path. Out — Claude (T03); doctor (T05); Pkl payload changes. + - Dependencies: T03 + - Done when: merging into a document carrying `model`, `mcp`, and a user plugin path retains all three, contains each canonical SCE plugin path exactly once after two consecutive installs, and drops a stale SCE-shaped plugin path the current catalog no longer declares; a missing file is created from the generated document verbatim; an unparseable existing file fails with a message naming the path and does not write. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::config_merge`; `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`. + - Implementation evidence: `cli/src/services/setup/config_merge.rs` gained `merge_or_create_opencode_config(existing_bytes: Option<&[u8]>, generated_bytes: &[u8], source_path: &str) -> Result>`, mirroring the Claude settings merge: returns `generated_bytes` verbatim when `existing_bytes` is `None`; otherwise parses both as JSON (an existing-document parse failure is a hard error naming `source_path`) and calls pure `merge_opencode_config(&Value, &Value, &str) -> Result`, which copies `$schema` from generated (SCE-owned) and, for `plugin`, filters `existing.plugin` down to entries that are not SCE-shaped — via new `plugin_entry_is_sce_owned`, which checks whether the string starts with new marker constant `OPENCODE_SCE_PLUGIN_PREFIX = "./plugins/sce-"` — then appends `generated.plugin`'s entries; ownership is matched structurally (by path shape) rather than by membership in the current generated array, so a plugin path an older or renamed catalog installed is still recognized and dropped even when the current generated document no longer declares it. Every other top-level key and any `plugin` entry not shaped like an SCE path are left untouched. In `mod install` (`cli/src/services/setup/mod.rs`), new `is_opencode_config_merge_target(target, relative_path)` (true only for `SetupTarget::OpenCode` + `default_paths::repo_file::OPENCODE_MANIFEST`, i.e. relative path `opencode.json`) gated a new branch in `install_single_asset_with_rename` alongside the existing Claude-settings branch: when true, it reads the existing destination bytes (if present) before staging, computes `install_bytes` via `config_merge::merge_or_create_opencode_config`, and stages/renames those bytes instead of `asset.bytes` directly. The install submodule's `use` list gained `crate::services::default_paths` (previously only `default_paths::claude_asset` was imported there) to resolve `repo_file::OPENCODE_MANIFEST`. + - Verification evidence: `./scripts/run-cli-cargo.sh build --manifest-path cli/Cargo.toml` — clean. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::config_merge` — 13 passed, including 7 new OpenCode-merge tests: user-key (`model`, `mcp`) and user-plugin preservation with the two canonical SCE plugin paths appended; idempotence across two merges producing an identical 2-entry `plugin` array; a stale SCE-shaped plugin path (`./plugins/sce-old-feature.ts`) not declared by the current generated document being dropped while a sibling user plugin and both canonical paths survive; a missing file returning the generated bytes verbatim; an unparseable existing file failing with an error naming the path; and a missing `plugin` key in the existing document being populated from generated. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 33 passed, including new integration test `install_merges_into_existing_opencode_config_json_and_stays_idempotent` (seeds `.opencode/opencode.json` with `model`, `mcp`, a user plugin path, and a stale SCE-shaped plugin path, installs, asserts the user keys and plugin survive, both canonical SCE plugin paths are present and the stale one is gone, reinstalls, and asserts the two installs produce byte-identical `opencode.json` content). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` — clean. + - Deviations/assumptions: SCE ownership of a `plugin` entry is identified structurally by the `./plugins/sce-` path prefix rather than by exact match against the two currently-canonical paths, so that a plugin path an earlier or renamed catalog once installed under that same directory convention is still recognized as SCE-owned and pruned even after the current generated document drops it — needed to satisfy the plan's "drops a stale SCE-shaped plugin path" done check, since a stale path by definition cannot be found by diffing against the current generated set. No other deviations beyond the review assumptions already recorded in the plan. + +- [x] T05: `Check merge-target configs by SCE-owned fragment in sce doctor` (status:done) + - Task ID: T05 + - Goal: `sce doctor` stops reporting drift for a merged JSON config that legitimately carries user content, and reports it only when an SCE-owned fragment is absent or stale. + - Boundaries (in/out of scope): In — `build_integration_child_from_asset` / `inspect_integration_asset_state` in `cli/src/services/doctor/inspect.rs`, so merge-target assets are inspected by SCE-fragment presence and equality instead of whole-file `sha256`; `--fix` for those assets reusing the T03/T04 merge install. Out — the doctor text layout vocabulary unless the fragment check needs a new state; every non-merge asset, which keeps byte-exact `sha256` checking; hook health checks. + - Dependencies: T03, T04 + - Done when: a `.claude/settings.json` merged with user `permissions` reports `[PASS]`; the same file with an SCE hook entry deleted or with a stale SCE hook command reports drift; `sce doctor --fix` repairs it by merging and leaves the user keys intact; `.opencode/opencode.json` behaves the same for SCE plugin entries. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::`; manual `sce doctor` in a checkout with a user-extended `.claude/settings.json`. + - Implementation evidence: `cli/src/services/setup/config_merge.rs` gained `pub(crate) fn claude_settings_fragment_is_current(existing_bytes, generated_bytes) -> Result` and `pub(crate) fn opencode_config_fragment_is_current(...)`, each parsing both documents, running the existing private `merge_claude_settings`/`merge_opencode_config`, and comparing the merged `Value` against the existing one — a no-op merge means the SCE-owned fragment is already current. `cli/src/services/setup/mod.rs` widened `mod config_merge;` to `pub(crate) mod config_merge;` and added `pub(crate) fn repair_merge_target_asset(repository_root, target, relative_path)`, which delegates to a new `install::repair_merge_target_asset` that looks up the one embedded asset by relative path in `embedded_assets_for_concrete_target` and reinstalls only that asset through the existing `install_single_asset_with_rename` (the same per-asset merge-install path T03/T04 wired up), leaving every other asset untouched. `cli/src/services/doctor/inspect.rs`: `build_integration_child_from_asset` now takes `Option<&MergeTargetAsset>` (new two-variant enum `ClaudeSettings`/`OpenCodeConfig`); for those two assets it calls new `inspect_merge_target_asset_state`, which reads the existing file and calls the matching fragment-check function (a read/parse failure or drifted fragment both surface as `Mismatch`, since remediation is the same either way); every other asset keeps the prior `sha256` path unchanged via `inspect_integration_asset_state`. New `repair_merge_target_configs(repository_root)` re-collects the Claude/OpenCode integration groups, and for each merge-target child currently in `Mismatch`, calls `repair_merge_target_asset` and records a `DoctorFixResultRecord`; a merge target already `Match` or fully `Missing` is left untouched (missing files stay covered by the existing "reinstall assets" guidance). `cli/src/services/doctor/mod.rs`'s `execute_doctor_with_lifecycle_providers` now calls `repair_merge_target_configs(repository_root)` during `--fix`, before re-diagnosing for the final report, alongside the existing lifecycle-provider fixes. + - Verification evidence: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::config_merge` — 21 passed, including 4 new fragment tests covering: a Claude settings file with extra user keys and a fully current SCE fragment reporting current, a deleted SCE hook entry reporting not-current, an OpenCode config with an extra user plugin reporting current, and a stale SCE-shaped plugin path reporting not-current. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` — 6 passed, including 3 new filesystem-backed tests: `claude_settings_reports_match_despite_extra_user_permissions` (user `permissions` alongside a current fragment reports `Match`), `claude_settings_reports_mismatch_when_sce_hook_entry_deleted_then_fix_repairs_it` (emptied hook arrays report `Mismatch`, `repair_merge_target_configs` fixes it, user `permissions` survive, and a second inspection reports `Match`), and the equivalent `opencode_config_reports_match_despite_extra_user_plugin_then_drift_and_fix` for `.opencode/opencode.json`. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 37 passed (no regressions). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` — clean. Manual verification: in a temp git checkout with `sce setup --claude` run, adding user `permissions`/`env` keys to `.claude/settings.json` and running `sce doctor` showed `[PASS] settings.json`; emptying `hooks.PreToolUse` showed `[FAIL] settings.json (... - content mismatch)`; `sce doctor --fix` printed `[fixed] Merged canonical SCE fragments into 'settings.json'.` and the subsequent `sce doctor` showed `[PASS]` again with the user `permissions`/`env` keys intact. + - Deviations/assumptions: A merge-target file that fails to parse as JSON is reported the same as a drifted fragment (`Mismatch`), not a distinct state, since the plan's boundaries keep new doctor vocabulary out of scope unless the fragment check needs it, and both cases point to the same remediation. `repair_merge_target_configs` only repairs a merge-target child already in `Mismatch`; a fully missing merge-target file is left to the existing generic "reinstall assets" missing-file guidance rather than being created in isolation by the fix path, since creating just that one file when the rest of the integration is absent would be a surprising partial repair. No other deviations beyond the review assumptions already recorded in the plan. + +## Open questions + +- `sce setup --hooks` still removes and replaces `.git/hooks/pre-commit`, + `commit-msg`, and `post-commit` wholesale, so a husky or lefthook repository + loses its hook on a setup run. Asked whether shell hooks can be merged: not + textually, but a dispatcher works. SCE would keep `.git/hooks/` as a thin + dispatcher, relocate a pre-existing foreign hook to `.git/hooks/.d/10-local` + preserving its mode, and have the dispatcher run every executable in `.d/` + in lexical order, abort on the first non-zero exit, then run the SCE logic. It is + tractable here because all three hooks have simple contracts — `pre-commit` and + `post-commit` take no arguments, `commit-msg` takes one message-file path, none + read stdin — and the ordering falls out correctly, with SCE's `commit-msg` last + so its trailer lands on the final message. Two caveats: husky and lefthook set + `core.hooksPath`, which `install_required_git_hooks` does not currently honour + (it resolves via `git rev-parse --git-path hooks`), so SCE and the manager write + to different directories until that resolution is fixed; and even where the paths + do collide, a manager reinstalling its own hooks overwrites the dispatcher, so the + scheme is cooperative rather than authoritative. Adding it means a marker line in + the hook templates, `core.hooksPath`-aware resolution, the dispatcher template, + and relocation logic — roughly two more tasks. Undecided: say whether to add them. + +## Validation Report + +**Status:** failed +**Date:** 2026-08-03 + +### Commands run + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` -> exit 0 (37 passed, 0 failed — covers AC1-AC4) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` -> exit 0 (6 passed, 0 failed — covers AC5's automated portion) +- Manual `sce doctor` in a temp checkout with a user-extended `.claude/settings.json` -> pass (AC5's manual portion: `[PASS] settings.json` with user `permissions`/`env` present; emptying `hooks.PreToolUse` produced `[FAIL] settings.json (... - content mismatch)`; `sce doctor --fix` reported `[fixed] Merged canonical SCE fragments into 'settings.json'.` and a follow-up `sce doctor` showed `[PASS]` again with the user keys intact) +- `nix flake check` -> exit 1 (`checks.x86_64-linux.cli-fmt` failed: `cargo fmt -- --check` reports unformatted diffs in `cli/src/services/setup/mod.rs` and `cli/src/services/setup/config_merge.rs`; `cli-clippy` and `cli-tests` build successfully in isolation) +- `nix run .#pkl-check-generated` -> exit 0 (Ephemeral Pkl generation passed: 101 files, inventory sha256 a1da453613edc8ecb1e04f35f37471ac02674bad5f2564ae70994e9f1acc6775) + +### Scaffolding removed + +- None. + +### Success-criteria verification + +- [x] AC1: Running setup for a target leaves every non-owned file in that target directory untouched, including files nested inside SCE-owned parent directories -> `install_preserves_user_owned_files_and_writes_sce_assets` passes +- [x] AC2: Deselecting an optional workflow removes only that workflow's files while leaving unrelated files intact -> `reinstall_with_empty_selection_prunes_deselected_workflow_without_touching_sibling_skill` and `reinstall_with_empty_selection_keeps_pruned_skill_dir_holding_a_user_file` pass +- [x] AC3: Installing into an existing `.claude/settings.json` preserves user keys and hook entries, with exactly one current SCE hook entry per event and no duplicates after repeated runs -> `install_merges_into_existing_claude_settings_json_and_stays_idempotent` and `config_merge` unit tests pass +- [x] AC4: Installing into an existing `.opencode/opencode.json` preserves user keys and plugin paths, with each canonical SCE plugin path present exactly once and no stale SCE plugin path left behind -> `install_merges_into_existing_opencode_config_json_and_stays_idempotent` and `config_merge` unit tests pass +- [x] AC5: `sce doctor` reports `[PASS]` for a target whose merged JSON configs carry extra user content, and reports drift only when an SCE-owned fragment is missing or stale -> `doctor::` tests pass; manual verification above confirms `[PASS]` with extra user content, drift detection on a deleted SCE hook entry, and `--fix` repair preserving user keys + +### Failed checks and follow-ups + +- `nix flake check` / `checks.x86_64-linux.cli-fmt`: `cargo fmt -- --check` fails against the current tree; evidence: the fmt derivation's build log shows reflow diffs across roughly a dozen sites in `cli/src/services/setup/mod.rs` (e.g. `is_opencode_config_merge_target`, several test bodies around lines 2090-2246) and `cli/src/services/setup/config_merge.rs` (test bodies around lines 350-469); required: run `cargo fmt --manifest-path cli/Cargo.toml` in a normal work session to reformat the affected files, then rerun `nix flake check`. Also required before any Nix check can see it: `cli/src/services/setup/config_merge.rs` was untracked in git going into this validation run (Nix flake source filtering only includes git-tracked files, so the module was invisible to `nix flake check` until staged) — it has been `git add`ed as part of this validation run; no file content was changed by that action. + +### Residual risks + +- None identified. + +### Retry + +After repairs, rerun: + +`/validate context/plans/non-destructive-setup-install.md` diff --git a/context/sce/doctor-human-text-contract.md b/context/sce/doctor-human-text-contract.md index 207ef256..15699eed 100644 --- a/context/sce/doctor-human-text-contract.md +++ b/context/sce/doctor-human-text-contract.md @@ -68,7 +68,7 @@ Human text output renders group rows only for the resolved targets: Within a resolved target, the required inventory is additionally scoped to the repository's optional-workflow selection. The doctor reads `integrations.optional_workflows` from `.sce/config.json`; an absent, unreadable, or key-less file means nothing is selected. There is no directory-detection fallback for optional workflows. An unselected optional workflow's command file and skill subtree are not part of the required inventory, so no child row and no missing-file problem is produced for them. A selected optional workflow's assets are required inventory like any core workflow's, keeping `[MISS]` and content-mismatch `[FAIL]` detection unchanged. Files belonging to a previously selected but now unselected optional workflow are not reported as stray; the doctor simply stops expecting them. See [setup local bootstrap](setup-repo-local-config-bootstrap.md). Integration checks for this contract inspect installed repo-root artifacts only. -They validate file presence and content hashes against embedded OpenCode, Claude, and Pi setup assets. +They validate file presence and content against embedded OpenCode, Claude, and Pi setup assets: byte-exact `sha256` for every asset except the two JSON configs `sce setup` installs by merge (`.claude/settings.json`, `.opencode/opencode.json`), which instead validate that the file's SCE-owned fragment matches the embedded catalog — a file that also carries extra user keys, permissions, or plugins still renders `[PASS]` as long as that fragment is current (see [non-destructive setup install merge seam](setup-no-backup-policy-seam.md)). Generated `config/.opencode/**`, `config/.claude/**`, and `config/.pi/**` trees are out of scope for doctor integration checks in this change stream. Claude installed assets are grouped by repo-root `.claude/` relative path: @@ -99,7 +99,8 @@ Integration child rows render as `[STATUS] relative/path (absolute/path)` in tex ## Non-goals for this contract slice - no JSON output shape or semantic changes -- no `sce doctor --fix` behavior changes - no Claude plugin registry or preset-catalog checks +These non-goals scoped the original text-contract slice only. A later plan (`non-destructive-setup-install` task `T05`) added `sce doctor --fix` behavior for the two merge-target JSON configs: when their SCE-owned fragment is missing or stale, `--fix` reinstalls just that one asset through the same per-asset merge-install path `sce setup` uses, leaving every other asset and every user key untouched. The status vocabulary and section order above are unchanged by that addition. + See also: [doctor operator contract](agent-trace-hook-doctor.md), [CLI command surface](../cli/cli-command-surface.md). diff --git a/context/sce/setup-no-backup-policy-seam.md b/context/sce/setup-no-backup-policy-seam.md index 437013a8..13bcba47 100644 --- a/context/sce/setup-no-backup-policy-seam.md +++ b/context/sce/setup-no-backup-policy-seam.md @@ -1,27 +1,28 @@ -# Setup remove-and-replace install policy +# Setup non-destructive per-asset install policy -`cli/src/services/setup/mod.rs` uses a unified remove-and-replace policy for all setup-managed write flows. There is no backup creation or backup-based rollback. +`cli/src/services/setup/mod.rs` installs every setup-managed file at file granularity: stage, remove the exact destination file if one exists, then swap the staged content into place. There is no backup creation or backup-based rollback, and setup-managed installs never remove an integration target directory as a whole. This per-file stage/swap choreography is shared by config install, required-hook install, and merge-target install; it is the JSON merge targets described below whose staged *content* differs from the embedded asset's bytes. ## Current state -- Both config install (`.opencode`/`.claude`) and required hook install use the same remove-and-replace choreography: - 1. Write canonical content to a unique staging file. - 2. Remove the existing target (if present) directly. - 3. Swap the staged content into the final target path. - 4. On swap failure, clean the staging artifact and return deterministic recovery guidance (recover from version control if needed). -- No `.backup` artifacts are created during any setup write flow. -- No backup-based rollback is attempted on swap failure. -- Recovery guidance is generic (not git-specific wording): "Setup does not create backups. Recover '' from version control if needed." - -## Implemented behavior - -- Config install removes the existing target directory before swapping staged content. On swap failure, it cleans the staging artifact and returns recovery guidance. -- Required hook install removes the existing hook file before swapping staged content. On swap failure, it cleans the staging artifact and returns recovery guidance. -- Success output reports target, file count, and per-hook status (`installed`/`updated`/`skipped`) without any backup-related lines. +- Config install (`.opencode`/`.claude`/`.pi`, `install_embedded_setup_assets` / `install_assets_for_concrete_target_with_rename`) writes every embedded asset to its own path under the target directory, creating parent directories as needed: + 1. Write the asset's canonical content to a unique staging file next to its final destination. + 2. If a file already exists at that exact destination path, remove only that file. If a directory exists there instead, fail with an actionable error instead of deleting it. + 3. Swap the staged content into the final destination. + 4. On swap failure, clean the staging artifact and return deterministic recovery guidance naming that asset's destination path (recover from version control if needed). +- Setup never removes an integration target directory (`.opencode`, `.claude`, `.pi`) as a whole, and never touches a path it did not author. Files a repository placed inside an SCE-owned target directory — at the top level or nested inside an SCE-owned subdirectory such as `skills/` or `commands/` — survive a setup run untouched. +- Required hook install (`install_required_git_hooks`) uses the same per-file stage/remove-if-present/swap choreography for each hook file; this predates and is unaffected by the config-install change above. +- After the per-asset install loop, config install prunes stale SCE-owned paths: `prune_stale_assets_for_concrete_target` diffs the full embedded-asset catalog for the concrete target against the assets this run actually installed, and deletes every catalog path present in the former but not the latter (deselected optional-workflow files, or an asset a newer catalog renamed or dropped). Each successful deletion is followed by `remove_empty_ancestor_directories`, which removes now-empty parent directories upward until it reaches the target root or hits a directory that still holds something (a directory holding a user file fails to remove and is left in place, so a user file nested inside an SCE-owned skill directory survives even though the SCE file next to it is pruned). Pruning is stateless and catalog-derived — no install manifest is persisted — so it only ever considers paths the compiled-in catalog still names. +- No `.backup` artifacts are created during any setup write flow, and no backup-based rollback is attempted on swap failure. +- Recovery guidance is generic (not git-specific wording): "Setup ... does not create backups. Recover '' from version control if needed." +- Two config assets are merge targets instead of verbatim-content assets: `.claude/settings.json` for the Claude target, and `.opencode/opencode.json` for the OpenCode target. `install_single_asset_with_rename` detects each (`is_claude_settings_merge_target`, `is_opencode_config_merge_target`) and, before staging, computes the bytes to stage from `cli/src/services/setup/config_merge.rs` rather than writing the embedded asset's bytes directly. Both merge functions return the generated document verbatim when no existing file is present; otherwise each parses the existing file as JSON (a parse failure is a hard error naming the file's path, and nothing is written) and merges the generated document into it, preserving every other top-level key untouched: + - `merge_or_create_claude_settings`: `$schema` and, event-by-event, every hook entry whose command contains the marker `run-sce-or-show-install-guidance.sh` are SCE-owned and replaced from the generated document; every hook entry or event key the generated document does not declare is preserved untouched. + - `merge_or_create_opencode_config`: `$schema` is SCE-owned and replaced from the generated document; the `plugin` array is merged as a set — any existing entry whose path starts with `./plugins/sce-` is dropped (structural ownership, so a plugin path an older or renamed catalog once installed is still recognized and pruned even after the current generated document stops declaring it), then the generated document's `plugin` entries are appended. + The merged bytes then flow through the same stage/remove-if-present/swap choreography as every other asset, so this is a content-computation seam layered on the shared install policy, not a different write path. +- `sce doctor --fix` reuses this same per-asset install path for the two merge targets rather than running its own repair logic: `crate::services::setup::repair_merge_target_asset` looks up the one embedded asset by relative path and reinstalls only it through `install_single_asset_with_rename`, so a drifted `.claude/settings.json` or `.opencode/opencode.json` is repaired by merge — every other installed asset and every user key is left untouched. `sce doctor` (diagnose or fix) tells a merge target's drift apart from a legitimately extended file by SCE-fragment equality (`config_merge::claude_settings_fragment_is_current`, `config_merge::opencode_config_fragment_is_current`) instead of the byte-exact `sha256` check every other integration asset uses (see [doctor human text contract](doctor-human-text-contract.md)). ## Scope boundary -- This file captures the unified remove-and-replace install policy and its use by both config-install and required-hook install flows. -- Future setup-managed write flows should follow the same remove-and-replace pattern instead of introducing backup creation. +- This file captures the non-destructive, per-file install policy shared by config-install and required-hook install flows, including the merge-target content-computation seam for `.claude/settings.json`. +- Future setup-managed write flows should follow the same per-file stage/remove-if-present/swap pattern instead of introducing backup creation or whole-directory replacement. A future merge target computes its staged content the same way `.claude/settings.json` does, ahead of the shared stage/swap step. -See also: [../overview.md](../overview.md), [../context-map.md](../context-map.md), [setup-githooks-install-flow.md](setup-githooks-install-flow.md) \ No newline at end of file +See also: [../overview.md](../overview.md), [../context-map.md](../context-map.md), [setup-githooks-install-flow.md](setup-githooks-install-flow.md) diff --git a/context/sce/setup-repo-local-config-bootstrap.md b/context/sce/setup-repo-local-config-bootstrap.md index 4a54cdca..d6b1270b 100644 --- a/context/sce/setup-repo-local-config-bootstrap.md +++ b/context/sce/setup-repo-local-config-bootstrap.md @@ -40,7 +40,7 @@ After config asset installation succeeds for a non-interactive target (`--openco The same write also records the run's resolved optional-workflow selection under `integrations.optional_workflows`: - Precedence: an interactively answered multi-select is the exact selection for the run; otherwise a supplied `--workflow ` list (repeatable) is; when neither is present the persisted `integrations.optional_workflows` is read back and reused, so a repeat `sce setup --claude --non-interactive` never silently uninstalls a previously selected optional workflow. The prompt's pre-checked rows come from the same persisted value (or from `--workflow` when it was supplied), so accepting the prompt unchanged records what a rerun would have kept. -- The resolved selection filters what is installed, so an unselected optional workflow's command file and skill directory are simply absent from the freshly installed target tree under the existing remove-and-replace policy. +- The resolved selection filters what is installed, and catalog-derived pruning (see [setup-no-backup-policy-seam.md](setup-no-backup-policy-seam.md)) removes an unselected optional workflow's command file and skill directory left behind by an earlier run, so deselection is effective on both a first-time install and a later reinstall. - A run that resolves to an empty selection records `[]`. Deselecting is therefore expressed by installing without that slug, not by a separate uninstall step. - The persisted set is repository-wide, not per target: a `--all` run records one selection covering `.opencode/`, `.claude/`, and `.pi/`. - Unknown slugs are rejected during request resolution, before any file or config write. From cb7da314e509ec8c049f2081d46c3ca927e385d3 Mon Sep 17 00:00:00 2001 From: David Abram Date: Fri, 7 Aug 2026 13:45:29 +0200 Subject: [PATCH 2/2] setup: Merge git hooks into a bounded managed block instead of overwriting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every setup write — config assets and required git hooks alike — unlinked the destination file before renaming the staged replacement over it. The staging file already lives in the destination's directory, so the rename alone is atomic; the unlink only opened a window where the file did not exist, and on a rename failure the error path deleted the staging copy too, leaving neither. This mattered once `.claude/settings.json` and `.opencode/opencode.json` became merge targets holding user-owned keys that setup cannot reconstruct. Both pre-deletes are dropped in favor of atomic rename. `sce setup --hooks` also replaced `pre-commit`, `commit-msg`, and `post-commit` wholesale, destroying any hook a repository already ran (husky, lefthook, hand-written). Each canonical hook payload is now delimited by an SCE managed block marker pair. Install creates the full script when no hook exists, replaces the block in place when one already carries it, recognizes and replaces a legacy pre-marker SCE payload wholesale, and otherwise appends the block after a foreign hook's content so SCE always runs last. A last-effective-line heuristic reports an advisory when a foreign hook's trailing zero-indent `exec`/`exit` would make the appended block unreachable. `sce doctor` now classifies hook content by managed-block currency instead of byte-exact comparison, so foreign content around a current block does not read as drift. Co-authored-by: SCE --- cli/assets/hooks/commit-msg | 6 +- cli/assets/hooks/post-commit | 14 +- cli/assets/hooks/pre-commit | 6 +- cli/src/services/doctor/inspect.rs | 189 ++++++- cli/src/services/hooks/lifecycle.rs | 10 +- cli/src/services/lifecycle.rs | 1 + cli/src/services/setup/command.rs | 1 + cli/src/services/setup/hook_merge.rs | 368 +++++++++++++ cli/src/services/setup/mod.rs | 483 +++++++++++++++--- context/architecture.md | 2 +- context/context-map.md | 7 +- ...8-07-git-hook-managed-block-cooperation.md | 124 +++++ context/glossary.md | 8 +- context/overview.md | 6 +- context/patterns.md | 6 +- .../git-hook-append-and-atomic-asset-swap.md | 201 ++++++++ context/sce/doctor-human-text-contract.md | 2 + .../setup-githooks-hook-asset-packaging.md | 4 +- .../sce/setup-githooks-install-contract.md | 22 +- context/sce/setup-githooks-install-flow.md | 26 +- context/sce/setup-no-backup-policy-seam.md | 14 +- 21 files changed, 1348 insertions(+), 152 deletions(-) create mode 100644 cli/src/services/setup/hook_merge.rs create mode 100644 context/decisions/2026-08-07-git-hook-managed-block-cooperation.md create mode 100644 context/plans/git-hook-append-and-atomic-asset-swap.md diff --git a/cli/assets/hooks/commit-msg b/cli/assets/hooks/commit-msg index 78af0644..13c85d5a 100644 --- a/cli/assets/hooks/commit-msg +++ b/cli/assets/hooks/commit-msg @@ -1,6 +1,7 @@ #!/bin/sh set -eu +# >>> sce managed block (do not edit) >>> if ! command -v sce >/dev/null 2>&1; then # sce brand colors — only emit ANSI when stderr is a real terminal if [ -t 2 ]; then @@ -25,4 +26,7 @@ if ! command -v sce >/dev/null 2>&1; then exit 0 fi -exec sce hooks commit-msg "$@" +sce hooks commit-msg "$@" +status=$? +exit "$status" +# <<< sce managed block <<< diff --git a/cli/assets/hooks/post-commit b/cli/assets/hooks/post-commit index a9822c09..7cb1579e 100644 --- a/cli/assets/hooks/post-commit +++ b/cli/assets/hooks/post-commit @@ -1,8 +1,7 @@ #!/bin/sh set -eu -remote_url="$(git remote get-url origin 2>/dev/null || true)" - +# >>> sce managed block (do not edit) >>> if ! command -v sce >/dev/null 2>&1; then # sce brand colors — only emit ANSI when stderr is a real terminal if [ -t 2 ]; then @@ -27,8 +26,13 @@ if ! command -v sce >/dev/null 2>&1; then exit 0 fi +remote_url="$(git remote get-url origin 2>/dev/null || true)" + if [ -n "$remote_url" ]; then - exec sce hooks post-commit --vcs git --remote-url "$remote_url" "$@" + sce hooks post-commit --vcs git --remote-url "$remote_url" "$@" +else + sce hooks post-commit --vcs git "$@" fi - -exec sce hooks post-commit --vcs git "$@" +status=$? +exit "$status" +# <<< sce managed block <<< diff --git a/cli/assets/hooks/pre-commit b/cli/assets/hooks/pre-commit index b4b93a78..e9890971 100644 --- a/cli/assets/hooks/pre-commit +++ b/cli/assets/hooks/pre-commit @@ -1,6 +1,7 @@ #!/bin/sh set -eu +# >>> sce managed block (do not edit) >>> if ! command -v sce >/dev/null 2>&1; then # sce brand colors — only emit ANSI when stderr is a real terminal if [ -t 2 ]; then @@ -25,4 +26,7 @@ if ! command -v sce >/dev/null 2>&1; then exit 0 fi -exec sce hooks pre-commit "$@" +sce hooks pre-commit "$@" +status=$? +exit "$status" +# <<< sce managed block <<< diff --git a/cli/src/services/doctor/inspect.rs b/cli/src/services/doctor/inspect.rs index a2860344..38b8212c 100644 --- a/cli/src/services/doctor/inspect.rs +++ b/cli/src/services/doctor/inspect.rs @@ -15,8 +15,9 @@ use crate::services::repository_identity::resolve::{ resolve_repository_identity, RepositoryIdentitySource, }; use crate::services::setup::{ - config_merge, iter_embedded_assets_for_setup_target_with_selection, iter_required_hook_assets, - persisted_optional_workflows, repair_merge_target_asset, EmbeddedAsset, SetupTarget, + config_merge, hook_merge, iter_embedded_assets_for_setup_target_with_selection, + iter_required_hook_assets, persisted_optional_workflows, repair_merge_target_asset, + EmbeddedAsset, SetupTarget, }; use super::types::{ @@ -318,17 +319,28 @@ fn inspect_hook_content_state_without_problem( }; match fs::read(hook_path) { - Ok(bytes) => { - if bytes == expected_hook.bytes { - HookContentState::Current - } else { - HookContentState::Stale - } - } + Ok(bytes) => hook_managed_block_content_state(hook_name, &bytes, expected_hook.bytes), Err(_) => HookContentState::Unknown, } } +/// Classifies a hook's on-disk bytes against the canonical template by SCE +/// managed-block currency (merging the canonical block into `bytes` is a +/// no-op) rather than whole-file equality, so foreign content a repository +/// has appended around the block does not read as drift. An unbalanced or +/// partial managed block is also reported `Stale`, since it needs the same +/// `--fix` repair as a drifted one. +fn hook_managed_block_content_state( + hook_name: &str, + bytes: &[u8], + canonical: &[u8], +) -> HookContentState { + match hook_merge::merge_or_create_hook(Some(bytes), canonical, hook_name) { + Ok(merge) if merge.bytes == bytes => HookContentState::Current, + Ok(_) | Err(_) => HookContentState::Stale, + } +} + #[allow(dead_code)] fn inspect_repository_hooks( repository_root: &Path, @@ -1568,13 +1580,7 @@ fn inspect_hook_content_state( }; match fs::read(hook_path) { - Ok(bytes) => { - if bytes == expected_hook.bytes { - HookContentState::Current - } else { - HookContentState::Stale - } - } + Ok(bytes) => hook_managed_block_content_state(hook_name, &bytes, expected_hook.bytes), Err(error) => { problems.push(DoctorProblem { kind: ProblemKind::HookReadFailed, @@ -1602,8 +1608,9 @@ mod tests { use std::path::PathBuf; use super::{ - collect_claude_integration_groups, collect_opencode_integration_groups, - collect_pi_integration_groups, inspect_claude_integration_health, IntegrationContentState, + collect_claude_integration_groups, collect_hook_file_health, + collect_opencode_integration_groups, collect_pi_integration_groups, + inspect_claude_integration_health, HookContentState, IntegrationContentState, IntegrationGroupHealth, }; use crate::services::setup::OPTIONAL_WORKFLOWS; @@ -1942,4 +1949,150 @@ mod tests { std::fs::remove_dir_all(&root).ok(); } + + fn canonical_pre_commit_bytes() -> &'static [u8] { + crate::services::setup::iter_required_hook_assets() + .find(|asset| asset.relative_path == "pre-commit") + .expect("embedded catalog carries pre-commit") + .bytes + } + + #[cfg(unix)] + fn mark_executable(path: &std::path::Path) { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)) + .expect("mark hook executable"); + } + + #[test] + fn hook_with_foreign_content_and_current_block_reports_current() { + let dir = unique_temp_repository_root("hook-foreign-current"); + let foreign_prefix = b"#!/bin/sh\necho husky-style-guard\n".to_vec(); + let merge = crate::services::setup::hook_merge::merge_or_create_hook( + Some(&foreign_prefix), + canonical_pre_commit_bytes(), + "pre-commit", + ) + .expect("merge over foreign hook should succeed"); + + let hook_path = dir.join("pre-commit"); + std::fs::write(&hook_path, &merge.bytes).expect("write foreign-plus-block hook"); + #[cfg(unix)] + mark_executable(&hook_path); + + let health = collect_hook_file_health(&dir); + let pre_commit = health + .iter() + .find(|hook| hook.name == "pre-commit") + .expect("pre-commit health present"); + assert_eq!(pre_commit.content_state, HookContentState::Current); + + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn hook_with_drifted_managed_block_reports_stale() { + let dir = unique_temp_repository_root("hook-drifted-stale"); + let canonical_text = + String::from_utf8(canonical_pre_commit_bytes().to_vec()).expect("hook is utf8"); + let drifted_text = canonical_text.replace( + "sce hooks pre-commit \"$@\"", + "sce hooks pre-commit \"$@\" # drifted", + ); + assert_ne!( + drifted_text, canonical_text, + "drift fixture should actually differ from canonical" + ); + + let hook_path = dir.join("pre-commit"); + std::fs::write(&hook_path, drifted_text.as_bytes()).expect("write drifted hook"); + #[cfg(unix)] + mark_executable(&hook_path); + + let health = collect_hook_file_health(&dir); + let pre_commit = health + .iter() + .find(|hook| hook.name == "pre-commit") + .expect("pre-commit health present"); + assert_eq!(pre_commit.content_state, HookContentState::Stale); + + std::fs::remove_dir_all(&dir).ok(); + } + + fn init_git_repo(label: &str) -> PathBuf { + let repo = unique_temp_repository_root(label); + let output = std::process::Command::new("git") + .args(["init", "-q"]) + .current_dir(&repo) + .output() + .expect("git init should spawn"); + assert!( + output.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + repo + } + + #[test] + fn fix_repairs_drifted_hook_content_while_preserving_foreign_content() { + let repo = init_git_repo("hook-fix-repair"); + + let initial_outcome = crate::services::setup::install_required_git_hooks(&repo) + .expect("initial hook install should succeed"); + let pre_commit_path = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == "pre-commit") + .expect("pre-commit hook installed") + .hook_path + .clone(); + let hooks_directory = pre_commit_path + .parent() + .expect("hook path has a parent directory") + .to_path_buf(); + + let foreign_prefix = b"#!/bin/sh\necho husky-style-guard\n".to_vec(); + let foreign_plus_block = crate::services::setup::hook_merge::merge_or_create_hook( + Some(&foreign_prefix), + canonical_pre_commit_bytes(), + "pre-commit", + ) + .expect("merge over foreign hook should succeed"); + let drifted_text = String::from_utf8(foreign_plus_block.bytes.clone()) + .expect("hook is utf8") + .replace( + "sce hooks pre-commit \"$@\"", + "sce hooks pre-commit \"$@\" # drifted", + ); + std::fs::write(&pre_commit_path, drifted_text.as_bytes()) + .expect("seed foreign-plus-drifted-block hook"); + #[cfg(unix)] + mark_executable(&pre_commit_path); + + let health_before = collect_hook_file_health(&hooks_directory); + let pre_commit_before = health_before + .iter() + .find(|hook| hook.name == "pre-commit") + .expect("pre-commit health present"); + assert_eq!(pre_commit_before.content_state, HookContentState::Stale); + + crate::services::setup::install_required_git_hooks(&repo) + .expect("'--fix' repair reuses the canonical setup hook installation"); + + let repaired_bytes = std::fs::read(&pre_commit_path).expect("read repaired hook"); + assert!( + repaired_bytes.starts_with(&foreign_prefix), + "foreign content should survive the repair" + ); + + let health_after = collect_hook_file_health(&hooks_directory); + let pre_commit_after = health_after + .iter() + .find(|hook| hook.name == "pre-commit") + .expect("pre-commit health present"); + assert_eq!(pre_commit_after.content_state, HookContentState::Current); + + std::fs::remove_dir_all(&repo).ok(); + } } diff --git a/cli/src/services/hooks/lifecycle.rs b/cli/src/services/hooks/lifecycle.rs index 8672fbf0..a8723ec5 100644 --- a/cli/src/services/hooks/lifecycle.rs +++ b/cli/src/services/hooks/lifecycle.rs @@ -11,7 +11,7 @@ use crate::services::lifecycle::{ RequiredHooksInstallOutcome, ServiceLifecycle, SetupOutcome, }; use crate::services::setup::{ - install_required_git_hooks, iter_required_hook_assets, + hook_merge, install_required_git_hooks, iter_required_hook_assets, RequiredHookInstallStatus as SetupRequiredHookInstallStatus, RequiredHooksInstallOutcome as SetupRequiredHooksInstallOutcome, }; @@ -298,10 +298,9 @@ fn inspect_hook_content_state( match fs::read(hook_path) { Ok(bytes) => { - if bytes == expected_hook.bytes { - HookContentState::Current - } else { - HookContentState::Stale + match hook_merge::merge_or_create_hook(Some(&bytes), expected_hook.bytes, hook_name) { + Ok(merge) if merge.bytes == bytes => HookContentState::Current, + Ok(_) | Err(_) => HookContentState::Stale, } } Err(error) => { @@ -375,6 +374,7 @@ fn required_hooks_outcome_from_setup( RequiredHookInstallStatus::Skipped } }, + unreachable_block_advisory: result.unreachable_block_advisory, }, ) .collect(), diff --git a/cli/src/services/lifecycle.rs b/cli/src/services/lifecycle.rs index df335874..bcb4ef0e 100644 --- a/cli/src/services/lifecycle.rs +++ b/cli/src/services/lifecycle.rs @@ -103,6 +103,7 @@ pub struct RequiredHookInstallResult { pub hook_name: String, pub hook_path: PathBuf, pub status: RequiredHookInstallStatus, + pub unreachable_block_advisory: bool, } #[derive(Clone, Debug, Eq, PartialEq)] diff --git a/cli/src/services/setup/command.rs b/cli/src/services/setup/command.rs index ee4fbf7b..3b848fe2 100644 --- a/cli/src/services/setup/command.rs +++ b/cli/src/services/setup/command.rs @@ -123,6 +123,7 @@ fn setup_required_hooks_outcome_from_lifecycle( RequiredHookInstallStatus::Updated => setup::RequiredHookInstallStatus::Updated, RequiredHookInstallStatus::Skipped => setup::RequiredHookInstallStatus::Skipped, }, + unreachable_block_advisory: result.unreachable_block_advisory, }) .collect(), } diff --git a/cli/src/services/setup/hook_merge.rs b/cli/src/services/setup/hook_merge.rs new file mode 100644 index 00000000..452ab5af --- /dev/null +++ b/cli/src/services/setup/hook_merge.rs @@ -0,0 +1,368 @@ +//! Pure byte-level merge for git hooks that a repository may already own and +//! extend with its own script (husky, lefthook, or a hand-written hook). +//! Mirrors `config_merge.rs`'s approach for the two JSON merge targets: no +//! filesystem access, a classification of what happened, and idempotence +//! across repeated merges. +//! +//! SCE's logic in a hook lives inside a stable marker pair, +//! `MANAGED_BLOCK_START` / `MANAGED_BLOCK_END`. A hook that already carries +//! the pair is owned within that block; a hook predating the markers is +//! recognized as SCE-owned wholesale by the presence of the canonical +//! guidance URL and replaced entirely; any other hook is foreign, and its +//! bytes are kept as an exact prefix with the canonical block appended after +//! them. + +use anyhow::{bail, Result}; + +/// Opening marker line of the SCE managed block, matched as an exact line +/// (`config/pkl/renderers/*-hooks.pkl` templates emit it verbatim). +pub const MANAGED_BLOCK_START: &str = "# >>> sce managed block (do not edit) >>>"; + +/// Closing marker line of the SCE managed block. +pub const MANAGED_BLOCK_END: &str = "# <<< sce managed block <<<"; + +/// Substring identifying a pre-marker SCE hook payload, from before the +/// managed block existed: the CLI installation guidance URL every canonical +/// template has always printed when `sce` is missing. +const LEGACY_GUIDANCE_URL: &str = "https://sce.crocoder.dev/docs/getting-started#install-cli"; + +/// What `merge_or_create_hook` did to produce its output bytes. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum HookMergeKind { + /// No hook existed; the canonical template was used verbatim. + Created, + /// A hook already carried a managed block, and the block's content + /// differed from the canonical block, or the hook was a legacy + /// pre-marker SCE payload replaced wholesale. + ManagedBlockReplaced, + /// A foreign hook without a managed block was kept, with the canonical + /// block appended after its content. + AppendedToForeign, + /// A hook already carried a managed block identical to the canonical + /// one; the input bytes are returned unchanged. + AlreadyCurrent, +} + +/// Result of merging a hook's existing bytes with the canonical template. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct HookMerge { + /// The bytes to install. + pub bytes: Vec, + /// What kind of merge produced `bytes`. + pub kind: HookMergeKind, + /// True when `kind` is `AppendedToForeign` and the foreign hook's last + /// effective line is a zero-indent `exec` or `exit`, so the appended + /// block would never run. + pub unreachable_block_advisory: bool, +} + +/// Computes the bytes to install for a git hook named `hook_name`, given its +/// current bytes (`existing`, `None` when no hook exists) and the canonical +/// template (`canonical`). Performs no filesystem access. +pub fn merge_or_create_hook( + existing: Option<&[u8]>, + canonical: &[u8], + hook_name: &str, +) -> Result { + let Some(existing) = existing else { + return Ok(HookMerge { + bytes: canonical.to_vec(), + kind: HookMergeKind::Created, + unreachable_block_advisory: false, + }); + }; + + let canonical_block = match locate_block(canonical) { + BlockLocation::Balanced(start, end) => &canonical[start..end], + _ => bail!( + "Canonical template for hook '{hook_name}' must contain exactly one complete SCE managed block" + ), + }; + + match locate_block(existing) { + BlockLocation::Balanced(start, end) => { + let existing_block = &existing[start..end]; + if existing_block == canonical_block { + Ok(HookMerge { + bytes: existing.to_vec(), + kind: HookMergeKind::AlreadyCurrent, + unreachable_block_advisory: false, + }) + } else { + let mut bytes = Vec::with_capacity(existing.len()); + bytes.extend_from_slice(&existing[..start]); + bytes.extend_from_slice(canonical_block); + bytes.extend_from_slice(&existing[end..]); + Ok(HookMerge { + bytes, + kind: HookMergeKind::ManagedBlockReplaced, + unreachable_block_advisory: false, + }) + } + } + BlockLocation::Unbalanced => { + bail!("Hook '{hook_name}' contains an unbalanced or partial SCE managed block marker") + } + BlockLocation::Absent => { + let existing_text = String::from_utf8_lossy(existing); + if existing_text.contains(LEGACY_GUIDANCE_URL) { + Ok(HookMerge { + bytes: canonical.to_vec(), + kind: HookMergeKind::ManagedBlockReplaced, + unreachable_block_advisory: false, + }) + } else { + let advisory = ends_with_unreachable_control_flow(&existing_text); + + let mut bytes = existing.to_vec(); + if !bytes.ends_with(b"\n") { + bytes.push(b'\n'); + } + bytes.push(b'\n'); + bytes.extend_from_slice(canonical_block); + + Ok(HookMerge { + bytes, + kind: HookMergeKind::AppendedToForeign, + unreachable_block_advisory: advisory, + }) + } + } + } +} + +/// Where the SCE managed block's marker lines were found in a byte buffer, +/// as byte offsets: `Balanced(start, end)` is the offset of the start +/// marker line's first byte and the offset just past the end marker line's +/// trailing newline (or end of buffer). +enum BlockLocation { + Absent, + Balanced(usize, usize), + Unbalanced, +} + +fn locate_block(bytes: &[u8]) -> BlockLocation { + let start = locate_marker_line(bytes, MANAGED_BLOCK_START); + let end = locate_marker_line(bytes, MANAGED_BLOCK_END); + match (start, end) { + (None, None) => BlockLocation::Absent, + (Some((start, _)), Some((_, end))) if start < end => BlockLocation::Balanced(start, end), + _ => BlockLocation::Unbalanced, + } +} + +/// Finds the line in `bytes` whose content, with a trailing `\r?\n` +/// stripped, is exactly `marker`. Returns `(line_start, line_end)` byte +/// offsets, where `line_end` includes the line's own trailing newline (or is +/// the buffer length for a final line with none). +fn locate_marker_line(bytes: &[u8], marker: &str) -> Option<(usize, usize)> { + let mut offset = 0; + for line in bytes.split_inclusive(|&byte| byte == b'\n') { + let content = line.strip_suffix(b"\n").unwrap_or(line); + let content = content.strip_suffix(b"\r").unwrap_or(content); + if content == marker.as_bytes() { + return Some((offset, offset + line.len())); + } + offset += line.len(); + } + None +} + +/// True when the last non-blank, non-comment line of `text` sits at zero +/// indentation and starts with `exec ` or `exit` — a narrow heuristic (no +/// shell parsing) for "a block appended after this line would not run". +/// Deliberately misses an early `exit` guarded by a conditional. +fn ends_with_unreachable_control_flow(text: &str) -> bool { + let Some(line) = text.lines().rev().find(|line| { + let trimmed = line.trim(); + !trimmed.is_empty() && !trimmed.starts_with('#') + }) else { + return false; + }; + + if line.starts_with(' ') || line.starts_with('\t') { + return false; + } + + line == "exit" || line.starts_with("exit ") || line == "exec" || line.starts_with("exec ") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn canonical_template() -> Vec { + format!( + "#!/bin/sh\nset -eu\n\n{MANAGED_BLOCK_START}\nsce hooks pre-commit \"$@\"\nstatus=$?\nexit \"$status\"\n{MANAGED_BLOCK_END}\n" + ) + .into_bytes() + } + + fn canonical_block_bytes() -> Vec { + format!("{MANAGED_BLOCK_START}\nsce hooks pre-commit \"$@\"\nstatus=$?\nexit \"$status\"\n{MANAGED_BLOCK_END}\n").into_bytes() + } + + #[test] + fn creates_from_absent() { + let canonical = canonical_template(); + let merge = merge_or_create_hook(None, &canonical, "pre-commit").unwrap(); + + assert_eq!(merge.bytes, canonical); + assert_eq!(merge.kind, HookMergeKind::Created); + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn replaces_block_in_place_preserving_surrounding_foreign_content() { + let canonical = canonical_template(); + let existing = format!( + "#!/bin/sh\n# husky-style guard\nrun-linter\n\n{MANAGED_BLOCK_START}\nsce hooks pre-commit \"$@\" # stale\n{MANAGED_BLOCK_END}\n\n# trailer\necho done\n" + ) + .into_bytes(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert_eq!(merge.kind, HookMergeKind::ManagedBlockReplaced); + let text = String::from_utf8(merge.bytes.clone()).unwrap(); + assert!(text.starts_with("#!/bin/sh\n# husky-style guard\nrun-linter\n\n")); + assert!(text.ends_with("\n\n# trailer\necho done\n")); + assert!(text.contains(&String::from_utf8(canonical_block_bytes()).unwrap())); + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn appends_to_foreign_hook_preserving_original_bytes_as_exact_prefix() { + let canonical = canonical_template(); + let existing = b"#!/bin/sh\necho foreign-hook\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert_eq!(merge.kind, HookMergeKind::AppendedToForeign); + assert!(merge.bytes.starts_with(&existing)); + let block = canonical_block_bytes(); + let block_text = String::from_utf8(block).unwrap(); + assert!(String::from_utf8(merge.bytes) + .unwrap() + .ends_with(&block_text)); + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn legacy_pre_marker_payload_is_replaced_wholesale() { + let canonical = canonical_template(); + let legacy = format!( + "#!/bin/sh\nset -eu\nif ! command -v sce >/dev/null 2>&1; then\n echo 'Install: {LEGACY_GUIDANCE_URL}'\n exit 0\nfi\nexec sce hooks pre-commit \"$@\"\n" + ) + .into_bytes(); + + let merge = merge_or_create_hook(Some(&legacy), &canonical, "pre-commit").unwrap(); + + assert_eq!(merge.kind, HookMergeKind::ManagedBlockReplaced); + assert_eq!(merge.bytes, canonical); + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn already_current_block_is_returned_unchanged() { + let canonical = canonical_template(); + let existing = canonical.clone(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert_eq!(merge.kind, HookMergeKind::AlreadyCurrent); + assert_eq!(merge.bytes, existing); + } + + #[test] + fn is_idempotent_across_two_merges_for_foreign_and_replace_shapes() { + let canonical = canonical_template(); + + let foreign = b"#!/bin/sh\necho foreign-hook\n".to_vec(); + let once = merge_or_create_hook(Some(&foreign), &canonical, "pre-commit").unwrap(); + let twice = merge_or_create_hook(Some(&once.bytes), &canonical, "pre-commit").unwrap(); + assert_eq!(once.bytes, twice.bytes); + assert_eq!(twice.kind, HookMergeKind::AlreadyCurrent); + + let legacy = + format!("#!/bin/sh\n{LEGACY_GUIDANCE_URL}\nexec sce hooks pre-commit \"$@\"\n") + .into_bytes(); + let once = merge_or_create_hook(Some(&legacy), &canonical, "pre-commit").unwrap(); + let twice = merge_or_create_hook(Some(&once.bytes), &canonical, "pre-commit").unwrap(); + assert_eq!(once.bytes, twice.bytes); + assert_eq!(twice.kind, HookMergeKind::AlreadyCurrent); + } + + #[test] + fn unbalanced_marker_fails_with_deterministic_error_naming_the_hook() { + let canonical = canonical_template(); + let existing = + format!("#!/bin/sh\n{MANAGED_BLOCK_START}\necho no-closing-marker\n").into_bytes(); + + let error = merge_or_create_hook(Some(&existing), &canonical, "commit-msg").unwrap_err(); + + assert!(error.to_string().contains("commit-msg")); + assert!(error.to_string().contains("unbalanced")); + } + + #[test] + fn partial_end_only_marker_fails_with_deterministic_error_naming_the_hook() { + let canonical = canonical_template(); + let existing = + format!("#!/bin/sh\necho no-opening-marker\n{MANAGED_BLOCK_END}\n").into_bytes(); + + let error = merge_or_create_hook(Some(&existing), &canonical, "post-commit").unwrap_err(); + + assert!(error.to_string().contains("post-commit")); + } + + #[test] + fn advisory_fires_on_trailing_zero_indent_exec() { + let canonical = canonical_template(); + let existing = b"#!/bin/sh\nexec some-tool \"$@\"\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert!(merge.unreachable_block_advisory); + } + + #[test] + fn advisory_fires_on_trailing_zero_indent_exit() { + let canonical = canonical_template(); + let existing = b"#!/bin/sh\nrun-linter\nexit 1\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert!(merge.unreachable_block_advisory); + } + + #[test] + fn advisory_does_not_fire_on_indented_exec() { + let canonical = canonical_template(); + let existing = + b"#!/bin/sh\nif [ -f .foo ]; then\n exec some-tool \"$@\"\nfi\necho done\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn advisory_does_not_fire_on_ordinary_final_command() { + let canonical = canonical_template(); + let existing = b"#!/bin/sh\necho done\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert!(!merge.unreachable_block_advisory); + } + + #[test] + fn advisory_ignores_trailing_comment_and_blank_lines() { + let canonical = canonical_template(); + let existing = b"#!/bin/sh\nexec some-tool \"$@\"\n\n# trailing comment\n".to_vec(); + + let merge = merge_or_create_hook(Some(&existing), &canonical, "pre-commit").unwrap(); + + assert!(merge.unreachable_block_advisory); + } +} diff --git a/cli/src/services/setup/mod.rs b/cli/src/services/setup/mod.rs index cfbef3ea..54520b02 100644 --- a/cli/src/services/setup/mod.rs +++ b/cli/src/services/setup/mod.rs @@ -10,6 +10,7 @@ use crate::services::{default_paths, default_paths::RepoPaths}; pub mod command; pub(crate) mod config_merge; +pub(crate) mod hook_merge; /// Canonical JSON payload for a newly bootstrapped repo-local `.sce/config.json`. /// Contains only the `$schema` declaration pointing to the SCE config JSON Schema. @@ -581,6 +582,14 @@ pub fn format_required_hook_install_success_message( value("at"), value(&format!("'{}'", result.hook_path.display())) )); + + if result.unreachable_block_advisory { + lines.push(format!( + " {} '{}' ends with 'exec'/'exit' before the SCE managed block, so the block will not run. Move it above that line.", + label("Advisory:"), + result.hook_name + )); + } } lines.join("\n") @@ -627,6 +636,9 @@ pub struct RequiredHookInstallResult { pub hook_name: String, pub hook_path: PathBuf, pub status: RequiredHookInstallStatus, + /// True when the hook's foreign content ends in a zero-indent `exec` or + /// `exit`, so the appended SCE managed block would never run. + pub unreachable_block_advisory: bool, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -814,6 +826,7 @@ mod install { use crate::services::security::{ensure_directory_is_writable, redact_sensitive_text}; use super::config_merge; + use super::hook_merge; use super::{ cleanup_path_if_exists, concrete_targets_for, embedded_assets_for_concrete_target, hook_install_recovery_guidance, iter_embedded_assets_for_setup_target_with_selection, @@ -836,10 +849,18 @@ mod install { pub(super) fn install_required_git_hooks( repository_root: &Path, ) -> Result { + install_required_git_hooks_with_rename(repository_root, |from, to| fs::rename(from, to)) + } + + pub(super) fn install_required_git_hooks_with_rename( + repository_root: &Path, + rename_fn: F, + ) -> Result + where + F: FnMut(&Path, &Path) -> io::Result<()>, + { let resolved_repository_root = prepare_setup_hooks_repository(repository_root)?; - install_required_git_hooks_in_resolved_repository(&resolved_repository_root, |from, to| { - fs::rename(from, to) - }) + install_required_git_hooks_in_resolved_repository(&resolved_repository_root, rename_fn) } pub(super) fn install_embedded_setup_assets( @@ -927,77 +948,79 @@ mod install { let hook_path = hooks_directory.join(hook_asset.relative_path); let existing_metadata = fs::metadata(&hook_path).ok(); - if existing_metadata + let existing_bytes = if existing_metadata .as_ref() .is_some_and(std::fs::Metadata::is_file) { - let existing_bytes = fs::read(&hook_path).with_context(|| { + Some(fs::read(&hook_path).with_context(|| { format!("Failed to read existing hook '{}'", hook_path.display()) - })?; - let executable = is_executable_file(&hook_path)?; + })?) + } else if existing_metadata.is_some() { + bail!( + "Existing hook target '{}' is not a file", + hook_path.display() + ); + } else { + None + }; + + let merge = hook_merge::merge_or_create_hook( + existing_bytes.as_deref(), + hook_asset.bytes, + hook_asset.relative_path, + )?; - if existing_bytes == hook_asset.bytes && executable { + if let Some(existing_bytes) = existing_bytes.as_deref() { + let executable = is_executable_file(&hook_path)?; + if merge.bytes == existing_bytes && executable { return Ok(RequiredHookInstallResult { hook_name: hook_asset.relative_path.to_string(), hook_path, status: RequiredHookInstallStatus::Skipped, + unreachable_block_advisory: merge.unreachable_block_advisory, }); } - } else if existing_metadata.is_some() { - bail!( - "Existing hook target '{}' is not a file", - hook_path.display() - ); } + let had_existing_hook = existing_metadata.is_some(); + let hook_staging_path = create_hook_staging_path(hooks_directory, hook_asset.relative_path)?; - if let Err(error) = write_hook_payload_to_staging(&hook_staging_path, hook_asset.bytes) { + if let Err(error) = write_hook_payload_to_staging(&hook_staging_path, &merge.bytes) { cleanup_path_if_exists(&hook_staging_path); return Err(error); } - if existing_metadata.is_none() { - if let Err(error) = rename_fn(&hook_staging_path, &hook_path).with_context(|| { - format!( - "Failed to install required hook '{}' at '{}'", - hook_asset.relative_path, - hook_path.display() - ) - }) { - cleanup_path_if_exists(&hook_staging_path); - return Err(error); - } - - return Ok(RequiredHookInstallResult { - hook_name: hook_asset.relative_path.to_string(), - hook_path, - status: RequiredHookInstallStatus::Installed, - }); - } - - remove_existing_install_target(&hook_path).with_context(|| { - format!( - "Failed to replace existing hook '{}' without creating a backup", - hook_path.display() - ) - })?; - + let action = if had_existing_hook { + "update" + } else { + "install" + }; if let Err(error) = rename_fn(&hook_staging_path, &hook_path).with_context(|| { format!( - "Failed to update required hook '{}' at '{}'", + "Failed to {action} required hook '{}' at '{}'", hook_asset.relative_path, hook_path.display() ) }) { cleanup_path_if_exists(&hook_staging_path); - return Err(error.context(hook_install_recovery_guidance(&hook_path))); + let error = if had_existing_hook { + error.context(hook_install_recovery_guidance(&hook_path)) + } else { + error + }; + return Err(error); } Ok(RequiredHookInstallResult { hook_name: hook_asset.relative_path.to_string(), hook_path, - status: RequiredHookInstallStatus::Updated, + status: if had_existing_hook { + RequiredHookInstallStatus::Updated + } else { + RequiredHookInstallStatus::Installed + }, + unreachable_block_advisory: merge.unreachable_block_advisory, }) } @@ -1405,18 +1428,6 @@ mod install { return Err(error); } - if destination.exists() { - if let Err(error) = fs::remove_file(&destination).with_context(|| { - format!( - "Failed to replace existing setup asset '{}' without creating a backup", - destination.display() - ) - }) { - cleanup_path_if_exists(&staging_path); - return Err(error); - } - } - if let Err(error) = rename_fn(&staging_path, &destination).with_context(|| { format!( "Failed to install staged asset '{}' into destination '{}'", @@ -1465,33 +1476,6 @@ mod install { ) } - fn remove_existing_install_target(destination_root: &Path) -> Result<()> { - let metadata = fs::metadata(destination_root).with_context(|| { - format!( - "Failed to inspect existing setup target '{}'", - destination_root.display() - ) - })?; - - if metadata.is_dir() { - fs::remove_dir_all(destination_root).with_context(|| { - format!( - "Failed to remove existing setup target directory '{}'", - destination_root.display() - ) - })?; - } else { - fs::remove_file(destination_root).with_context(|| { - format!( - "Failed to remove existing setup target file '{}'", - destination_root.display() - ) - })?; - } - - Ok(()) - } - fn validate_embedded_relative_path(relative_path: &str) -> Result<()> { let path = Path::new(relative_path); @@ -2359,6 +2343,10 @@ mod tests { let claude_dir = default_paths::InstallTargetPaths::new(&repo).claude_target_dir(); let failing_destination = claude_dir.join("commands/next-task.md"); + fs::create_dir_all(claude_dir.join("commands")).expect("create commands dir"); + let prior_content = b"prior next-task content\n"; + fs::write(&failing_destination, prior_content).expect("seed prior next-task content"); + let result = install::install_embedded_setup_assets_with_rename( &repo, SetupTarget::Claude, @@ -2383,6 +2371,12 @@ mod tests { "error should include recovery guidance: {message}" ); + assert_eq!( + fs::read(&failing_destination).expect("read failing destination after rename failure"), + prior_content, + "prior content at the failing destination should survive a rename failure" + ); + let commands_staging_dir = claude_dir.join("commands"); if commands_staging_dir.exists() { let leftover_staging_files = fs::read_dir(&commands_staging_dir) @@ -2402,4 +2396,329 @@ mod tests { let _ = fs::remove_dir_all(&repo); } + + #[test] + fn hook_install_leaves_prior_hook_intact_on_rename_failure() { + let repo = init_git_repo("hook-install-rename-failure"); + + let initial_outcome = install::install_required_git_hooks(&repo) + .expect("initial hook install should succeed"); + let pre_commit_result = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook should be installed"); + let pre_commit_path = pre_commit_result.hook_path.clone(); + + let prior_hook_bytes = b"#!/bin/sh\necho prior pre-commit\n".to_vec(); + fs::write(&pre_commit_path, &prior_hook_bytes).expect("seed prior pre-commit hook"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&pre_commit_path, fs::Permissions::from_mode(0o755)) + .expect("mark prior pre-commit hook executable"); + } + let prior_mode = fs::metadata(&pre_commit_path) + .expect("stat prior pre-commit hook") + .permissions(); + + let result = install::install_required_git_hooks_with_rename(&repo, |from, to| { + if to == pre_commit_path { + Err(std::io::Error::other("simulated rename failure")) + } else { + fs::rename(from, to) + } + }); + + let error = result.expect_err("rename failure should surface as an error"); + let message = format!("{error:#}"); + assert!( + message.contains(&pre_commit_path.display().to_string()), + "error should name the failing hook path: {message}" + ); + + assert_eq!( + fs::read(&pre_commit_path).expect("read pre-commit hook after rename failure"), + prior_hook_bytes, + "prior hook content should survive a rename failure" + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode_after = fs::metadata(&pre_commit_path) + .expect("stat pre-commit hook after rename failure") + .permissions(); + assert_eq!( + mode_after.mode() & 0o777, + prior_mode.mode() & 0o777, + "prior hook executable mode should survive a rename failure" + ); + } + + let hooks_staging_dir = pre_commit_path + .parent() + .expect("pre-commit hook should have a parent directory"); + let leftover_staging_files = fs::read_dir(hooks_staging_dir) + .expect("read hooks staging dir") + .filter_map(Result::ok) + .any(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with(".sce-hook-staging-") + }); + assert!( + !leftover_staging_files, + "staging artifact for the failed hook should be cleaned up" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn foreign_pre_commit_hook_keeps_its_content_and_gains_the_sce_block() { + let repo = init_git_repo("hook-install-foreign-append"); + + let initial_outcome = install::install_required_git_hooks(&repo) + .expect("initial hook install should succeed"); + let pre_commit_path = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook should be installed") + .hook_path + .clone(); + + let foreign_bytes = b"#!/bin/sh\necho husky-style-guard\n".to_vec(); + fs::write(&pre_commit_path, &foreign_bytes).expect("seed foreign pre-commit hook"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&pre_commit_path, fs::Permissions::from_mode(0o755)) + .expect("mark foreign pre-commit hook executable"); + } + + let outcome = install::install_required_git_hooks(&repo) + .expect("hook install over a foreign hook should succeed"); + let result = outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook result should be present"); + + assert_eq!(result.status, RequiredHookInstallStatus::Updated); + assert!(!result.unreachable_block_advisory); + + let installed_bytes = fs::read(&pre_commit_path).expect("read installed pre-commit hook"); + assert!( + installed_bytes.starts_with(&foreign_bytes), + "foreign hook content should survive as an exact prefix" + ); + let installed_text = String::from_utf8(installed_bytes).expect("hook should be utf8"); + assert!(installed_text.contains(hook_merge::MANAGED_BLOCK_START)); + assert!(installed_text.contains(hook_merge::MANAGED_BLOCK_END)); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(&pre_commit_path) + .expect("stat installed pre-commit hook") + .permissions() + .mode(); + assert_ne!(mode & 0o111, 0, "installed hook should remain executable"); + } + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn rerunning_hook_install_is_idempotent_for_block_only_and_foreign_plus_block_shapes() { + let repo = init_git_repo("hook-install-idempotent"); + + let first_outcome = + install::install_required_git_hooks(&repo).expect("first hook install should succeed"); + let pre_commit_result = first_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook should be installed"); + assert_eq!( + pre_commit_result.status, + RequiredHookInstallStatus::Installed + ); + + let second_outcome = + install::install_required_git_hooks(&repo).expect("second hook install should succeed"); + let second_pre_commit = second_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook result should be present"); + assert_eq!(second_pre_commit.status, RequiredHookInstallStatus::Skipped); + assert_eq!( + fs::read(&second_pre_commit.hook_path).expect("read block-only pre-commit hook"), + fs::read(&pre_commit_result.hook_path).expect("read initial pre-commit hook"), + "block-only hook bytes should be unchanged across reruns" + ); + + let commit_msg_result = first_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::COMMIT_MSG) + .expect("commit-msg hook should be installed"); + let commit_msg_path = commit_msg_result.hook_path.clone(); + let foreign_prefix = b"#!/bin/sh\necho foreign-commit-msg-guard\n".to_vec(); + fs::write(&commit_msg_path, &foreign_prefix).expect("seed foreign commit-msg hook"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&commit_msg_path, fs::Permissions::from_mode(0o755)) + .expect("mark foreign commit-msg hook executable"); + } + + let appended_outcome = install::install_required_git_hooks(&repo) + .expect("hook install appending to foreign commit-msg hook should succeed"); + let appended_result = appended_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::COMMIT_MSG) + .expect("commit-msg hook result should be present"); + assert_eq!(appended_result.status, RequiredHookInstallStatus::Updated); + let appended_bytes = fs::read(&commit_msg_path).expect("read appended commit-msg hook"); + + let rerun_outcome = install::install_required_git_hooks(&repo) + .expect("rerunning hook install over foreign-plus-block hook should succeed"); + let rerun_result = rerun_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::COMMIT_MSG) + .expect("commit-msg hook result should be present"); + assert_eq!(rerun_result.status, RequiredHookInstallStatus::Skipped); + assert_eq!( + fs::read(&commit_msg_path).expect("read commit-msg hook after rerun"), + appended_bytes, + "foreign-plus-block hook bytes should be unchanged across reruns" + ); + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn legacy_pre_marker_hook_upgrades_to_the_managed_block_form() { + let repo = init_git_repo("hook-install-legacy-upgrade"); + + let initial_outcome = install::install_required_git_hooks(&repo) + .expect("initial hook install should succeed"); + let pre_commit_path = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook should be installed") + .hook_path + .clone(); + let canonical_bytes = fs::read(&pre_commit_path).expect("read canonical pre-commit hook"); + + let legacy_bytes = b"#!/bin/sh\nset -eu\nif ! command -v sce >/dev/null 2>&1; then\n echo 'Install: https://sce.crocoder.dev/docs/getting-started#install-cli'\n exit 0\nfi\nexec sce hooks pre-commit \"$@\"\n".to_vec(); + fs::write(&pre_commit_path, &legacy_bytes).expect("seed legacy pre-commit hook"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&pre_commit_path, fs::Permissions::from_mode(0o755)) + .expect("mark legacy pre-commit hook executable"); + } + + let outcome = install::install_required_git_hooks(&repo) + .expect("hook install upgrading a legacy hook should succeed"); + let result = outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook result should be present"); + + assert_eq!(result.status, RequiredHookInstallStatus::Updated); + assert_eq!( + fs::read(&pre_commit_path).expect("read upgraded pre-commit hook"), + canonical_bytes, + "a legacy pre-marker hook should upgrade to the canonical marker form" + ); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(&pre_commit_path) + .expect("stat upgraded pre-commit hook") + .permissions() + .mode(); + assert_ne!(mode & 0o111, 0, "upgraded hook should remain executable"); + } + + let _ = fs::remove_dir_all(&repo); + } + + #[test] + fn foreign_hook_ending_in_exec_installs_the_block_and_reports_the_advisory() { + let repo = init_git_repo("hook-install-unreachable-advisory"); + + let initial_outcome = install::install_required_git_hooks(&repo) + .expect("initial hook install should succeed"); + let pre_commit_path = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook should be installed") + .hook_path + .clone(); + let commit_msg_path = initial_outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::COMMIT_MSG) + .expect("commit-msg hook should be installed") + .hook_path + .clone(); + + let unreachable_foreign = b"#!/bin/sh\nexec some-other-tool \"$@\"\n".to_vec(); + fs::write(&pre_commit_path, &unreachable_foreign).expect("seed unreachable foreign hook"); + let ordinary_foreign = b"#!/bin/sh\necho foreign-commit-msg-guard\n".to_vec(); + fs::write(&commit_msg_path, &ordinary_foreign).expect("seed ordinary foreign hook"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&pre_commit_path, fs::Permissions::from_mode(0o755)) + .expect("mark unreachable foreign hook executable"); + fs::set_permissions(&commit_msg_path, fs::Permissions::from_mode(0o755)) + .expect("mark ordinary foreign hook executable"); + } + + let outcome = install::install_required_git_hooks(&repo) + .expect("hook install over foreign hooks should succeed"); + + let pre_commit_result = outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::PRE_COMMIT) + .expect("pre-commit hook result should be present"); + assert_eq!(pre_commit_result.status, RequiredHookInstallStatus::Updated); + assert!( + pre_commit_result.unreachable_block_advisory, + "a hook ending in a zero-indent exec should report the advisory" + ); + assert!( + fs::read(&pre_commit_path) + .expect("read pre-commit hook") + .starts_with(&unreachable_foreign), + "the block should still be installed even though it is unreachable" + ); + + let commit_msg_result = outcome + .hook_results + .iter() + .find(|result| result.hook_name == default_paths::hook_dir::COMMIT_MSG) + .expect("commit-msg hook result should be present"); + assert!( + !commit_msg_result.unreachable_block_advisory, + "a hook ending in an ordinary command should not report the advisory" + ); + + let _ = fs::remove_dir_all(&repo); + } } diff --git a/context/architecture.md b/context/architecture.md index 57751b1c..2a940059 100644 --- a/context/architecture.md +++ b/context/architecture.md @@ -123,7 +123,7 @@ The repository includes a new placeholder Rust binary crate at `cli/`. - `cli/src/services/local_db/mod.rs` provides the concrete local DB spec and `LocalDb` type alias over the shared generic `TursoDb` adapter. `LocalDbSpec` resolves the deterministic persistent runtime DB target through the shared default-path seam and declares no local migrations; `TursoDb` supplies retry-backed blocking `execute`/`query`, parent-directory creation, Turso connection setup, tokio current-thread runtime bridging, and generic migration execution. - `cli/src/services/auth_db/mod.rs` provides the encrypted auth DB spec and `AuthDb` type alias over `EncryptedTursoDb`. `AuthDbSpec` resolves `/sce/auth.db` through the shared default-path seam and embeds ordered auth migrations. Auth DB lifecycle setup/doctor integration is wired through `AuthDbLifecycle`; auth command/token-storage reads/writes are directed through `token_storage.rs`. - `cli/src/services/agent_trace_db/mod.rs` owns the shared Agent Trace insert payloads, SQL constants, and typed row helpers (diff-trace/intersection/Agent Trace/message/part) plus `ensure_schema_ready_for_hooks()` consumed by the repository adapter. `cli/src/services/agent_trace_db/repository.rs` defines the sole `RepositoryAgentTraceDb` adapter over `TursoDb` with one fresh `agent-trace-repository/001_repository_schema.sql` baseline for `diff_traces` (including `payload_type`), `post_commit_patch_intersections`, `agent_traces`, `messages`, `parts`, indexes, and triggers, `repository_metadata` validation, no trace-table `checkout_id` columns, `agent_traces.agent_trace_id NOT NULL UNIQUE`, and `recent_diff_trace_patches(cutoff_time_ms, end_time_ms)` using the inclusive chronological parser without checkout filtering. Active hook runtime, setup/lifecycle storage, and trace status/list/shell flows resolve through `agent_trace_storage` and use `RepositoryAgentTraceDb`. The checkout-scoped `AgentTraceDb`/`AgentTraceDbSpec` adapter, its `agent_trace_db_path()`/`agent_trace_db_path_for_checkout()` helpers, the 15-file `cli/migrations/agent-trace/` chain, and the `sce trace --legacy` surface were removed by the `retire-legacy-agent-trace-db` plan. -- `cli/src/services/setup/mod.rs` defines the setup command contract (`SetupMode`, `SetupTarget`, `SetupRequest`, CLI flag parser/validator), an `inquire`-backed interactive target prompter (`InquireSetupTargetPrompter`), setup dispatch outcomes (proceed/cancelled), additive durable-context bootstrap (`bootstrap_context_baseline` for standalone `--bootstrap-context` and every normal successful setup path), and compile-time embedded asset access (`EmbeddedAsset`, target-scoped iterators, required-hook asset iterators/lookups). It also owns the install-time optional-workflow seam: the `OptionalWorkflow` type plus the build-generated `OPTIONAL_WORKFLOWS` catalog, a per-target `WorkflowAssetLayout` built from the existing `default_paths` command/skill directory constants (`command`/`commands`/`prompts` plus `skills`), and `iter_embedded_assets_for_setup_target_with_selection(target, selection)`, which yields every embedded asset except the `{command_dir}/{command_slug}.md` file and `{skills_dir}/{skill_slug}/` subtree of each optional workflow the selection omits. Membership is derived from the catalog's slugs rather than an enumerated file list, so a new optional workflow needs no Rust change. This filtered iterator is the only way embedded assets are enumerated; setup installs through it and doctor inspects through it, so there is no unfiltered enumeration path that could reintroduce an unselected workflow. The non-interactive selection flows through the repeatable `--workflow ` flag into `SetupRequest.optional_workflows: Option>` (`None` meaning the flag was absent); `validate_optional_workflow_slugs` checks each slug against `OPTIONAL_WORKFLOWS` during request resolution, before any write, and `run_setup_for_mode` resolves `None` to the persisted `integrations.optional_workflows` (exported as `persisted_optional_workflows`) before installing through the filtered iterator and persisting the resolved selection. The interactive selection flows through the same seam: `SetupTargetPrompter` carries `prompt_target` plus `prompt_optional_workflows(defaults)` (returning `None` for a cancelled prompt), `SetupDispatch::Proceed { mode, optional_workflows }` carries a prompted selection alongside the resolved mode, and `resolve_setup_dispatch(mode, prompter, defaults)` runs the workflow prompt only after an interactive target prompt, mapping either cancellation to `SetupDispatch::Cancelled`. The prompt module builds its `inquire::MultiSelect` from `optional_workflow_prompt_inputs(catalog, defaults)`, which returns `None` for an empty catalog (skipping the prompt) and otherwise catalog-ordered rows plus the indices to pre-check, ignoring ids absent from the catalog. `setup/command.rs` therefore resolves the repository root before dispatch, so a non-git directory fails before any prompt. For repository builds, `cli/build.rs` validates the `SCE_CLI_GENERATED_INPUT_DIR` payload and canonical-input inventories, copies the payload into Cargo `OUT_DIR/pkl-generated`, stages `cli/assets/hooks/**` under `OUT_DIR/static`, requires the staged `config/optional-workflows.json`, and generates both the setup manifest and the optional-workflow catalog (`optional_workflows.rs`, rejecting a manifest whose `schemaVersion` is not 1 or whose entries lack a non-empty `id`/`title`/`description`/`commandSlug`/`skillSlug`) in `OUT_DIR`; focused internal seams separate install-flow from prompt-flow logic; `cli/src/services/setup/command.rs` owns the `SetupCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Its install engine/orchestrator (`install_assets_for_concrete_target_with_rename`) installs each embedded asset individually: it stages the asset's content next to its final destination, removes only that exact destination file if one already exists, and swaps the staged content into place, with deterministic recovery guidance naming the failing asset's path on swap failure and no backup artifact creation; it never removes an integration target directory as a whole, so files a repository owns inside `.opencode`/`.claude`/`.pi` — including nested inside an SCE-owned subdirectory such as `skills/` or `commands/` — survive a setup run. For the two assets that are merge targets — the Claude target's `settings.json`, detected by `is_claude_settings_merge_target`, and the OpenCode target's `opencode.json`, detected by `is_opencode_config_merge_target` — the content staged is not the embedded asset's bytes but the result of `cli/src/services/setup/config_merge.rs::merge_or_create_claude_settings`/`merge_or_create_opencode_config(existing_bytes, generated_bytes, source_path)`: each returns the generated document verbatim when no file exists yet, otherwise parses the existing file as JSON (a parse failure is a hard error naming `source_path`, and nothing is written) and merges it with a pure per-shape function that copies `$schema` from the generated document and preserves every other key from the existing file untouched. `merge_claude_settings` replaces, per hook event key the generated document declares, only the entries whose command contains the ownership marker `run-sce-or-show-install-guidance.sh`, preserving every event key and non-SCE hook entry from the existing file. `merge_opencode_config` merges the `plugin` array as a set: existing entries whose path starts with the ownership marker `./plugins/sce-` are dropped structurally — so a stale plugin path an older or renamed catalog once installed is still recognized and pruned even after the current generated document stops declaring it — and the generated document's `plugin` entries are appended after the surviving entries. After that per-asset install loop, `prune_stale_assets_for_concrete_target` deletes every path the full embedded catalog for the concrete target claims but the resolved selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), and `remove_empty_ancestor_directories` removes any parent directory left empty by that deletion, stopping at the target root or at a directory that still holds something such as a user file. It formats deterministic completion messaging; required-hook install orchestration (`install_required_git_hooks`) follows the same per-file stage/swap choreography (removing an existing hook file before swapping staged content, with deterministic recovery guidance on swap failure). After the Git gate, setup always ensures the context baseline; context-only requests return there, while normal modes derive a repo-root-scoped context before aggregating static lifecycle provider `setup` dispatch across providers (config → local_db → auth_db → agent_trace_db → hooks when requested), so setup providers consume only repo-root access from the scoped context. +- `cli/src/services/setup/mod.rs` defines the setup command contract (`SetupMode`, `SetupTarget`, `SetupRequest`, CLI flag parser/validator), an `inquire`-backed interactive target prompter (`InquireSetupTargetPrompter`), setup dispatch outcomes (proceed/cancelled), additive durable-context bootstrap (`bootstrap_context_baseline` for standalone `--bootstrap-context` and every normal successful setup path), and compile-time embedded asset access (`EmbeddedAsset`, target-scoped iterators, required-hook asset iterators/lookups). It also owns the install-time optional-workflow seam: the `OptionalWorkflow` type plus the build-generated `OPTIONAL_WORKFLOWS` catalog, a per-target `WorkflowAssetLayout` built from the existing `default_paths` command/skill directory constants (`command`/`commands`/`prompts` plus `skills`), and `iter_embedded_assets_for_setup_target_with_selection(target, selection)`, which yields every embedded asset except the `{command_dir}/{command_slug}.md` file and `{skills_dir}/{skill_slug}/` subtree of each optional workflow the selection omits. Membership is derived from the catalog's slugs rather than an enumerated file list, so a new optional workflow needs no Rust change. This filtered iterator is the only way embedded assets are enumerated; setup installs through it and doctor inspects through it, so there is no unfiltered enumeration path that could reintroduce an unselected workflow. The non-interactive selection flows through the repeatable `--workflow ` flag into `SetupRequest.optional_workflows: Option>` (`None` meaning the flag was absent); `validate_optional_workflow_slugs` checks each slug against `OPTIONAL_WORKFLOWS` during request resolution, before any write, and `run_setup_for_mode` resolves `None` to the persisted `integrations.optional_workflows` (exported as `persisted_optional_workflows`) before installing through the filtered iterator and persisting the resolved selection. The interactive selection flows through the same seam: `SetupTargetPrompter` carries `prompt_target` plus `prompt_optional_workflows(defaults)` (returning `None` for a cancelled prompt), `SetupDispatch::Proceed { mode, optional_workflows }` carries a prompted selection alongside the resolved mode, and `resolve_setup_dispatch(mode, prompter, defaults)` runs the workflow prompt only after an interactive target prompt, mapping either cancellation to `SetupDispatch::Cancelled`. The prompt module builds its `inquire::MultiSelect` from `optional_workflow_prompt_inputs(catalog, defaults)`, which returns `None` for an empty catalog (skipping the prompt) and otherwise catalog-ordered rows plus the indices to pre-check, ignoring ids absent from the catalog. `setup/command.rs` therefore resolves the repository root before dispatch, so a non-git directory fails before any prompt. For repository builds, `cli/build.rs` validates the `SCE_CLI_GENERATED_INPUT_DIR` payload and canonical-input inventories, copies the payload into Cargo `OUT_DIR/pkl-generated`, stages `cli/assets/hooks/**` under `OUT_DIR/static`, requires the staged `config/optional-workflows.json`, and generates both the setup manifest and the optional-workflow catalog (`optional_workflows.rs`, rejecting a manifest whose `schemaVersion` is not 1 or whose entries lack a non-empty `id`/`title`/`description`/`commandSlug`/`skillSlug`) in `OUT_DIR`; focused internal seams separate install-flow from prompt-flow logic; `cli/src/services/setup/command.rs` owns the `SetupCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Its install engine/orchestrator (`install_assets_for_concrete_target_with_rename`) installs each embedded asset individually: it stages the asset's content next to its final destination and swaps it into place by renaming the staging file directly over the destination — never unlinking the destination first, since `fs::rename` already replaces an existing file atomically — with deterministic recovery guidance naming the failing asset's path on swap failure (the pre-existing destination content, if any, is untouched) and no backup artifact creation; it never removes an integration target directory as a whole, so files a repository owns inside `.opencode`/`.claude`/`.pi` — including nested inside an SCE-owned subdirectory such as `skills/` or `commands/` — survive a setup run. For the two assets that are merge targets — the Claude target's `settings.json`, detected by `is_claude_settings_merge_target`, and the OpenCode target's `opencode.json`, detected by `is_opencode_config_merge_target` — the content staged is not the embedded asset's bytes but the result of `cli/src/services/setup/config_merge.rs::merge_or_create_claude_settings`/`merge_or_create_opencode_config(existing_bytes, generated_bytes, source_path)`: each returns the generated document verbatim when no file exists yet, otherwise parses the existing file as JSON (a parse failure is a hard error naming `source_path`, and nothing is written) and merges it with a pure per-shape function that copies `$schema` from the generated document and preserves every other key from the existing file untouched. `merge_claude_settings` replaces, per hook event key the generated document declares, only the entries whose command contains the ownership marker `run-sce-or-show-install-guidance.sh`, preserving every event key and non-SCE hook entry from the existing file. `merge_opencode_config` merges the `plugin` array as a set: existing entries whose path starts with the ownership marker `./plugins/sce-` are dropped structurally — so a stale plugin path an older or renamed catalog once installed is still recognized and pruned even after the current generated document stops declaring it — and the generated document's `plugin` entries are appended after the surviving entries. After that per-asset install loop, `prune_stale_assets_for_concrete_target` deletes every path the full embedded catalog for the concrete target claims but the resolved selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), and `remove_empty_ancestor_directories` removes any parent directory left empty by that deletion, stopping at the target root or at a directory that still holds something such as a user file. It formats deterministic completion messaging; required-hook install orchestration (`install_required_git_hooks`, backed by the rename-injectable `install_required_git_hooks_with_rename`) is a third content-computation seam alongside the two JSON merge targets: `install_single_required_hook_with_rename` computes the bytes to stage with `cli/src/services/setup/hook_merge.rs::merge_or_create_hook(existing_bytes, canonical_bytes, hook_name)` rather than writing `hook_asset.bytes` verbatim — a foreign hook (no SCE managed block, no legacy guidance-URL marker) is kept as an exact byte prefix with the canonical block appended after it, an SCE-owned hook has only its block spliced in place or left unchanged, and a legacy pre-marker hook is replaced wholesale — then follows the same per-file stage/atomic-swap choreography as config-asset install: the staging file is renamed directly over the existing hook without unlinking it first, so a rename failure leaves the prior hook's bytes and executable bit intact, with deterministic recovery guidance on swap failure. `Installed`/`Updated`/`Skipped` are decided against the merged bytes plus the executable bit rather than the canonical asset's raw bytes, so an already-current foreign-plus-block hook reports `Skipped`; `RequiredHookInstallResult.unreachable_block_advisory` is set, and rendered as a named advisory line in setup's hook output, when an appended block follows a foreign hook's zero-indent `exec`/`exit` and so would never run. After the Git gate, setup always ensures the context baseline; context-only requests return there, while normal modes derive a repo-root-scoped context before aggregating static lifecycle provider `setup` dispatch across providers (config → local_db → auth_db → agent_trace_db → hooks when requested), so setup providers consume only repo-root access from the scoped context. - `cli/src/services/setup/mod.rs` keeps those responsibilities inside one file for now, but the current ownership split is explicit: the inline `install` module owns repository-path normalization, staging/swap install behavior, required-hook installation, and filesystem safety guards, while the inline `prompt` module owns interactive target selection and prompt styling. - `cli/src/services/security.rs` provides shared security utilities for deterministic secret redaction (`redact_sensitive_text`) and directory write-permission probes (`ensure_directory_is_writable`) used by app/setup/observability surfaces. - `cli/src/services/doctor/mod.rs` owns the current doctor request/report surface while focused submodules (`doctor/inspect.rs`, `doctor/render.rs`, `doctor/fixes.rs`, `doctor/types.rs`) split report fact collection, rendering, manual fix reporting, and doctor-owned domain types into smaller seams; `cli/src/services/doctor/command.rs` owns the `DoctorCommand` payload used by the static `RuntimeCommand` enum and executes against any context implementing repo-root scoping. Runtime doctor execution resolves a repository root, derives a scoped context, requests the shared static lifecycle provider catalog with hooks included for service-owned `diagnose` and `fix` behavior, adapts lifecycle-owned health/fix records into doctor-owned problem/fix records, and then renders stable text/JSON problem records with category/severity/fixability/remediation fields plus deterministic fix-result reporting in fix mode. Checkout DB discovery no longer lives in `doctor`; it moved to the `trace` group (`sce trace db list`) in `cli/src/services/trace/`. Report fact collection preserves environment/repository/hook/integration display data and checkout identity facts, while service-owned lifecycle providers own config validation, local DB and repository-scoped Agent Trace DB readiness/bootstrap, and hook rollout diagnosis/repair. Integration inspection in `doctor/inspect.rs` is scoped twice over: `resolve_doctor_integration_targets` picks which targets to inspect, and `persisted_optional_workflows` (reused from setup) resolves which optional workflows the repository selected, which the OpenCode/Claude/Pi child collectors apply through `iter_embedded_assets_for_setup_target_with_selection`. An unselected optional workflow therefore contributes no expected children at all, so no row and no missing/mismatch problem can be produced for it, while a selected one keeps the unchanged presence and content-hash checks. diff --git a/context/context-map.md b/context/context-map.md index 793f9139..cf623b72 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -49,10 +49,10 @@ Feature/domain context: - `context/sce/agent-trace-post-commit-dual-write.md` (historical post-commit no-op/dual-write reference; current post-commit behavior is documented in `agent-trace-hooks-command-routing.md`) - `context/sce/agent-trace-hook-doctor.md` (approved operator-environment contract for broadening `sce doctor` into the canonical health-and-repair entrypoint, including stable problem taxonomy, `--fix` semantics, checkout-aware Agent Trace DB reporting, setup-to-doctor alignment rules, canonical Git-hook payload restoration, and the approved downstream human text-mode layout/status/integration contract) - `context/sce/doctor-human-text-contract.md` (implemented `sce doctor` human text layout contract: section order, `[PASS]`/`[FAIL]`/`[MISS]` status vocabulary, simplified hook rows, target-scoped integration checks with configured/detected/empty target resolution, selection-scoped optional-workflow inventory read from `integrations.optional_workflows`, no-installed-integrations guidance, and OpenCode, Claude, plus Pi integration group rendering rules including the `Pi extensions` group) -- `context/sce/setup-githooks-install-contract.md` (canonical `sce setup --hooks` install contract for target-path resolution, all-hook non-blocking missing-CLI bootstrap behavior, idempotent outcomes, remove-and-replace behavior, and doctor-readiness alignment) -- `context/sce/setup-no-backup-policy-seam.md` (non-destructive per-asset install policy: config install writes/swaps each embedded asset individually and never removes an integration target directory as a whole, then prunes catalog-derived stale/deselected asset paths and any parent directory left empty by that pruning; required-hook install keeps the prior per-file remove-and-replace choreography; `.claude/settings.json` and `.opencode/opencode.json` are merge targets whose staged content is computed by JSON-merging the generated document into the user's existing one before the shared stage/swap step; no backup creation; deterministic recovery guidance naming the failing asset on swap failure) +- `context/sce/setup-githooks-install-contract.md` (canonical `sce setup --hooks` install contract for target-path resolution, all-hook non-blocking missing-CLI bootstrap behavior, foreign-hook preservation and managed-block merge/idempotent outcomes, atomic-swap replacement behavior, and doctor-readiness alignment) +- `context/sce/setup-no-backup-policy-seam.md` (non-destructive per-asset install policy: config install writes/swaps each embedded asset individually by atomic rename over the destination, without ever unlinking it first, and never removes an integration target directory as a whole, then prunes catalog-derived stale/deselected asset paths and any parent directory left empty by that pruning; required-hook install uses the same per-file stage/atomic-swap choreography and, like the two JSON merge targets, computes its staged content ahead of the swap — a foreign hook's bytes are kept as an exact prefix with the SCE managed block appended; `.claude/settings.json` and `.opencode/opencode.json` are merge targets whose staged content is computed by JSON-merging the generated document into the user's existing one before the shared stage/swap step; no backup creation; a swap failure leaves prior destination content untouched, with deterministic recovery guidance naming the failing asset) - `context/sce/setup-githooks-hook-asset-packaging.md` (compile-time `sce setup --hooks` required-hook template packaging contract, including all-hook non-blocking missing-`sce` install guidance, available-CLI argument forwarding, post-commit-only origin remote lookup plus remote-URL forwarding/fallback behavior, setup-service accessor surface, and current validation posture) -- `context/sce/setup-githooks-install-flow.md` (setup-service required-hook install orchestration with git-truth hooks-path resolution, canonical missing-CLI payload installation, per-hook installed/updated/skipped outcomes, and remove-and-replace behavior with recovery guidance) +- `context/sce/setup-githooks-install-flow.md` (setup-service required-hook install orchestration with git-truth hooks-path resolution, managed-block merge content computation that preserves foreign hook content, per-hook installed/updated/skipped outcomes decided against the merged content, the unreachable-block advisory, and atomic-swap replacement with recovery guidance) - `context/sce/setup-githooks-cli-ux.md` (T04 composable `sce setup` target+`--hooks` / `--repo` command-surface contract, option compatibility validation, and deterministic setup/hook output semantics) - `context/sce/setup-repo-local-config-bootstrap.md` (setup local bootstrap behavior: additive durable-context baseline via `sce setup --bootstrap-context` and every normal setup path, repo-local `.sce/config.json` create-if-missing via config lifecycle, additive `integrations.target` persistence after successful target installs, `integrations.optional_workflows` selection persistence with prompt-over-flag-over-persisted precedence, plus lifecycle-owned local DB initialization before hooks/config asset dispatch) - `context/sce/cli-security-hardening-contract.md` (T06 CLI redaction contract, setup `--repo` canonicalization/validation, and setup write-permission probe behavior) @@ -108,3 +108,4 @@ Recent decision records: - `context/decisions/2026-03-09-migrate-lexopt-to-clap.md` (CLI argument parsing migration from lexopt to clap derive macros) - `context/decisions/2026-03-25-first-install-channels.md` (approved first-wave install/distribution scope for `sce`, canonical naming, and Nix-owned build policy) - `context/decisions/2026-07-17-retire-legacy-agent-trace-db.md` (retire the checkout-scoped Agent Trace DB surface; `RepositoryAgentTraceDb` is the sole adapter, no `sce trace --legacy`, no global/checkout fallback path; pre-migration on-disk files are never touched and no longer inspectable via the CLI) +- `context/decisions/2026-08-07-git-hook-managed-block-cooperation.md` (SCE-installed git hooks are a bounded in-place editor, not an exclusive owner: hook ownership is decided structurally by the SCE managed-block marker pair or a legacy guidance-URL marker, a foreign hook's bytes are preserved as an exact prefix with the block appended after them, and coexistence with third-party hook managers is cooperative, not authoritative) diff --git a/context/decisions/2026-08-07-git-hook-managed-block-cooperation.md b/context/decisions/2026-08-07-git-hook-managed-block-cooperation.md new file mode 100644 index 00000000..fbfa722c --- /dev/null +++ b/context/decisions/2026-08-07-git-hook-managed-block-cooperation.md @@ -0,0 +1,124 @@ +# Decision: SCE git hooks are a bounded in-place editor, not an exclusive owner + +Date: 2026-08-07 +Status: Accepted +Plan: `context/plans/git-hook-append-and-atomic-asset-swap.md` +Task: T02, T03, T04 + +## Context + +`sce setup --hooks` installs `pre-commit`, `commit-msg`, and `post-commit`. The +predecessor plan (`non-destructive-setup-install`) replaced these files +wholesale, which is safe only as long as SCE is the sole author of every hook it +touches. That assumption breaks the moment a repository already runs husky, +lefthook, or a hand-written hook: a wholesale install silently destroys that +script. The predecessor plan named this out of scope and left the resolution +undecided (see its still-open question, carried into this plan's Open +questions). This plan had to decide how SCE coexists with a hook it does not +own, and the choice determines whether `sce setup --hooks` is safe to run in an +arbitrary repository. + +## Decision + +SCE treats an existing hook file as either SCE-owned or foreign, never as +something to overwrite by default. Each canonical hook payload lives inside a +delimited SCE managed block (`# >>> sce managed block (do not edit) >>>` / +`# <<< sce managed block <<<`). Ownership is decided structurally, not by +trusting the whole file: + +- A hook already carrying a balanced marker pair is SCE-owned within that + block; only the block is replaced or refreshed, and any surrounding content + is left untouched. +- A marker-free hook containing the legacy pre-marker guidance URL (from before + the marker pair existed) is recognized as SCE-owned wholesale and replaced + entirely. +- Any other hook is foreign. Its bytes are preserved as an exact byte prefix, + and the canonical SCE block is appended after them, so the foreign hook keeps + its shebang, content, and first-run position; SCE always runs last. + +This computation is pure (`cli/src/services/setup/hook_merge.rs::merge_or_create_hook`) +and is wired into install (`cli/src/services/setup/mod.rs`, +`install_single_required_hook_with_rename`), so `Installed`/`Updated`/`Skipped` +are now decided against the merged bytes plus the executable bit rather than +the canonical asset's raw bytes. + +## Rationale + +Appending after existing content, rather than requiring a dispatcher directory +or hook-manager detection, needs no cooperation protocol and works uniformly +whether the existing hook is husky-managed, lefthook-managed, or hand-written. +Running SCE last is not an arbitrary ordering choice: `commit-msg` trailer +insertion must see the final message, so SCE has to observe whatever the +foreign hook already did to it. Structural ownership detection (the marker +pair, then the legacy marker) mirrors the precedent already set for the two +JSON merge targets (`config_merge.rs`'s `./plugins/sce-` and +`run-sce-or-show-install-guidance.sh` ownership markers), so the same mental +model — "match markers, not full-file identity" — now applies to every merge +target SCE writes into. + +## Alternatives considered + +- **Wholesale replacement (status quo)** — simplest, but destroys any + co-installed hook manager's script; ruled out as the defect this plan exists + to fix. +- **`.d/` dispatcher directory** — the predecessor plan's proposal. + Requires every hook manager to cooperate with a dispatch convention SCE + invents, and a manager that reinstalls its own hook still drops SCE from the + chain just as easily as today's approach. Rejected as added complexity with + no corresponding robustness gain. +- **Hook-manager detection and cooperation protocol** — explicitly out of + scope for this plan; open-ended and highly manager-specific. + +## Compatibility and risks + +- **Cooperative, not authoritative**: a hook manager (husky, lefthook) that + rewrites its own hooks on `npm install` / `lefthook install` will drop the + SCE block silently, and SCE stops running until the next `sce setup --hooks`. + Nothing currently alerts the user in-band; `sce doctor` catching this drift + is a stated goal of a later task (T05) in the same plan, not this decision. +- **Unreachable appended block**: appending after a foreign hook whose last + effective line is a zero-indent `exec`/`exit` produces a block that never + runs. Mitigated by a narrow last-effective-line heuristic that surfaces a + named advisory rather than silently installing a dead block; the heuristic + deliberately does not parse shell, so it can miss an early conditional + `exit`. +- **Argument propagation**: the appended block relies on the invoking hook's + top-level `"$@"`; a foreign hook that `shift`s its arguments before the block + changes what SCE receives. Accepted rather than defended against. + +## Guardrails + +- Ownership detection is structural only (exact marker-line match, or the + fixed legacy guidance-URL substring) — never a heuristic guess at whether a + file "looks like" an SCE hook. +- The merge computation stays pure and filesystem-free + (`cli/src/services/setup/hook_merge.rs`); all I/O and swap choreography stay + in the install seam. +- No new hook-manager-specific protocol, detection, or dispatcher directory is + introduced by this decision. + +## Consequences + +- `sce setup --hooks` is now safe to run in a repository that already has + husky, lefthook, or a hand-written hook installed: that hook survives, and + SCE's logic runs alongside it. +- The existing outcome vocabulary (`Installed`/`Updated`/`Skipped`) and the + no-backup atomic-swap policy are preserved unchanged; only what counts as + "current" changed, from whole-file byte identity to managed-block currency. +- `sce doctor`'s hook inspection has not yet moved to the same block-currency + model as of this decision (T04); until the follow-up task lands, doctor may + report drift on a hook this decision considers current. + +## Follow-up + +- T05 in the same plan (`Inspect hook content by SCE managed block currency`) + moves `sce doctor` from whole-file byte comparison to the same block-currency + predicate this decision establishes for install, so a legitimately extended + hook reports `[PASS]` instead of drift. + +## References + +- Plan: [`git-hook-append-and-atomic-asset-swap.md`](../plans/git-hook-append-and-atomic-asset-swap.md) +- Task: T02, T03, T04 +- Current-state context: [`setup-githooks-install-contract.md`](../sce/setup-githooks-install-contract.md), [`setup-githooks-install-flow.md`](../sce/setup-githooks-install-flow.md), [`setup-no-backup-policy-seam.md`](../sce/setup-no-backup-policy-seam.md), [`setup-githooks-hook-asset-packaging.md`](../sce/setup-githooks-hook-asset-packaging.md) +- Evidence: `cli/src/services/setup/hook_merge.rs` unit tests; `cli/src/services/setup/mod.rs` integration tests `foreign_pre_commit_hook_keeps_its_content_and_gains_the_sce_block`, `rerunning_hook_install_is_idempotent_for_block_only_and_foreign_plus_block_shapes`, `legacy_pre_marker_hook_upgrades_to_the_managed_block_form`, `foreign_hook_ending_in_exec_installs_the_block_and_reports_the_advisory` (`./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 55 passed, 0 failed) diff --git a/context/glossary.md b/context/glossary.md index d6a960c4..1ba991b7 100644 --- a/context/glossary.md +++ b/context/glossary.md @@ -98,7 +98,9 @@ - `setup dispatch outcome`: Execution model in `cli/src/services/setup/mod.rs` (`SetupDispatch`) where setup either proceeds with a selected/non-interactive target or exits as cancelled without file changes. - `setup embedded asset manifest`: Compile-time generated file index emitted by `cli/build.rs` into `OUT_DIR/setup_embedded_assets.rs`, embedding bytes from Pkl-generated `OUT_DIR/pkl-generated/config/.{opencode,claude,pi}/**` plus staged `OUT_DIR/static/hooks/**` as deterministic normalized relative-path entries consumed by `cli/src/services/setup/mod.rs`; `OPENCODE_EMBEDDED_ASSETS`, `CLAUDE_EMBEDDED_ASSETS`, and `PI_EMBEDDED_ASSETS` all back live setup targets. - `setup required-hook embedded assets`: Setup-service accessors in `cli/src/services/setup/mod.rs` (`iter_required_hook_assets`, `get_required_hook_asset`) that expose canonical embedded templates for `pre-commit`, `commit-msg`, and `post-commit` without runtime config reads. -- `setup required-hook install orchestration`: Setup-service flow in `cli/src/services/setup/mod.rs` (`install_required_git_hooks`) that resolves repository root + effective hooks directory via git truth, installs canonical required hooks with deterministic per-hook outcomes (`Installed`, `Updated`, `Skipped`), enforces executable permissions, and uses a unified remove-and-replace policy that removes existing hooks before swapping staged content with deterministic recovery guidance on swap failure. +- `SCE managed block`: The CLI-presence check plus `sce hooks ` invocation in each canonical hook template (`cli/assets/hooks/{pre-commit,commit-msg,post-commit}`), delimited by `# >>> sce managed block (do not edit) >>>` / `# <<< sce managed block <<<` comment markers so the same block content can be embedded inside a foreign hook without disturbing content around it (see `context/sce/setup-githooks-hook-asset-packaging.md`). The block propagates an available `sce` command's exit status by capturing `$?` and calling `exit` explicitly rather than by `exec`, so it terminates the script deterministically even when appended after other content. A pure merge module computes hook install bytes against this marker pair (see `setup hook-merge seam`); both `sce setup --hooks` install (see `setup required-hook install orchestration`) and `sce doctor` hook inspection decide currency against this marker pair rather than whole-file byte comparison, so a hook a repository has extended around the block still reports current. +- `setup hook-merge seam`: Pure module `cli/src/services/setup/hook_merge.rs`, covering `pre-commit`, `commit-msg`, and `post-commit`. `merge_or_create_hook(existing: Option<&[u8]>, canonical: &[u8], hook_name: &str) -> Result` returns `canonical` verbatim (`HookMergeKind::Created`) when no hook exists; otherwise it locates the `SCE managed block` marker pair by exact line match. A hook already carrying a balanced marker pair identical to the canonical block returns its bytes unchanged (`AlreadyCurrent`); one whose block differs gets that block spliced in place between the same marker lines, leaving surrounding content untouched (`ManagedBlockReplaced`); a marker-free hook containing the legacy pre-marker guidance URL (`https://sce.crocoder.dev/docs/getting-started#install-cli`) is treated as SCE-owned wholesale and replaced entirely with `canonical` (also `ManagedBlockReplaced`); any other marker-free hook is foreign and kept as an exact byte prefix with the canonical block appended after it (`AppendedToForeign`). An unbalanced or partial marker pair is a hard, deterministic error naming `hook_name`, with no bytes returned. For the `AppendedToForeign` case, `HookMerge.unreachable_block_advisory` is set when the foreign hook's last non-blank, non-comment line sits at zero indentation and starts with `exec ` or `exit` — a narrow heuristic (no shell parsing) flagging that the appended block would never run. This module is pure and filesystem-free per "Unit testing in Nix sandbox"; required-hook install calls it (see `setup required-hook install orchestration`), and doctor hook inspection (`cli/src/services/hooks/lifecycle.rs`, `cli/src/services/doctor/inspect.rs`) also calls it, reporting a hook `Current` only when merging the canonical template into its on-disk bytes is a no-op — including treating an unbalanced or partial marker pair as `Stale` rather than `Unknown`, so `sce doctor --fix` repairs it. +- `setup required-hook install orchestration`: Setup-service flow in `cli/src/services/setup/mod.rs` (`install_required_git_hooks`, backed by the rename-injectable `install_required_git_hooks_with_rename`) that resolves repository root + effective hooks directory via git truth, then for each hook computes the bytes to stage with the `setup hook-merge seam` (`hook_merge::merge_or_create_hook`) instead of writing the canonical asset verbatim, reports deterministic per-hook outcomes (`Installed`, `Updated`, `Skipped`) against that merged content plus the executable bit, enforces executable permissions, sets `RequiredHookInstallResult.unreachable_block_advisory` (rendered as a named advisory line) when an appended block would be unreachable, and uses the `setup atomic-swap` policy (see `setup atomic-swap`) — staged content is renamed directly over an existing hook without unlinking it first — with deterministic recovery guidance on swap failure. - `setup hooks CLI mode`: `sce setup` behavior activated by `--hooks` (with optional `--repo `), supporting both hooks-only runs and composable target+hooks runs in one invocation; implemented through `cli/src/services/setup/command.rs` + `cli/src/services/setup/mod.rs`, enforces deterministic compatibility validation (`--repo` requires `--hooks`; target flags remain mutually exclusive), and emits stable setup/hook status output. - `setup repo gate`: Preflight check in `cli/src/services/setup/command.rs` that calls `cli/src/services/setup/mod.rs` (`ensure_git_repository`) before any setup writes begin; enforces that all `sce setup` modes (config-only, hooks-only, combined, and interactive) require the current directory to be inside a git repository, failing with actionable guidance to run `git init` and rerun `sce setup` when the precondition is not met. - `setup local bootstrap`: Pre-install setup bootstrap behavior now owned by lifecycle providers: `ConfigLifecycle::setup` creates missing `.sce/config.json` with the canonical schema-only payload (`{"$schema": "https://sce.crocoder.dev/config.json"}`), `LocalDbLifecycle::setup` initializes the canonical local DB via `LocalDb::new()`, and `AgentTraceDbLifecycle::setup` creates/reuses checkout identity, resolves repository identity, initializes the repository-scoped Agent Trace DB via `agent_trace_storage`, and records repository ID, checkout ID, and `database_path`; the setup command aggregates these calls before config/hooks dispatch across all normal setup modes after context baseline bootstrap. @@ -155,10 +157,10 @@ - `sce policy command adapter`: Hidden/internal `sce policy bash` command in `cli/src/services/bash_policy.rs` that exposes the Rust bash-policy evaluator to hook callers. It reads JSON from STDIN, resolves bash-policy config from the project root (git root with current-directory fallback), evaluates the command against active policies, and emits hook-safe output: Claude Code deny JSON (`hookSpecificOutput` with `permissionDecision: "deny"`) or empty string for allowed commands in `--output claude-hook` mode (default), and structured `{"status","decision","command","normalized_argv","reason","policy_id"}` JSON in `--output json` mode. Input modes are `--input claude-pre-tool-use` (default, parses Claude `PreToolUse` event JSON with `tool_name`/`tool_input.command`) and `--input normalized` (parses `{"command":...}` for OpenCode delegation). The command uses explicit `--input`/`--output` flags rather than auto-detection; Claude Code hooks invoke `sce policy bash` with defaults, while OpenCode plugin delegation passes `--input normalized --output json`. Invalid invocation/input returns deterministic validation diagnostics without executing target commands. - `bash policy redundancy warning`: Non-fatal config validation output emitted when `forbid-git-all` and `forbid-git-commit` are enabled together; the config remains valid, but `sce config show|validate` reports the overlap deterministically as a warning instead of an error. - `auth config baked default`: Optional key-declared fallback in `cli/src/services/config/mod.rs` (with schema/parsing in `schema.rs`) used only after env and config-file inputs are absent; the first implemented case is `workos_client_id`, which currently falls back to `client_sce_default`. -- `setup install engine`: Installer in `cli/src/services/setup/mod.rs` (`install_embedded_setup_assets`) that writes each embedded setup asset into its own staged file next to its final destination under repository-root `.opencode/`/`.claude/`/`.pi/`, removing only that exact destination file if present, then swapping the staged content into place. It never removes an integration target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. After the per-asset install loop, it prunes stale SCE-owned paths (see `setup catalog-derived pruning`). Two assets, the Claude target's `settings.json` and the OpenCode target's `opencode.json`, stage merged content instead of the embedded asset's bytes verbatim (see `setup config-merge seam`). +- `setup install engine`: Installer in `cli/src/services/setup/mod.rs` (`install_embedded_setup_assets`) that writes each embedded setup asset into its own staged file next to its final destination under repository-root `.opencode/`/`.claude/`/`.pi/`, then swaps it into place via the `setup atomic-swap` policy (see `setup atomic-swap`) — renaming the staging file directly over the destination without unlinking it first. It never removes an integration target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. After the per-asset install loop, it prunes stale SCE-owned paths (see `setup catalog-derived pruning`). Two assets, the Claude target's `settings.json` and the OpenCode target's `opencode.json`, stage merged content instead of the embedded asset's bytes verbatim (see `setup config-merge seam`). - `setup catalog-derived pruning`: `prune_stale_assets_for_concrete_target` in `cli/src/services/setup/mod.rs`, run after per-asset install, deletes every path the full embedded catalog for a concrete target claims but the resolved selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), then `remove_empty_ancestor_directories` removes any parent directory left empty by that deletion, walking upward until it reaches the target root or a directory that still holds something (a user file inside an SCE-owned skill directory keeps that directory from being removed). Pruning is stateless and derives only from the compiled-in catalog; no install manifest is persisted, so an orphaned file installed by an older `sce` binary under a name the current catalog no longer knows is left alone. - `setup config-merge seam`: Pure module `cli/src/services/setup/config_merge.rs`, covering `.claude/settings.json` and `.opencode/opencode.json`. Both `merge_or_create_claude_settings` and `merge_or_create_opencode_config` share the shape `(existing_bytes: Option<&[u8]>, generated_bytes: &[u8], source_path: &str) -> Result>`: each returns `generated_bytes` verbatim when no existing file is present; otherwise each parses both as JSON (an existing-file parse failure is a hard error naming `source_path`, with no write) and delegates to a pure merge function that copies the SCE-owned `$schema` key from the generated document and preserves every other top-level key from the existing document untouched. `merge_claude_settings` additionally replaces, per hook event key the generated `hooks` object declares, only the entries whose `hooks[].command` contains the SCE ownership marker `run-sce-or-show-install-guidance.sh`, appended after the surviving non-SCE entries; event keys the generated document does not declare are left untouched. `merge_opencode_config` additionally merges the `plugin` array as a set: existing entries whose path starts with the SCE ownership marker `./plugins/sce-` are dropped structurally (so a stale plugin path an older or renamed catalog installed is recognized and pruned even after the current generated document stops declaring it), then the generated document's `plugin` entries are appended. `install_single_asset_with_rename` (`mod install` in `cli/src/services/setup/mod.rs`) detects each merge target via `is_claude_settings_merge_target` / `is_opencode_config_merge_target` and stages the merged bytes instead of the embedded asset's bytes before the shared stage/swap step. The same module also exposes `claude_settings_fragment_is_current` / `opencode_config_fragment_is_current`, which merge `existing_bytes` into `generated_bytes` and compare the merged `Value` against the existing one: a no-op merge means the existing file already carries a current copy of the SCE-owned fragment, whatever else it holds. `cli/src/services/doctor/inspect.rs` uses these two functions (instead of byte-exact `sha256`) to inspect `.claude/settings.json` and `.opencode/opencode.json`, and `crate::services::setup::repair_merge_target_asset` lets `sce doctor --fix` repair a drifted merge target by reinstalling just that one asset through the same merge-install path. -- `setup remove-and-replace`: Per-file replacement choreography in `cli/src/services/setup/mod.rs` where an existing destination file is removed before staged content is swapped into its place; on swap failure, the engine cleans that file's staging artifact and returns deterministic recovery guidance naming its path (recover from version control). No backup artifacts are created. Config install (`install_embedded_setup_assets`) applies this at individual-asset granularity and never removes an integration target directory as a whole; required-hook install applies it per hook file. +- `setup atomic-swap`: Per-file replacement choreography in `cli/src/services/setup/mod.rs` where staged content is renamed directly over an existing destination file, without ever unlinking that destination first — `fs::rename` replaces it atomically on both Unix and Windows, so a rename failure leaves the prior destination content and permissions untouched; on swap failure, the engine cleans that file's staging artifact and returns deterministic recovery guidance naming its path (recover from version control). No backup artifacts are created. Config install (`install_embedded_setup_assets`) applies this at individual-asset granularity and never removes an integration target directory as a whole; required-hook install applies it per hook file. Formerly called "setup remove-and-replace"; that name predates the removal of the pre-swap unlink step. - `hooks command routing contract`: Current hook command parser/dispatcher plus runtime wiring in `cli/src/services/hooks/mod.rs` (`HookSubcommand`, `run_hooks_subcommand`) supports `pre-commit`, `commit-msg `, `post-commit`, `post-rewrite `, `diff-trace`, and `conversation-trace` with deterministic invocation validation/usage errors; `session-model` is no longer supported. `commit-msg` is the only active attribution path behind the attribution hooks gate and staged-diff AI-overlap preflight; `pre-commit`/`post-rewrite` are no-ops; `post-commit` persists intersections and built Agent Trace payloads; `diff-trace` persists DB-only AgentTraceDb rows using tool-prefixed `session_id` values plus direct `model_id` and `tool_version` values (session-model fallback was removed in the `remove-session-models-direct-claude-model-id` plan). - `Claude raw hook capture (removed)`: Former hidden/internal `sce hooks claude-capture ` intake path removed in T05 of the `claude-typescript-model-cache-remove-rust-capture` plan. Rust now exposes `diff-trace` and `conversation-trace` intakes for active Claude/OpenCode editor runtimes; `session-model` is also removed from the supported hook command surface. The removed route previously wrote pretty-printed JSON artifacts under `context/tmp/claude/` without AgentTraceDb writes. See `context/sce/claude-raw-hook-capture.md`. - `deferred sync command`: `sce sync` has no command wiring and no `cli/src/services/sync.rs` module in the current runtime. Local DB initialization and health ownership are split between setup and doctor instead. diff --git a/context/overview.md b/context/overview.md index 0cbed150..f0532e2f 100644 --- a/context/overview.md +++ b/context/overview.md @@ -25,7 +25,7 @@ Agent Trace lifecycle setup now resolves repository storage, creates/reuses chec The CLI now also includes a shared text styling service in `cli/src/services/style.rs` that provides deterministic color enablement via `owo-colors`, automatic TTY detection, and `NO_COLOR` compliance for human-facing text output; stdout help/text surfaces, stderr diagnostics, and interactive prompt-adjacent text now reuse that shared styling policy while JSON, completion, and other non-interactive/machine-readable flows remain unstyled. The service exports color-detection, conditional styling, help/diagnostic/label/prompt styling, and `banner_with_gradient()` helpers for use across command surfaces while preserving pipe-safe output for non-interactive environments. The `setup` command includes an `inquire`-backed target-selection flow: default interactive selection for OpenCode/Claude/Pi/All with required-hook installation in the same run, explicit non-interactive target flags (`--opencode`, `--claude`, `--pi`, `--all`), standalone `--bootstrap-context` for additive durable-context baseline creation without integration installs, deterministic mutually-exclusive validation, and non-destructive cancellation exits; the former `--both` flag was removed in favor of `--all` (opencode+claude+pi). Every normal successful setup path also ensures the same context baseline after the Git gate. Workflows the catalog marks optional are installed only when a repository opts in: the repeatable `sce setup --workflow ` flag names the selection for a run, an omitted flag reuses the selection persisted in `integrations.optional_workflows`, and the resolved selection filters the installed target assets and is written back to repo-local config. Interactive runs ask for the selection instead: a multi-select prompt follows target selection with every row unchecked on a first run and pre-checked from the persisted selection afterwards, cancelling it exits non-destructively like the target prompt, and the prompt is skipped when the catalog marks no workflow optional. `brownfield` is currently the only optional workflow, so a default run installs the five core workflows and no brownfield assets. `sce doctor` scopes its integration checks to that same recorded selection, so it never reports an unselected optional workflow's files as missing. For repository generation consumers, `config/pkl/generator-inputs.txt` declares the canonical Pkl/plugin input set and `scripts/produce-cli-generated-input.sh` owns its discovery, two-pass `config/pkl/generate.pkl` evaluation, determinism comparison, payload/input inventories, in-flight input-mutation rejection, atomic handoff publication, and staging cleanup. `scripts/run-cli-cargo.sh` creates a fresh temporary destination, delegates generation to that producer, invokes the requested Cargo workflow with `SCE_CLI_GENERATED_INPUT_DIR`, and removes the handoff after Cargo success, failure, or handled signals. `config/pkl/check-generated.sh` delegates the same production mechanics while retaining contract and path assertions. `scripts/prepare-cli-generated-assets.sh` moves the producer-validated Pkl payload and checksums into the unchanged package fallback, adds hooks, migrations, and the Agent Trace schema, and appends only those static checksums to the combined inventory. The root flake's pre-Cargo `cliGeneratedInput` derivation invokes the same producer from a declarative source containing the producer plus its declared inputs. `cli/build.rs` rejects missing, incomplete, modified, or stale repository handoffs, copies the validated payload into Cargo `OUT_DIR/pkl-generated`, stages static inputs under `OUT_DIR/static`, and writes setup-asset, optional-workflow-catalog, and migration Rust manifests into `OUT_DIR`; it never invokes Pkl. Published crates carry the ignored packaging-only fallback, and unpacked downstream builds validate and copy it into their own `OUT_DIR` without requiring Pkl or parent repository paths. -The setup service also provides repository-root install orchestration: it resolves the repository root, ensures the additive durable-context baseline, then for normal modes derives a repo-root-scoped `AppContext` from the runtime command context, aggregates `ServiceLifecycle::setup` calls across lifecycle providers (config → local_db → auth_db → agent_trace_db → hooks when requested), handles interactive or flag-based target selection for config asset installation, and reports deterministic completion details (selected target(s) and installed file counts). Setup installs config assets (`.opencode`/`.claude`/`.pi`) per file: each embedded asset is staged and swapped into its own destination path, creating parent directories as needed, without removing or recreating the target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. Two assets are merge targets rather than verbatim writes: Claude's `.claude/settings.json` and OpenCode's `.opencode/opencode.json`. For each, setup JSON-merges the generated document into the user's existing file rather than overwriting it, and fails deterministically without writing if the existing file is not valid JSON; a missing file is still created from the generated document verbatim. Claude's merge replaces only SCE-owned hook entries (identified by a command containing `run-sce-or-show-install-guidance.sh`) and the `$schema` key while preserving every other key and hook entry untouched. OpenCode's merge replaces the `$schema` key and merges the `plugin` array as a set: any entry shaped like an SCE plugin path (`./plugins/sce-*`) is dropped, structurally, so a path an older or renamed catalog once installed is still recognized and pruned, and the generated document's canonical plugin entries are appended after the surviving user entries. Required-hook install keeps the prior remove-and-replace choreography at file granularity — it removes an existing hook file before swapping staged content. Both flows return deterministic recovery guidance (recover from version control) on swap failure, without creating backup artifacts. After installing, config install prunes stale SCE-owned assets: it deletes every path the full embedded catalog for the target claims but the current selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), then removes any parent directory left empty by that deletion, leaving a directory intact if a user file still lives inside it. The setup command gates all modes on an existing git repository before any writes. Internally, `cli/src/services/setup/mod.rs` now separates install-flow logic from interactive prompt logic through focused support seams. +The setup service also provides repository-root install orchestration: it resolves the repository root, ensures the additive durable-context baseline, then for normal modes derives a repo-root-scoped `AppContext` from the runtime command context, aggregates `ServiceLifecycle::setup` calls across lifecycle providers (config → local_db → auth_db → agent_trace_db → hooks when requested), handles interactive or flag-based target selection for config asset installation, and reports deterministic completion details (selected target(s) and installed file counts). Setup installs config assets (`.opencode`/`.claude`/`.pi`) per file: each embedded asset is staged and swapped into its own destination path, creating parent directories as needed, without removing or recreating the target directory as a whole, so files a repository owns inside an SCE-managed target directory survive a setup run untouched. Two assets are merge targets rather than verbatim writes: Claude's `.claude/settings.json` and OpenCode's `.opencode/opencode.json`. For each, setup JSON-merges the generated document into the user's existing file rather than overwriting it, and fails deterministically without writing if the existing file is not valid JSON; a missing file is still created from the generated document verbatim. Claude's merge replaces only SCE-owned hook entries (identified by a command containing `run-sce-or-show-install-guidance.sh`) and the `$schema` key while preserving every other key and hook entry untouched. OpenCode's merge replaces the `$schema` key and merges the `plugin` array as a set: any entry shaped like an SCE plugin path (`./plugins/sce-*`) is dropped, structurally, so a path an older or renamed catalog once installed is still recognized and pruned, and the generated document's canonical plugin entries are appended after the surviving user entries. Required-hook install uses the same per-file stage/atomic-swap choreography as config-asset install — the staging file is renamed directly over an existing hook without unlinking it first, so a rename failure leaves the prior hook untouched. Both flows return deterministic recovery guidance (recover from version control) on swap failure, without creating backup artifacts. After installing, config install prunes stale SCE-owned assets: it deletes every path the full embedded catalog for the target claims but the current selection did not install (a deselected optional workflow, or an asset a newer catalog renamed or dropped), then removes any parent directory left empty by that deletion, leaving a directory intact if a user file still lives inside it. The setup command gates all modes on an existing git repository before any writes. Internally, `cli/src/services/setup/mod.rs` now separates install-flow logic from interactive prompt logic through focused support seams. The CLI now also applies baseline security hardening for reliability-driven automation: diagnostics/logging paths use deterministic secret redaction, `sce setup --hooks --repo ` canonicalizes and validates repository paths before execution, and setup write flows run explicit directory write-permission probes before staging/swap operations. The config service now provides deterministic runtime config resolution with explicit precedence (`flags > env > config file > defaults`), strict config-file validation (`$schema`, `log_level`, `log_format`, `log_dir`, `timeout_ms`, `workos_client_id`, and nested `policies.bash`, `policies.attribution_hooks.enabled`, plus `policies.database_retry` with per-DB `connection_open`/`query` retry policy specs), deterministic default discovery/merge of global+local config files (`${config_root}/sce/config.json` then `.sce/config.json` with local override, where `config_root` comes from the shared default-path seam with XDG/`dirs::config_dir()` config-root resolution), defaults for the resolved observability value set (`log_level=error`, `log_format=text`, `log_dir=/sce/logs`), shared auth-key resolution with optional baked defaults starting at `workos_client_id`, first-class bash-policy preset/custom parsing with deterministic conflict and duplicate-prefix validation, custom-policy `satisfied_by` wrapper exemption (a policy does not fire when the matched command was unwrapped from a declared wrapper such as `nix shell nixpkgs#ripgrep`), and a canonical Pkl-authored `sce/config.json` JSON Schema generated beneath Cargo `OUT_DIR` and embedded by `cli/src/services/config/mod.rs` for both `sce config validate` and doctor-time config checks. Runtime startup config loading keeps parity with that schema by accepting its `$schema` declaration in repo-local and global config files, so startup commands such as `sce version` no longer fail before dispatch on that field; the canonical declaration is `"https://sce.crocoder.dev/config.json"`. App-runtime observability now consumes flat logging keys through the shared resolver, so env values still override config-file values while config files provide deterministic fallback for `log_dir`; positive-integer `log_file_retention_limit` uses config-file/default precedence, defaults to `10`, and controls creation-triggered cleanup for primary and v2 log files; `sce config show` reports resolved observability/auth/policy values with provenance, while `sce config validate` is now a trimmed validation surface that reports only pass/fail plus validation errors or warnings in text and JSON modes. The canonical preset catalog and matching contract live in `config/pkl/base/bash-policy-presets.pkl` and `context/sce/bash-tool-policy-enforcement-contract.md`. Invalid default-discovered config files now also degrade gracefully at startup: `sce` keeps running with degraded observability defaults, logs `sce.config.invalid_config` warnings, and reserves hard failures for explicit `--config` / `SCE_CONFIG_FILE` targets or other truly invalid runtime observability inputs. @@ -65,7 +65,7 @@ The CLI now also includes an approved operator-environment doctor contract docum The local DB service now provides `LocalDb` as a thin `TursoDb` alias in `cli/src/services/local_db/mod.rs`; `LocalDbSpec` resolves the canonical local DB path from the shared default-path catalog and currently declares zero migrations. Shared Turso infrastructure lives in `cli/src/services/db/mod.rs`, where `DbSpec` and generic `TursoDb` support local or remote sync-mode opens, parent-directory creation, connection setup, synchronous query helpers, embedded migration execution, and shared DB lifecycle helpers. Auth DB persistence uses encrypted `AuthDb = EncryptedTursoDb` and token storage persists credentials through the `auth_credentials` table. Agent Trace persistence uses the sole `RepositoryAgentTraceDb = TursoDb` adapter at `/sce/repos//agent-trace.db`, with a one-file repository schema for `repository_metadata`, `diff_traces`, `post_commit_patch_intersections`, `agent_traces`, `messages`, `parts`, indexes/triggers, and no `checkout_id` columns on trace rows. The checkout-scoped `AgentTraceDb = TursoDb` adapter, its `agent_trace_db_path()`/`agent_trace_db_path_for_checkout()` helpers, and the 15-file `cli/migrations/agent-trace/` chain were removed by the `retire-legacy-agent-trace-db` plan; active hook runtime writes direct nullable diff-trace attribution without a `session_models` API/table dependency. The hooks command surface now also supports concrete runtime subcommand routing (`pre-commit`, `commit-msg`, `post-commit`, `post-rewrite`, `diff-trace`, and `conversation-trace`) with deterministic argument/STDIN validation; `session-model` is no longer supported. Current runtime behavior keeps commit-msg attribution enabled by default unless explicitly opted out: the attribution gate enables canonical trailer insertion in `commit-msg` only when the staged-diff AI-overlap preflight confirms AI/editor evidence (no trailer is appended when the preflight finds no overlap or encounters any error); `pre-commit`/`post-rewrite` remain deterministic no-ops, `post-commit` requires validated `--remote-url`, threads that URL into the Agent Trace flow, prints it to stderr, and remains the active bounded recent-diff-trace intersection path, and `diff-trace` is the active intake path for parsed STDIN `{ sessionID, diff, time, model_id?, tool_name, tool_version }` payload persistence with optional `model_id`, required non-empty `tool_name`, required nullable/non-empty `tool_version`, direct `model_id` and `tool_version` values (no session-model fallback), required `u64` millisecond `time`, same-tool-idempotent stored `session_id` prefixing (`oc_` for OpenCode, `cc_` for Claude, `pi_` for Pi), non-lossy AgentTraceDb `time_ms` conversion, and no parsed-payload artifact fallback. This behavior is documented in `context/sce/agent-trace-hooks-command-routing.md`. The removed `sce hooks claude-capture` raw capture route is documented in `context/sce/claude-raw-hook-capture.md` as a removed feature. The setup service now also exposes deterministic required-hook embedded asset accessors (`iter_required_hook_assets`, `get_required_hook_asset`) backed by canonical templates in `cli/assets/hooks/` for `pre-commit`, `commit-msg`, and `post-commit`; this behavior is documented in `context/sce/setup-githooks-hook-asset-packaging.md`. -The setup service now also includes required-hook install orchestration (`install_required_git_hooks`) that resolves repository root and effective hooks path from git truth, enforces deterministic per-hook outcomes (`Installed`/`Updated`/`Skipped`), and uses a unified remove-and-replace policy that removes existing hooks before swapping staged content with deterministic recovery guidance on swap failures; this behavior is documented in `context/sce/setup-githooks-install-flow.md`. +The setup service now also includes required-hook install orchestration (`install_required_git_hooks`) that resolves repository root and effective hooks path from git truth, computes the bytes to stage by merging the canonical hook template with any existing hook (preserving a foreign hook's content as an exact prefix with the SCE managed block appended, or bringing an SCE-owned block current in place) rather than writing canonical bytes verbatim, enforces deterministic per-hook outcomes (`Installed`/`Updated`/`Skipped`) against that merged content, surfaces a deterministic advisory when an appended block would be unreachable, and uses a unified atomic-swap policy that renames staged content directly over existing hooks without unlinking them first, with deterministic recovery guidance on swap failures; this behavior is documented in `context/sce/setup-githooks-install-flow.md`. The setup command parser/dispatch now also supports composable setup+hooks runs (`sce setup --opencode|--claude|--pi|--all --hooks`) plus hooks-only mode (`sce setup --hooks` with optional `--repo `), enforces deterministic compatibility validation (`--repo` requires `--hooks`; target flags remain mutually exclusive), and emits deterministic setup/hook outcome messaging (`installed`/`updated`/`skipped`); this behavior is documented in `context/sce/setup-githooks-cli-ux.md`. ## Repository model @@ -132,5 +132,5 @@ Lightweight post-task verification baseline (required after each completed task) - Use `context/sce/agent-trace-hooks-command-routing.md` for the implemented T02 `sce hooks` command routing contract (subcommand parsing, deterministic invocation errors, and initial runtime entrypoint behavior). - Use `context/sce/claude-raw-hook-capture.md` (removed feature) for the former hidden/internal Claude raw hook JSON capture intake. The `sce hooks claude-capture` CLI route, `ClaudeCaptureEvent`, `claude_transcript.rs`, and `RepoPaths::claude_capture_tmp_dir()` were removed in T05. Rust now exposes `diff-trace` and `conversation-trace` intakes for active Agent Trace hook data; `session-model` is also removed from the supported hook command surface. - Use `context/sce/setup-githooks-hook-asset-packaging.md` for the implemented `sce-setup-githooks-any-repo` T02 compile-time hook-template packaging contract and setup-service required-hook embedded accessor surface. -- Use `context/sce/setup-githooks-install-flow.md` for the implemented `sce-setup-githooks-any-repo` T03 required-hook install orchestration contract (git-truth hooks-path resolution, per-hook installed/updated/skipped outcomes, and remove-and-replace behavior). +- Use `context/sce/setup-githooks-install-flow.md` for the implemented `sce-setup-githooks-any-repo` T03 required-hook install orchestration contract (git-truth hooks-path resolution, per-hook installed/updated/skipped outcomes, and atomic-swap replacement behavior). - Use `context/sce/setup-githooks-cli-ux.md` for the implemented `sce-setup-githooks-any-repo` T04 setup command-surface contract (`--hooks`, optional `--repo`), compatibility validation rules, and deterministic hook setup messaging. diff --git a/context/patterns.md b/context/patterns.md index bdf860f9..13ebd1c0 100644 --- a/context/patterns.md +++ b/context/patterns.md @@ -136,9 +136,9 @@ - Treat setup prompt cancellation/interrupt as a non-destructive exit path with explicit user messaging (no file mutations and no partial side effects). - For repository setup-asset build prep, declare canonical generator inputs in `config/pkl/generator-inputs.txt` and route input discovery, two-pass Pkl evaluation, determinism comparison, payload/input inventory creation, in-flight input checks, atomic publication, and private staging cleanup through `scripts/produce-cli-generated-input.sh`. The Cargo wrapper, generated-output check, package-fallback helper, and Nix `cliGeneratedInput` derivation must consume that producer rather than implement those mechanics independently. Keep each consumer's domain checks separate: the generated-output check owns metadata/contract/negative/path assertions; packaging owns static hook/schema/migration staging and the combined Pkl-plus-static checksum inventory; Nix owns declarative producer/input source selection and pre-Cargo handoff wiring. Route build, run, targeted-test, Clippy, and local-install Cargo workflows through `scripts/run-cli-cargo.sh`, which passes the producer handoff through `SCE_CLI_GENERATED_INPUT_DIR` and owns cleanup around Cargo. Keep `cli/build.rs` free of Pkl subprocesses and source-tree generated mirrors. - For CLI database migration prep, keep SQL files under immediate `cli/migrations//` directories named `NNN_description.sql`; `cli/build.rs` stages those files under `OUT_DIR/static/migrations`, sorts by the numeric prefix before `_`, and writes deterministic `OUT_DIR/generated_migrations.rs` constants with `include_str!` references for service `DbSpec` consumers. -- For setup install execution, write each selected embedded asset into its own staging file next to its final destination, remove only that destination file if one already exists, then swap the staged content into place; never remove or recreate the integration target directory as a whole. On swap failure, clean the failing asset's staging path and return deterministic recovery guidance naming that asset's destination (recover from version control). No backup artifacts are created. After the install loop, prune stale SCE-owned paths by diffing the full embedded catalog for the target against the assets actually installed, deleting each catalog path not installed, then removing any parent directory left empty by that deletion (a directory still holding a user file fails to remove and survives). +- For setup install execution, write each selected embedded asset into its own staging file next to its final destination, then swap the staged content into place by renaming it directly over the destination — never unlink the destination first, since `fs::rename` already replaces an existing file atomically; never remove or recreate the integration target directory as a whole. On swap failure, clean the failing asset's staging path and return deterministic recovery guidance naming that asset's destination (recover from version control); the pre-existing destination content, if any, is untouched. No backup artifacts are created. After the install loop, prune stale SCE-owned paths by diffing the full embedded catalog for the target against the assets actually installed, deleting each catalog path not installed, then removing any parent directory left empty by that deletion (a directory still holding a user file fails to remove and survives). - For a config asset a user may already own and extend (`.claude/settings.json`, `.opencode/opencode.json`), do not write the embedded asset's bytes verbatim: compute the bytes to stage with a pure `serde_json`-based merge (`cli/src/services/setup/config_merge.rs`) that copies SCE-owned keys/entries from the generated document — identified by a fixed ownership marker, such as a hook command substring for Claude hooks or a plugin path prefix for OpenCode plugins — over the existing file, and preserves every other key and entry untouched. A parse failure on the existing file is a hard, deterministic error naming the file's path with no write; a missing file still gets the generated document verbatim. Keep this pure and filesystem-free per "Unit testing in Nix sandbox" below; the install seam reads the existing file and calls the merge before staging. -- For required-hook setup execution, resolve repository root and effective hooks directory from git (`rev-parse --show-toplevel`, `rev-parse --git-path hooks`), then apply deterministic per-hook outcomes (`Installed`, `Updated`, `Skipped`) with staged writes, executable-bit enforcement, and remove-and-replace behavior that removes existing hooks before swapping staged content. +- For required-hook setup execution, resolve repository root and effective hooks directory from git (`rev-parse --show-toplevel`, `rev-parse --git-path hooks`), then compute the bytes to stage with a pure merge (`cli/src/services/setup/hook_merge.rs::merge_or_create_hook`) — mirroring the config-asset merge-target pattern above — rather than the canonical asset's bytes verbatim: a foreign hook's content is kept as an exact byte prefix with the SCE managed block appended after it, an SCE-owned hook has only its block replaced or left unchanged, and a legacy pre-marker hook upgrades wholesale. Apply deterministic per-hook outcomes (`Installed`, `Updated`, `Skipped`) against the merged bytes plus executable bit, with staged writes, executable-bit enforcement, and the same atomic-swap behavior as config-asset install: the staged file is renamed directly over the existing hook without unlinking it first, so a rename failure leaves the prior hook's bytes and executable bit intact. Surface a deterministic advisory naming the hook when the appended block would be unreachable (a foreign hook whose last effective line is a zero-indent `exec`/`exit`). - For hook setup CLI UX, allow `--hooks` as both hooks-only and composable target+hooks execution (optional `--repo `), enforce deterministic option compatibility (`--repo` requires `--hooks`; target flags stay mutually exclusive), and emit stable section-ordered setup/hook status lines for automation-friendly logs. - For setup command messaging, emit deterministic completion output that includes selected target(s) and per-target install counts. - Keep module seams for future domains present and compile-safe even when behavior is deferred. @@ -151,7 +151,7 @@ - For service-owned operator health, keep command modules as thin aggregators over `ServiceLifecycle` providers once a lifecycle slice is wired: providers own diagnosis/fix problem production through narrow capability accessors, while command-specific report builders preserve existing output facts and rendering contracts. - Keep static lifecycle provider-list construction centralized in the lifecycle service layer so doctor/setup choose provider inclusion without maintaining parallel concrete provider lists. - Keep `ServiceLifecycle` trait signatures lifecycle-owned and capability-narrow; adapt lifecycle health/fix/setup records into doctor/setup-owned output records at command orchestration boundaries rather than making provider contracts depend on command modules or the full production context type. -- For repo-scoped hook-health diagnostics, resolve effective hooks location from git truth, distinguish git-unavailable vs outside-repo vs bare-repo failure modes explicitly, and compare required hook payload bytes against the canonical embedded hook assets so stale SCE-managed hook content is reported deterministically (`cli/src/services/doctor/mod.rs`, `cli/src/services/doctor/inspect.rs`, `cli/src/services/setup/mod.rs`). +- For repo-scoped hook-health diagnostics, resolve effective hooks location from git truth, distinguish git-unavailable vs outside-repo vs bare-repo failure modes explicitly, and decide hook content currency the same way install does — merging the canonical template into the on-disk bytes via `hook_merge::merge_or_create_hook` and reporting `Current` only when that merge is a no-op — rather than whole-file byte equality, so foreign content a repository has appended around the SCE managed block does not read as drift; an unbalanced or partial managed block also reports `Stale` so `--fix` repairs it (`cli/src/services/hooks/lifecycle.rs`, `cli/src/services/doctor/inspect.rs`, `cli/src/services/setup/hook_merge.rs`). - For cross-service CLI dependencies exposed through the borrowed `AppContext` view, prefer shared capability/accessor traits over one-off per-service abstractions; keep production wrappers thin over `std::fs` and `git` process execution until call-site migration tasks approve deeper service refactors, and keep command execution generic over the narrow accessors each command needs where practical. - For future CLI domains, define trait-first service contracts with request/plan models in `cli/src/services/*` and keep placeholder implementations explicitly non-runnable until production behavior is approved. - Model deferred integration boundaries with concrete event/capability data structures (for example hook-runtime attribution snapshots/policies and cloud-sync checkpoints) so later tasks can implement behavior without reshaping public seams. diff --git a/context/plans/git-hook-append-and-atomic-asset-swap.md b/context/plans/git-hook-append-and-atomic-asset-swap.md new file mode 100644 index 00000000..37c54478 --- /dev/null +++ b/context/plans/git-hook-append-and-atomic-asset-swap.md @@ -0,0 +1,201 @@ +# Plan: git-hook-append-and-atomic-asset-swap + +## Change summary + +Two defects in the setup install write path, both surfaced by review of the +`non-destructive-setup-install` change. First, every setup write — config assets +and required git hooks alike — unlinks the destination file before renaming the +staged replacement over it (`cli/src/services/setup/mod.rs:1408`, `:979`). The +staging file always lives in the destination's own directory, so `fs::rename` +already replaces atomically and the unlink buys nothing; it only opens a window +in which the file does not exist, and on a rename failure the error path deletes +the staging file too, leaving neither copy. That was tolerable when every +installed file was SCE-owned and regenerable, but `.claude/settings.json` and +`.opencode/opencode.json` are now merge targets holding the user's +`permissions`, `env`, `model`, and `mcp` keys, which setup cannot reconstruct. +This plan drops both pre-deletes and relies on atomic rename. + +Second, `sce setup --hooks` still replaces `.git/hooks/pre-commit`, +`commit-msg`, and `post-commit` wholesale, so a repository already running husky, +lefthook, or a hand-written hook loses it. The predecessor plan named this as +out of scope and left the design undecided. This plan implements the append +variant the user chose: each canonical hook payload is delimited by SCE managed +block markers, and install replaces only that block — creating the full script +when no hook exists, replacing the block in place when one carries it, and +appending the block after the existing content when a foreign hook is found. The +foreign hook keeps its shebang, its content, and its position first in the run +order; SCE runs last, which is what `commit-msg` trailer insertion needs anyway. +`sce doctor` moves from byte-exact hook comparison to the same SCE-fragment +comparison the two JSON merge targets already use, so a legitimately extended +hook is not reported as drift. + +This extends the existing non-destructive install policy to git hooks and +corrects the swap choreography that policy is built on. It preserves every +existing outcome vocabulary (`Installed`/`Updated`/`Skipped`, +`[PASS]`/`[FAIL]`/`[MISS]`, `Missing`/`Current`/`Stale`/`Unknown`) and adds no +new one. + +## Acceptance criteria + +How this plan is proven complete. Each criterion is observable and names the +check that proves it. `/validate` runs these checks; no task in the stack +performs final validation. + +- [x] AC1: A rename failure while installing a setup config asset leaves the previous file's content intact on disk, and leaves no staging artifact. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — the injected-`rename_fn`-failure test asserts the pre-existing destination bytes are unchanged after the error, in addition to the existing path-naming, recovery-guidance, and staging-cleanup assertions. +- [x] AC2: A rename failure while installing a required git hook leaves the previous hook file's content and executable bit intact. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — an injected hook `rename_fn` failure asserts the seeded prior hook bytes and mode survive. +- [x] AC3: Running `sce setup --hooks` in a repository whose `pre-commit` is a foreign script preserves that script byte-for-byte at the head of the file and appends the SCE managed block after it, with the file executable. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — an integration test seeds a foreign `pre-commit`, installs, and asserts the original content is a prefix of the result, the managed block follows it, and the file is executable. +- [x] AC4: Re-running setup against a hook that already carries the current SCE managed block reports `skipped` and leaves the file byte-identical, whether or not foreign content sits above the block. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — a two-run test asserts identical bytes and `RequiredHookInstallStatus::Skipped` on the second run for both the foreign-plus-block and block-only shapes. +- [x] AC5: A hook carrying a pre-marker SCE payload is recognized as SCE-owned and replaced wholesale with the marker form, without preserving any of the old payload as if it were foreign content. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::hook_merge` — a unit test feeds the legacy payload and asserts the output equals the canonical marker-form document. +- [x] AC6: `sce doctor` reports a hook carrying foreign content plus a current SCE block as current, reports it stale when the block is missing or outdated, and `sce doctor --fix` repairs it by rewriting only the block while preserving the foreign content. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` — filesystem-backed tests covering current-with-foreign-content, drifted block, and post-`--fix` re-inspection with the foreign content asserted intact. +- [x] AC7: When a foreign hook's last effective line is a zero-indent `exec` or `exit`, setup still installs the block and reports a deterministic advisory naming that hook, because the appended block would not run. + - Validate: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — a test seeds a foreign hook ending in `exec some-tool "$@"`, asserts the advisory is present in the install result, and asserts no advisory for a foreign hook ending in an ordinary command. + +### Full validation + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` +- `nix run .#pkl-check-generated` +- `nix flake check` +- `sh -n cli/assets/hooks/pre-commit && sh -n cli/assets/hooks/commit-msg && sh -n cli/assets/hooks/post-commit` + +### Context sync + +- `context/sce/setup-no-backup-policy-seam.md` — the per-file choreography is no longer "stage, remove destination, swap" but "stage, atomically swap"; required-hook install is no longer remove-and-replace but a managed-block merge, making it a second merge-target family alongside the two JSON configs. +- `context/sce/setup-githooks-install-contract.md` — "Preservation and replacement policy" and "Rollback guarantees" both state remove-before-swap and wholesale replacement. +- `context/sce/setup-githooks-install-flow.md` — "Staged write and remove-and-replace behavior" and the per-hook outcome definitions. +- `context/sce/setup-githooks-hook-asset-packaging.md` — the canonical templates now carry managed-block markers and no longer `exec`. +- `context/sce/doctor-human-text-contract.md` — hook rows are decided by SCE-fragment currency rather than byte-exact `sha256`. +- `context/patterns.md` — the setup-install execution pattern (currently "remove only that destination file if one already exists, then swap") and the required-hook execution pattern (currently "remove-and-replace behavior that removes existing hooks before swapping staged content"). +- `context/architecture.md` — the `cli/src/services/setup/mod.rs` paragraph describing install-engine and required-hook choreography. +- `context/overview.md` — the setup-service paragraph stating remove-and-replace for hooks. +- `context/glossary.md` — the SCE managed block term, if the glossary carries the merge-target/ownership-marker vocabulary. +- `context/context-map.md` — annotation updates for any of the above whose subject line changes. + +This change makes SCE a bounded in-place editor of user-authored git hooks, +which the synchronization decision gate may judge a qualifying system-wide +decision. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/setup/mod.rs` (config-asset and required-hook install choreography), a new pure hook-merge module under `cli/src/services/setup/`, `cli/assets/hooks/{pre-commit,commit-msg,post-commit}`, `cli/src/services/doctor/inspect.rs` hook content inspection, and the durable-context files named under Context sync. +- **Out of scope:** `core.hooksPath` resolution, which already works — `git rev-parse --git-path hooks` returns the configured path (verified against git 2.54.0), contradicting the predecessor plan's open question. +- **Out of scope:** the two JSON merge targets and `config_merge.rs`. Their merge semantics are unchanged; only the swap step beneath them changes. +- **Out of scope:** a `.d/` dispatcher directory, hook-manager detection, or cooperation protocol with husky/lefthook. +- **Out of scope:** repairing the stale `## Validation Report` in `context/plans/non-destructive-setup-install.md`. +- **Constraints:** No backup artifacts and no backup-based rollback (`context/sce/setup-no-backup-policy-seam.md`). No new crate dependency; the hook merge is byte/string work over the existing asset bytes. Unit tests stay filesystem-free (`context/patterns.md`, "Unit testing in Nix sandbox"): the merge is pure and unit-tested, install and doctor behavior belongs in integration tests. Hook payloads stay POSIX `sh`. Existing outcome vocabularies are not extended. +- **Non-goal:** a general shell-script merge engine, or parsing shell to determine reachability. The unreachable-block advisory is a last-effective-line heuristic, not an analysis. +- **Non-goal:** guaranteeing the SCE block survives a third-party hook manager reinstalling its own hooks. This is cooperative. + +## Assumptions + +- SCE ownership of an existing hook is matched structurally, mirroring the `./plugins/sce-` precedent in `config_merge.rs`: a file carrying the managed-block markers is owned within that block, and a pre-marker file containing the canonical guidance URL (`https://sce.crocoder.dev/docs/getting-started#install-cli`) is owned wholesale. Any other file is foreign and preserved. +- The appended block runs last on purpose. `commit-msg` must be last so the SCE trailer lands on the final message, and `pre-commit`/`post-commit` are order-indifferent. +- The block invokes `sce hooks "$@"` and propagates its status by ordinary exit rather than `exec`, so a block appended after SCE's by some later tool would still run. The canonical fresh-install script uses the identical block. +- The block relies on the invoking hook's top-level `"$@"`. A foreign hook that `shift`s its arguments before the block changes what SCE receives; this is accepted rather than defended against. +- Removing the pre-delete is safe on Windows: `std::fs::rename` replaces an existing destination file there as it does on Unix. + +## Task stack + +- [x] T01: `Replace setup destinations by atomic rename` (status:done) + - Task ID: T01 + - Goal: Setup never unlinks a destination before renaming staged content over it, so a failed swap leaves the previous file intact. + - Boundaries (in/out of scope): In — delete the `if destination.exists() { fs::remove_file(...) }` block in `install_single_asset_with_rename`, delete the `remove_existing_install_target(&hook_path)` call and the now-unused `remove_existing_install_target` helper in `install_single_required_hook_with_rename`, collapse that function's two rename branches into one, and update the existing rename-failure tests to assert the prior file survives. Out — hook content merging, doctor, and any change to staging-path allocation or recovery-guidance text. + - Dependencies: none + - Done when: Neither install path calls `remove_file`/`remove_dir_all` on a destination before renaming; `remove_existing_install_target` is gone; the config-asset rename-failure test asserts the seeded prior content is unchanged after the error alongside its existing assertions; an equivalent hook rename-failure test asserts prior hook bytes and mode survive; recovery guidance and staging cleanup behavior are unchanged. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets`. + - Implementation evidence: `cli/src/services/setup/mod.rs` — removed the pre-rename `fs::remove_file` in `install_single_asset_with_rename`; collapsed `install_single_required_hook_with_rename`'s install/update branches into one rename call (status and recovery-guidance context still chosen from whether a prior hook existed); deleted `remove_existing_install_target`; added `pub(super) install_required_git_hooks_with_rename` (mirroring the existing `install_embedded_setup_assets_with_rename` seam) so hook rename failures are test-injectable; updated `install_cleans_up_staging_and_reports_asset_path_on_rename_failure` to seed prior destination content and assert it survives; added `hook_install_leaves_prior_hook_intact_on_rename_failure` asserting prior hook bytes, executable mode, and staging cleanup survive a rename failure. + - Verification outcome: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 38 passed, 0 failed. `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` — clean, no warnings. + - Deviations/assumptions: Exposed `install_required_git_hooks_with_rename` (crate-internal `pub(super)`) as a minimal, in-convention test seam; no other deviation from the reviewed scope. + +- [x] T02: `Delimit canonical hook payloads with SCE managed block markers` (status:done) + - Task ID: T02 + - Goal: Each canonical hook template carries its SCE logic inside stable start/end markers and exits by status propagation instead of `exec`, so the same block can be embedded in a foreign hook. + - Boundaries (in/out of scope): In — `cli/assets/hooks/{pre-commit,commit-msg,post-commit}`: wrap the missing-CLI guidance plus the `sce hooks ` invocation in `# >>> sce managed block (do not edit) >>>` / `# <<< sce managed block <<<`, replace `exec sce hooks ...` with a status-propagating invocation, and keep the shebang, `set -eu`, the branded guidance text, the terminal-only ANSI policy, and `post-commit`'s `origin`/`--remote-url` behavior unchanged. Out — the merge module, install wiring, and doctor; byte-exact comparison still governs install/doctor at the end of this task, so a pre-existing hook simply reports `Updated` once. + - Dependencies: T01 + - Done when: All three templates pass `sh -n`; each contains exactly one marker pair; no template invokes `exec`; a hook installed fresh into an empty hooks directory still blocks nothing when `sce` is absent and still forwards arguments and exit status when it is present. + - Verification notes (commands or checks): `sh -n cli/assets/hooks/pre-commit && sh -n cli/assets/hooks/commit-msg && sh -n cli/assets/hooks/post-commit`; `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`; manual run of an installed `post-commit` with and without `sce` on `PATH`. + - Implementation evidence: `cli/assets/hooks/pre-commit`, `cli/assets/hooks/commit-msg`, `cli/assets/hooks/post-commit` — wrapped the missing-CLI guidance plus the `sce hooks ` invocation in `# >>> sce managed block (do not edit) >>>` / `# <<< sce managed block <<<`; replaced each `exec sce hooks ...` with an explicit invocation followed by `status=$?; exit "$status"`; in `post-commit`, moved the `remote_url="$(git remote get-url origin ...)"` computation inside the managed block so it travels with the block when a future foreign-hook append embeds only the block; shebang, `set -eu`, branded guidance text, terminal-only ANSI policy, and `post-commit`'s `--remote-url` behavior are unchanged outside these edits. + - Verification outcome: `sh -n` on all three templates — clean. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 38 passed, 0 failed. Manual check: each file contains exactly 2 marker-comment lines (one pair) and zero `exec` invocations. Manually ran an installed `post-commit` and `pre-commit` in a scratch git repo with `sce` absent from `PATH` (exit 0, guidance printed to stderr) and with a fake `sce` present (arguments forwarded correctly, non-zero exit status propagated). + - Deviations/assumptions: Placed `remote_url` computation inside the managed block rather than before it, as an ordinary local implementation choice needed so the `--remote-url` behavior is preserved when T04 later appends only the block (not the whole file) to a foreign `post-commit`; no other deviation from the reviewed scope. + +- [x] T03: `Add pure hook managed-block merge module` (status:done) + - Task ID: T03 + - Goal: A filesystem-free module computes the bytes to install for a hook from the existing file's bytes and the canonical template, mirroring `config_merge.rs`. + - Boundaries (in/out of scope): In — new `cli/src/services/setup/hook_merge.rs` exposing the marker constants, the legacy-ownership marker, a `merge_or_create_hook(existing: Option<&[u8]>, canonical: &[u8]) -> Result` returning the merged bytes plus a classification (created / managed-block replaced / appended to foreign hook / already current) and an unreachable-block advisory flag, plus the last-effective-line heuristic (last non-blank, non-comment line at zero indentation starting with `exec ` or `exit`), and its unit tests. Out — install and doctor wiring; no filesystem access anywhere in the module. + - Dependencies: T02 + - Done when: The module is pure and its tests use no temp directories; unit tests cover create-from-absent, replace-in-place preserving surrounding foreign content, append-to-foreign preserving the original bytes as an exact prefix, legacy pre-marker payload replaced wholesale, idempotence across two merges producing identical bytes, a foreign hook with unbalanced or partial markers failing with a deterministic error naming the hook, and the advisory firing on a trailing zero-indent `exec`/`exit` but not on an indented one or on an ordinary final command. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::hook_merge`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets`. + - Implementation evidence: `cli/src/services/setup/hook_merge.rs` (new) — `MANAGED_BLOCK_START`/`MANAGED_BLOCK_END` marker constants; `HookMergeKind` (`Created`/`ManagedBlockReplaced`/`AppendedToForeign`/`AlreadyCurrent`) and `HookMerge { bytes, kind, unreachable_block_advisory }`; `merge_or_create_hook(existing, canonical, hook_name)` locating the marker pair by exact-line match, splicing the canonical block in place when the existing block differs, replacing wholesale when the legacy guidance-URL marker is found with no block, appending the canonical block after the existing bytes (kept as an exact prefix) otherwise, and failing with a `hook_name`-naming error on an unbalanced/partial marker pair; `ends_with_unreachable_control_flow` implementing the last-effective-line heuristic. `cli/src/services/setup/mod.rs` — added `pub(crate) mod hook_merge;` alongside `config_merge`. + - Verification outcome: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::hook_merge` — 13 passed, 0 failed. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 51 passed, 0 failed (no regression from the new module or its registration). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` — clean, no warnings. + - Deviations/assumptions: Added a `hook_name: &str` third parameter to `merge_or_create_hook`, beyond the two-argument signature sketched in Boundaries, because the done check requires the unbalanced-marker error to name the offending hook and the module has no other source for that name; mirrors `config_merge.rs`'s own `source_path` parameter on `merge_or_create_claude_settings`/`merge_or_create_opencode_config`, so it stays in convention. `merge_or_create_hook`, `HookMerge`, and `HookMergeKind` are marked `#[allow(dead_code)]` since T04 is the first caller, matching the existing forward-declared-item precedent in this file (`RequiredHookAsset`, `get_required_hook_asset`). No other deviation from the reviewed scope. + +- [x] T04: `Install required hooks through the managed-block merge` (status:done) + - Task ID: T04 + - Goal: `sce setup --hooks` preserves a foreign hook and appends the SCE block, keeps `Installed`/`Updated`/`Skipped` accurate against the block rather than the whole file, and surfaces the unreachable-block advisory. + - Boundaries (in/out of scope): In — `install_single_required_hook_with_rename` reads the existing hook, computes bytes via `hook_merge`, and stages those instead of `hook_asset.bytes`; the skip decision compares merged bytes plus executable bit against the file on disk; the advisory is carried on `RequiredHookInstallResult` and rendered in the hook section of setup output; integration tests for foreign-hook append, idempotent rerun, legacy upgrade, and the advisory. Out — doctor inspection, which still compares bytes and will report drift until T05. + - Dependencies: T03 + - Done when: A foreign `pre-commit` survives as an exact prefix with the block appended and the file executable; a second run reports `Skipped` with identical bytes for both foreign-plus-block and block-only shapes; a legacy pre-marker SCE hook upgrades to the marker form; a foreign hook ending in a zero-indent `exec` installs and reports the advisory; existing hooks-path resolution, write-permission probes, recovery guidance, and no-backup behavior are unchanged. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::`; manual `sce setup --hooks` in a temp checkout seeded with a husky-style `pre-commit`, then `git commit` to confirm both the foreign hook and the SCE block run. + - Implementation evidence: `cli/src/services/setup/mod.rs` — the install/skip wiring (`install_single_required_hook_with_rename` reading the existing hook, calling `hook_merge::merge_or_create_hook`, staging `merge.bytes`, comparing merged bytes plus the executable bit for the skip decision, and carrying `unreachable_block_advisory` on `RequiredHookInstallResult` with its advisory line in `format_required_hook_install_success_message`) was already present on this branch from prior work on this task stack; this task's remaining scope was the missing integration coverage, added as four new tests in `cli/src/services/setup/mod.rs`'s `tests` module: `foreign_pre_commit_hook_keeps_its_content_and_gains_the_sce_block` (foreign hook survives as an exact prefix, block appended, file executable, `Updated`, no advisory), `rerunning_hook_install_is_idempotent_for_block_only_and_foreign_plus_block_shapes` (second run `Skipped` with identical bytes for both the block-only `pre-commit` and a foreign-plus-block `commit-msg`), `legacy_pre_marker_hook_upgrades_to_the_managed_block_form` (a pre-marker guidance-URL hook upgrades to the exact canonical bytes, stays executable), and `foreign_hook_ending_in_exec_installs_the_block_and_reports_the_advisory` (a foreign `pre-commit` ending in `exec` installs the block and sets the advisory, while a sibling foreign `commit-msg` ending in an ordinary command does not). + - Verification outcome: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 55 passed, 0 failed (51 pre-existing plus the 4 new tests). `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` — clean, no warnings. + - Deviations/assumptions: The install/skip wiring itself was not written by this task — it was found already implemented and uncommitted on the branch when this task's review began, matching the Boundaries description exactly (merge-based staging, executable-bit-aware skip comparison, advisory field and rendering). This task's actual work was limited to closing the test gap the Done-when checks require; no production code was changed. The manual `sce setup --hooks` / `git commit` check in Verification notes was not run in this session — the four automated integration tests directly assert the same properties (foreign-prefix preservation, executable bit, idempotent rerun, legacy upgrade, advisory) that the manual check exists to spot-confirm. + +- [x] T05: `Inspect hook content by SCE managed block currency` (status:done) + - Task ID: T05 + - Goal: `sce doctor` decides a hook's content state from its SCE block rather than whole-file bytes, so a hook a repository has extended reports `[PASS]`. + - Boundaries (in/out of scope): In — `inspect_hook_content_state` and `inspect_hook_content_state_without_problem` in `cli/src/services/doctor/inspect.rs` compare via a `hook_merge` currency predicate (merging canonical into the file is a no-op) instead of `bytes == expected_hook.bytes`; `HookReadFailed` handling, remediation text, and the `Missing`/`Current`/`Stale`/`Unknown` vocabulary are preserved; doctor tests for current-with-foreign-content, drifted block, and `--fix` repair. Out — new problem kinds, new status vocabulary, and any change to `--fix` plumbing, which already reuses canonical setup hook installation. + - Dependencies: T04 + - Done when: A hook with foreign content plus a current block reports `Current`; deleting or corrupting the block reports `Stale`; `sce doctor --fix` restores the block and a follow-up inspection reports `Current` with the foreign content intact; no doctor status string, section order, or problem taxonomy changes. + - Verification notes (commands or checks): `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::`; manual `sce doctor` / `sce doctor --fix` in a temp checkout with an extended `pre-commit`. + - Implementation evidence: `cli/src/services/doctor/inspect.rs` — `inspect_hook_content_state_without_problem` and `inspect_hook_content_state` (the latter unreachable dead code, since its only caller `inspect_repository_hooks` has no callers) now classify hook content via `hook_merge::merge_or_create_hook(Some(bytes), canonical, hook_name)`: `Current` when merging is a no-op (`merge.bytes == bytes`), `Stale` otherwise, including an unbalanced/partial managed block (previously `Unknown`, now `Stale` so `--fix` repairs it); `HookReadFailed` handling and the `Missing`/`Unknown` cases for a read failure or unrecognized hook name are unchanged. Added a shared private helper `hook_managed_block_content_state` used by both functions. Added three filesystem-backed tests: `hook_with_foreign_content_and_current_block_reports_current`, `hook_with_drifted_managed_block_reports_stale`, `fix_repairs_drifted_hook_content_while_preserving_foreign_content` (drifts a foreign-plus-block hook, reinstalls via `install_required_git_hooks`, and asserts the foreign prefix survives and the state returns to `Current`). `cli/src/services/hooks/lifecycle.rs` — `inspect_hook_content_state` (the function that actually feeds the live `HookContentStale` problem and `--fix` eligibility, found during investigation to be a separate near-duplicate of the doctor/inspect.rs function of the same name) updated with the identical currency-based classification, since the acceptance criteria do not hold end-to-end without this change. + - Verification outcome: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` — 9 passed, 0 failed. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::` — 55 passed, 0 failed (no regression). `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` (full suite) — 230 passed, 0 failed. `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` — clean, no warnings. + - Deviations/assumptions: Updated `cli/src/services/hooks/lifecycle.rs::inspect_hook_content_state` beyond the file named in Boundaries, because that function — not the one in `doctor/inspect.rs` — is the one reachable from `sce doctor`'s actual problem list and `--fix` eligibility; the `doctor/inspect.rs` function of the same name is dead code. Without this, AC6's `--fix` repair and stale-detection behavior would not hold in the live CLI path. No other deviation from the reviewed scope; the manual `sce doctor` / `sce doctor --fix` check in Verification notes was not run in this session — the three automated tests directly assert the same current/stale/repair properties it exists to spot-confirm. + +## Open questions + +- The appended block is cooperative, not authoritative. husky and lefthook rewrite their own hooks on `npm install` or `lefthook install`, which drops the SCE block silently — SCE stops running until the next `sce setup --hooks`, and nothing tells the user. The predecessor plan reached the same conclusion and proposed a `.d/` dispatcher instead, which loses the block just as easily. If silent breakage is the concern, the fix is `sce doctor` catching it, which T05 gives you for free; if it is not a concern, this note can be dropped. +- The unreachable-block advisory (AC7, T03's heuristic) exists because appending after a foreign hook that ends in `exec` or `exit` produces a block that never runs, which is otherwise invisible. It is deliberately narrow — last effective line, zero indentation — so it will miss an early `exit 0` inside a conditional. A heuristic that catches some cases and not others may be worse than none; say if you would rather T03 and AC7 be dropped. +- `context/plans/non-destructive-setup-install.md` still records `**Status:** failed` with a `Retry` instruction, from a `cargo fmt --check` failure that no longer reproduces (`cargo fmt --manifest-path cli/Cargo.toml -- --check` exits 0 on the current tree). That plan covers the code T01 modifies, so its report will stay misleading unless someone reruns `/validate` on it. Explicitly out of scope here. + +## Validation Report + +**Status:** validated +**Date:** 2026-08-07 + +### Commands run + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` -> exit 0 (230 passed, 0 failed) +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets` -> exit 0 (clean, no warnings) +- `nix run .#pkl-check-generated` -> exit 0 (ephemeral Pkl generation passed: 101 files) +- `nix flake check` -> exit 0 (all checks passed, including `checks.x86_64-linux.cli-fmt`; previously failed because `cli/src/services/setup/hook_merge.rs` was untracked by git, now staged and visible to the Nix-sandboxed build) +- `sh -n cli/assets/hooks/pre-commit && sh -n cli/assets/hooks/commit-msg && sh -n cli/assets/hooks/post-commit` -> exit 0 (no syntax errors) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml setup::hook_merge` -> exit 0 (13 passed, 0 failed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml doctor::` -> exit 0 (9 passed, 0 failed) + +### Scaffolding removed + +- None. + +### Success-criteria verification + +- [x] AC1: Rename failure preserves prior config-asset content and leaves no staging artifact -> `hook_install_leaves_prior_hook_intact_on_rename_failure` and the updated asset rename-failure test pass in the `setup::` suite. +- [x] AC2: Rename failure preserves prior hook content and executable bit -> `hook_install_leaves_prior_hook_intact_on_rename_failure` passes. +- [x] AC3: Foreign `pre-commit` preserved as exact prefix with block appended, executable -> `foreign_pre_commit_hook_keeps_its_content_and_gains_the_sce_block` passes. +- [x] AC4: Idempotent rerun reports `Skipped` with identical bytes for both shapes -> `rerunning_hook_install_is_idempotent_for_block_only_and_foreign_plus_block_shapes` passes. +- [x] AC5: Legacy pre-marker payload replaced wholesale with marker form -> `legacy_pre_marker_hook_upgrades_to_the_managed_block_form` (integration) and `legacy_pre_marker_payload_is_replaced_wholesale` (`setup::hook_merge` unit test) pass. +- [x] AC6: `sce doctor` reports current/stale by block currency and `--fix` repairs while preserving foreign content -> `hook_with_foreign_content_and_current_block_reports_current`, `hook_with_drifted_managed_block_reports_stale`, and `fix_repairs_drifted_hook_content_while_preserving_foreign_content` pass in `doctor::`. +- [x] AC7: Unreachable-block advisory fires only on trailing zero-indent `exec`/`exit` -> `foreign_hook_ending_in_exec_installs_the_block_and_reports_the_advisory` (integration) and the `advisory_*` unit tests in `setup::hook_merge` pass. + +### Failed checks and follow-ups + +- None. + +### Residual risks + +- None identified. diff --git a/context/sce/doctor-human-text-contract.md b/context/sce/doctor-human-text-contract.md index 15699eed..ccaaf001 100644 --- a/context/sce/doctor-human-text-contract.md +++ b/context/sce/doctor-human-text-contract.md @@ -71,6 +71,8 @@ Integration checks for this contract inspect installed repo-root artifacts only. They validate file presence and content against embedded OpenCode, Claude, and Pi setup assets: byte-exact `sha256` for every asset except the two JSON configs `sce setup` installs by merge (`.claude/settings.json`, `.opencode/opencode.json`), which instead validate that the file's SCE-owned fragment matches the embedded catalog — a file that also carries extra user keys, permissions, or plugins still renders `[PASS]` as long as that fragment is current (see [non-destructive setup install merge seam](setup-no-backup-policy-seam.md)). Generated `config/.opencode/**`, `config/.claude/**`, and `config/.pi/**` trees are out of scope for doctor integration checks in this change stream. +Required git hooks (`Git Hooks` section) are a third merge-target family with the same fragment-currency rule: a hook is `[PASS]` when merging the canonical template into its on-disk bytes is a no-op, whether or not foreign content (a hand-written hook, husky, lefthook) sits around the SCE managed block — not byte-exact equality against the canonical hook (see [git hooks install contract](setup-githooks-install-contract.md)). + Claude installed assets are grouped by repo-root `.claude/` relative path: - `settings.json` and `hooks/**` -> `ClaudeCode plugins` (including `hooks/run-sce-or-show-install-guidance.sh`) diff --git a/context/sce/setup-githooks-hook-asset-packaging.md b/context/sce/setup-githooks-hook-asset-packaging.md index c9b1056d..8a5a8e3e 100644 --- a/context/sce/setup-githooks-hook-asset-packaging.md +++ b/context/sce/setup-githooks-hook-asset-packaging.md @@ -14,13 +14,13 @@ Task `sce-setup-githooks-any-repo` `T02` defines how required git-hook templates The build script copies these templates to `OUT_DIR/static/hooks/`, then emits them into `OUT_DIR/setup_embedded_assets.rs` as `HOOK_EMBEDDED_ASSETS` with deterministic sorted relative paths. Production Rust includes therefore resolve through `OUT_DIR` rather than back into `cli/assets/`. -All three templates are POSIX `sh` scripts with `set -eu`. Before invoking `sce`, each checks `command -v sce`; when the CLI is unavailable, it prints branded, multiline installation guidance to stderr and exits successfully so Git operations are not blocked solely by a missing local CLI installation. ANSI styling is emitted only when stderr is a terminal; redirected output remains plain text. Failures from an available `sce` command continue to propagate through `exec`. +All three templates are POSIX `sh` scripts with `set -eu`. The CLI-presence check and the `sce hooks` invocation are wrapped in an SCE managed block delimited by `# >>> sce managed block (do not edit) >>>` / `# <<< sce managed block <<<` comment markers, so the same block content can later be embedded inside a foreign hook without disturbing content around it. Before invoking `sce`, each checks `command -v sce`; when the CLI is unavailable, it prints branded, multiline installation guidance to stderr and exits successfully so Git operations are not blocked solely by a missing local CLI installation. ANSI styling is emitted only when stderr is a terminal; redirected output remains plain text. Failures from an available `sce` command propagate by capturing `$?` and calling `exit` explicitly, not by `exec`, so the block terminates the script deterministically even when it is not the file's only content. Available-CLI behavior remains hook-specific: - `pre-commit` invokes `sce hooks pre-commit "$@"`. - `commit-msg` invokes `sce hooks commit-msg "$@"`. -- `post-commit` resolves `origin` with `git remote get-url origin`; when the lookup returns a non-empty URL, it invokes `sce hooks post-commit --vcs git --remote-url "$remote_url" "$@"`, otherwise it invokes `sce hooks post-commit --vcs git "$@"`. Remote metadata forwarding is exclusive to `post-commit`. +- `post-commit` resolves `origin` with `git remote get-url origin` inside the managed block, after the CLI-presence check; when the lookup returns a non-empty URL, it invokes `sce hooks post-commit --vcs git --remote-url "$remote_url" "$@"`, otherwise it invokes `sce hooks post-commit --vcs git "$@"`. Remote metadata forwarding is exclusive to `post-commit`, and computing it inside the block keeps that behavior intact wherever the block is embedded. ## Setup-service accessor surface diff --git a/context/sce/setup-githooks-install-contract.md b/context/sce/setup-githooks-install-contract.md index b58cbe2d..b4aa81f1 100644 --- a/context/sce/setup-githooks-install-contract.md +++ b/context/sce/setup-githooks-install-contract.md @@ -9,7 +9,7 @@ In scope for this contract: - target repository and hooks-path resolution policy - required hook ownership and idempotent update rules -- remove-and-replace replacement flow for all repositories +- atomic-swap replacement flow for all repositories - deterministic outcome vocabulary and failure diagnostics - `sce doctor` readiness alignment after successful install @@ -18,6 +18,8 @@ Out of scope for this contract task: - runtime implementation details of file writes - CLI parser wiring and final flag surface implementation +See [ADR: SCE git hooks are a bounded in-place editor, not an exclusive owner](../decisions/2026-08-07-git-hook-managed-block-cooperation.md) for why hook ownership is decided structurally and foreign hooks are preserved rather than overwritten. + ## Command surface contract - Canonical operator command: `sce setup --hooks` @@ -53,21 +55,23 @@ Install behavior must write required hooks into the effective hooks directory re ## Hook ownership and idempotency rules -Each required hook has one canonical SCE-managed payload. +Each required hook carries its SCE-owned logic inside a stable managed-block marker pair. A hook is SCE-owned within that block; a hook predating the markers is recognized wholesale by a legacy guidance-URL marker; any other hook is foreign, and setup preserves it rather than overwriting it. -Per hook, setup reports exactly one deterministic outcome: +Per hook, setup reports exactly one deterministic outcome, decided against the merged (not verbatim canonical) content: - `installed`: hook was missing and is now present -- `updated`: hook existed and was replaced with newer canonical content -- `skipped`: hook already matched canonical content +- `updated`: hook existed and the merged content and/or executable bit did not already match the file on disk — this covers a stale-block replacement, a legacy-hook upgrade, and appending the block to a foreign hook alike +- `skipped`: hook already matched the merged content and executable state — including a foreign hook that already carries the current SCE block -Re-running setup with unchanged canonical assets must be idempotent and produce `skipped` for all already-synced hooks. +Re-running setup with unchanged canonical assets must be idempotent and produce `skipped` for all already-synced hooks, whether or not they carry foreign content above the SCE block. ## Preservation and replacement policy -When setup needs to replace an existing hook file, it performs replacement through a staged write/swap flow and preserves executable permissions required by git hooks. +When setup needs to write an existing hook file, it first computes the bytes to write: a foreign hook (no managed block, no legacy marker) is kept as an exact byte prefix with the canonical SCE block appended after it; an SCE-owned hook has only its block replaced or brought current, leaving any content around the block untouched. Setup then performs that write through a staged write/swap flow and preserves executable permissions required by git hooks. + +Setup stages the computed content, then swaps it into place by atomic rename over the existing hook; the destination is never unlinked before the rename, so a hook is never briefly absent during replacement. No installer-managed backup artifacts are created. Recovery from a failed swap relies on version control state rather than installer-created backups. -Setup removes the existing hook directly before swapping staged content. No installer-managed backup artifacts are created. Recovery from a failed swap relies on version control state rather than installer-created backups. +When appending the SCE block to a foreign hook whose last effective line is a zero-indent `exec` or `exit`, the block is still installed but setup surfaces a deterministic advisory naming that hook, because the appended block would not run as-is. ## Rollback guarantees @@ -105,5 +109,5 @@ T02-T05 implementation and tests must verify this contract across: - fresh install in empty hook directories - rerun idempotency with unchanged assets - upgrade path from older/non-canonical hook content -- remove-and-replace behavior under injected replacement failures +- atomic-swap replacement behavior under injected replacement failures - post-setup `sce doctor` readiness \ No newline at end of file diff --git a/context/sce/setup-githooks-install-flow.md b/context/sce/setup-githooks-install-flow.md index fbea388d..ff1cbcd9 100644 --- a/context/sce/setup-githooks-install-flow.md +++ b/context/sce/setup-githooks-install-flow.md @@ -36,24 +36,32 @@ This keeps behavior compatible with: ## Per-hook installation contract -The flow iterates canonical embedded required hooks (`pre-commit`, `commit-msg`, `post-commit`) and applies deterministic per-hook outcomes: +The flow iterates canonical embedded required hooks (`pre-commit`, `commit-msg`, `post-commit`) and, for each, computes the bytes to stage with `cli/src/services/setup/hook_merge.rs::merge_or_create_hook(existing_bytes, canonical_bytes, hook_name)` rather than writing the canonical asset's bytes verbatim — the same content-computation-before-swap seam the two JSON merge targets use (see [setup-no-backup-policy-seam.md](setup-no-backup-policy-seam.md)). No hook existing yields the canonical template unchanged; a hook already carrying the current SCE managed block is left unchanged; a hook carrying a stale block gets the block spliced in place, its surrounding content untouched; a marker-free hook containing the legacy pre-marker guidance URL is recognized as SCE-owned wholesale and replaced entirely with canonical bytes; any other marker-free hook is foreign and is kept as an exact byte prefix with the canonical block appended after it. Deterministic per-hook outcomes are then reported against the merged result: - `Installed`: hook was absent and is now present. -- `Updated`: hook existed but content and/or executable bit did not match canonical state. -- `Skipped`: hook already matched canonical bytes and executable state. +- `Updated`: hook existed but the merged bytes and/or executable bit did not match the file on disk (covers a foreign-hook append, a stale-block replacement, and a legacy-hook upgrade alike). +- `Skipped`: the file on disk already equals the merged bytes and is executable — including a foreign hook that already carries the current SCE block, not only a hook that is untouched foreign-content-free canonical output. -The installed bytes include the shared non-blocking missing-CLI bootstrap: all three hooks warn to stderr and exit successfully when `sce` is unavailable, while an available `sce` receives unchanged hook arguments and its failures propagate. Only `post-commit` performs origin lookup and remote metadata forwarding; see [setup-githooks-hook-asset-packaging.md](setup-githooks-hook-asset-packaging.md) for the canonical payload contract. +When a foreign hook is appended to and its last effective line is a zero-indent `exec`/`exit`, so the appended block would never run, `RequiredHookInstallResult.unreachable_block_advisory` is set and rendered as a named advisory line in the hook section of setup output; every other outcome leaves it `false`. -## Staged write and remove-and-replace behavior +The canonical bytes merged into each hook include the shared non-blocking missing-CLI bootstrap: all three hooks warn to stderr and exit successfully when `sce` is unavailable, while an available `sce` receives unchanged hook arguments and its failures propagate. Only `post-commit` performs origin lookup and remote metadata forwarding; see [setup-githooks-hook-asset-packaging.md](setup-githooks-hook-asset-packaging.md) for the canonical payload contract. -When replacing an existing hook, setup always writes canonical bytes to a unique staging file in the hooks directory, enforces executable permissions on the staged payload, removes the existing hook directly, and swaps the staged content into the final hook path. +## Staged write and atomic-swap behavior -On swap failure, setup removes the staging artifact and returns deterministic recovery guidance (recover the hook from version control if needed). No backup artifacts are created and no backup-based rollback is attempted. +When installing or replacing a hook, setup always writes the merged bytes to a unique staging file in the hooks directory and enforces executable permissions on the staged payload. It never unlinks an existing hook before swapping: the staged file is renamed directly over the final hook path, so `fs::rename` performs the replacement atomically and a hook is never briefly absent mid-install. This mirrors the config-asset swap in [setup-no-backup-policy-seam.md](setup-no-backup-policy-seam.md). + +On swap failure, setup removes the staging artifact and returns deterministic recovery guidance (recover the hook from version control if needed) whenever a prior hook existed. No backup artifacts are created and no backup-based rollback is attempted; a rename failure leaves the pre-existing hook's bytes and executable bit untouched because the old file was never removed. ## Verification coverage -`cli/src/services/setup/mod.rs` includes T03-focused tests for: +`cli/src/services/setup/mod.rs` includes tests for: - hook update in the default hooks directory with no backup artifact creation - hook update in custom `core.hooksPath` with no backup artifact creation -- injected swap failure with staging cleanup and deterministic recovery guidance \ No newline at end of file +- injected swap failure with staging cleanup, deterministic recovery guidance, and the prior hook's bytes and executable bit surviving the failure +- a foreign hook's content surviving as an exact byte prefix with the SCE block appended and the file executable +- idempotent reruns reporting `Skipped` with unchanged bytes for both a block-only hook and a foreign-plus-block hook +- a legacy pre-marker SCE hook upgrading to the current canonical marker form +- a foreign hook ending in a zero-indent `exec` installing the block and reporting `unreachable_block_advisory`, with a sibling foreign hook ending in an ordinary command not reporting it + +`cli/src/services/setup/hook_merge.rs` unit-tests the pure merge computation itself (filesystem-free); the tests above verify the install-time wiring around it. \ No newline at end of file diff --git a/context/sce/setup-no-backup-policy-seam.md b/context/sce/setup-no-backup-policy-seam.md index 13bcba47..b8c40c24 100644 --- a/context/sce/setup-no-backup-policy-seam.md +++ b/context/sce/setup-no-backup-policy-seam.md @@ -1,28 +1,28 @@ # Setup non-destructive per-asset install policy -`cli/src/services/setup/mod.rs` installs every setup-managed file at file granularity: stage, remove the exact destination file if one exists, then swap the staged content into place. There is no backup creation or backup-based rollback, and setup-managed installs never remove an integration target directory as a whole. This per-file stage/swap choreography is shared by config install, required-hook install, and merge-target install; it is the JSON merge targets described below whose staged *content* differs from the embedded asset's bytes. +`cli/src/services/setup/mod.rs` installs every setup-managed file at file granularity: stage the new content next to its destination, then swap it into place by renaming the staging file directly over the destination. The destination is never unlinked first — `fs::rename` replaces an existing file atomically on both Unix and Windows, so a file is never briefly absent mid-install and a rename failure leaves the prior destination content untouched. There is no backup creation or backup-based rollback, and setup-managed installs never remove an integration target directory as a whole. This per-file stage/atomic-swap choreography is shared by config install, required-hook install, and merge-target install; it is the JSON merge targets described below whose staged *content* differs from the embedded asset's bytes. ## Current state - Config install (`.opencode`/`.claude`/`.pi`, `install_embedded_setup_assets` / `install_assets_for_concrete_target_with_rename`) writes every embedded asset to its own path under the target directory, creating parent directories as needed: 1. Write the asset's canonical content to a unique staging file next to its final destination. - 2. If a file already exists at that exact destination path, remove only that file. If a directory exists there instead, fail with an actionable error instead of deleting it. - 3. Swap the staged content into the final destination. - 4. On swap failure, clean the staging artifact and return deterministic recovery guidance naming that asset's destination path (recover from version control if needed). + 2. If a directory exists at that exact destination path, fail with an actionable error instead of deleting it. + 3. Rename the staging file directly over the final destination, replacing any existing file there atomically. + 4. On swap failure, clean the staging artifact and return deterministic recovery guidance naming that asset's destination path (recover from version control if needed); the pre-existing destination content, if any, is untouched because it was never removed. - Setup never removes an integration target directory (`.opencode`, `.claude`, `.pi`) as a whole, and never touches a path it did not author. Files a repository placed inside an SCE-owned target directory — at the top level or nested inside an SCE-owned subdirectory such as `skills/` or `commands/` — survive a setup run untouched. -- Required hook install (`install_required_git_hooks`) uses the same per-file stage/remove-if-present/swap choreography for each hook file; this predates and is unaffected by the config-install change above. +- Required hook install (`install_required_git_hooks`) uses the same per-file stage/atomic-swap choreography for each hook file, and — like the two JSON merge targets below — is itself a content-computation seam ahead of that shared swap: `install_single_required_hook_with_rename` computes the bytes to stage with `cli/src/services/setup/hook_merge.rs::merge_or_create_hook`, which preserves a foreign hook's bytes as an exact prefix and appends the canonical SCE managed block after them, rather than always writing the canonical asset verbatim (see [setup-githooks-install-flow.md](setup-githooks-install-flow.md)). - After the per-asset install loop, config install prunes stale SCE-owned paths: `prune_stale_assets_for_concrete_target` diffs the full embedded-asset catalog for the concrete target against the assets this run actually installed, and deletes every catalog path present in the former but not the latter (deselected optional-workflow files, or an asset a newer catalog renamed or dropped). Each successful deletion is followed by `remove_empty_ancestor_directories`, which removes now-empty parent directories upward until it reaches the target root or hits a directory that still holds something (a directory holding a user file fails to remove and is left in place, so a user file nested inside an SCE-owned skill directory survives even though the SCE file next to it is pruned). Pruning is stateless and catalog-derived — no install manifest is persisted — so it only ever considers paths the compiled-in catalog still names. - No `.backup` artifacts are created during any setup write flow, and no backup-based rollback is attempted on swap failure. - Recovery guidance is generic (not git-specific wording): "Setup ... does not create backups. Recover '' from version control if needed." - Two config assets are merge targets instead of verbatim-content assets: `.claude/settings.json` for the Claude target, and `.opencode/opencode.json` for the OpenCode target. `install_single_asset_with_rename` detects each (`is_claude_settings_merge_target`, `is_opencode_config_merge_target`) and, before staging, computes the bytes to stage from `cli/src/services/setup/config_merge.rs` rather than writing the embedded asset's bytes directly. Both merge functions return the generated document verbatim when no existing file is present; otherwise each parses the existing file as JSON (a parse failure is a hard error naming the file's path, and nothing is written) and merges the generated document into it, preserving every other top-level key untouched: - `merge_or_create_claude_settings`: `$schema` and, event-by-event, every hook entry whose command contains the marker `run-sce-or-show-install-guidance.sh` are SCE-owned and replaced from the generated document; every hook entry or event key the generated document does not declare is preserved untouched. - `merge_or_create_opencode_config`: `$schema` is SCE-owned and replaced from the generated document; the `plugin` array is merged as a set — any existing entry whose path starts with `./plugins/sce-` is dropped (structural ownership, so a plugin path an older or renamed catalog once installed is still recognized and pruned even after the current generated document stops declaring it), then the generated document's `plugin` entries are appended. - The merged bytes then flow through the same stage/remove-if-present/swap choreography as every other asset, so this is a content-computation seam layered on the shared install policy, not a different write path. + The merged bytes then flow through the same stage/atomic-swap choreography as every other asset, so this is a content-computation seam layered on the shared install policy, not a different write path. - `sce doctor --fix` reuses this same per-asset install path for the two merge targets rather than running its own repair logic: `crate::services::setup::repair_merge_target_asset` looks up the one embedded asset by relative path and reinstalls only it through `install_single_asset_with_rename`, so a drifted `.claude/settings.json` or `.opencode/opencode.json` is repaired by merge — every other installed asset and every user key is left untouched. `sce doctor` (diagnose or fix) tells a merge target's drift apart from a legitimately extended file by SCE-fragment equality (`config_merge::claude_settings_fragment_is_current`, `config_merge::opencode_config_fragment_is_current`) instead of the byte-exact `sha256` check every other integration asset uses (see [doctor human text contract](doctor-human-text-contract.md)). ## Scope boundary - This file captures the non-destructive, per-file install policy shared by config-install and required-hook install flows, including the merge-target content-computation seam for `.claude/settings.json`. -- Future setup-managed write flows should follow the same per-file stage/remove-if-present/swap pattern instead of introducing backup creation or whole-directory replacement. A future merge target computes its staged content the same way `.claude/settings.json` does, ahead of the shared stage/swap step. +- Future setup-managed write flows should follow the same per-file stage/atomic-swap pattern instead of introducing backup creation or whole-directory replacement. A future merge target computes its staged content the same way `.claude/settings.json` does, ahead of the shared stage/swap step. See also: [../overview.md](../overview.md), [../context-map.md](../context-map.md), [setup-githooks-install-flow.md](setup-githooks-install-flow.md)