From da77411e1b10a1fef5524b0f7d7e077613549e83 Mon Sep 17 00:00:00 2001 From: Kent Huang Date: Fri, 21 Aug 2026 20:21:42 +0800 Subject: [PATCH 1/2] Type split-root sync eligibility --- AGENTS.md | 6 +- README.md | 9 +- crates/spacetop-core/src/domain/mod.rs | 49 +++- crates/spacetop-core/src/state_checkout.rs | 223 ++++++++++++++++-- .../tests/state_checkout_fixtures.rs | 61 ++++- crates/spacetop/src/app/overview.rs | 72 +++++- crates/spacetop/src/lib.rs | 193 +++++++++++---- crates/spacetop/src/ui/tests/task_list.rs | 35 ++- docs/development-policy.md | 6 +- 9 files changed, 553 insertions(+), 101 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 84915be..0911f79 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,7 +114,8 @@ Keep module boundaries clear and testable: resolved dir to the active and archive scans. - `crates/spacetop-core/src/state_checkout.rs` owns the two-backend storage classifier and read-only Git probes for attached, detached, wrong-branch, - missing, and probe-failed split-root state checkouts. + missing, and typed-unverified split-root state checkouts, plus the explicit + distinct-root state-sync eligibility decision. - `crates/spacetop-core/src/index.rs`, `query.rs`, and `sources.rs` own the v2 index/query spine; TUI code must consume `WorkflowIndex` through query methods instead of inferring schema rules from raw vectors. @@ -125,7 +126,8 @@ Keep module boundaries clear and testable: - `crates/spacetop-core/src/git_sync.rs` owns the explicit read-refresh sync helper and must remain limited to audited fast-forward pulls. Top-level sync may call it for the definition repository and only for a verified attached - split-root state checkout. + split-root state checkout whose canonical Git top differs from the definition + sync root. - `crates/spacetop-core/src/session_activity.rs` is the local agent-session facade; its `session_activity/{projection,state,codex,claude,reducer}.rs` modules project privacy-safe typed facts, retain coherent scan evidence, diff --git a/README.md b/README.md index d50b3cc..e9c95b1 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,11 @@ remain on the definition directory. Absolute paths and paths with `..` are unsupported and fail closed to single-root. A relative path whose canonical target escapes the definition directory is unverified: available entities stay readable, but Spacetop will not run Git sync operations against that target. +Materialize the checkout at the contained path named by `state:` to make it +eligible for verification. Spacetop will not move, relink, or repair a state +checkout. Even an attached checkout is sync-eligible only when its canonical +Git top is distinct from the definition repository, so `Y` never pulls the same +checkout twice. A split-root checkout then has a separate runtime disposition. Attached means it holds the expected `state-branch:` (or the default @@ -60,8 +65,8 @@ it holds the expected `state-branch:` (or the default wrong-branch checkouts remain fully readable, but the footer warns that their snapshot may be stale or names the actual and expected branches. Missing state shows an empty list together with “State checkout missing; no state loaded.” A -Git probe failure shows “State topology unverified” instead of claiming the -workflow is healthy. +Git probe failure shows “State topology unverified” and explains that sync is +blocked instead of claiming the workflow is healthy. The product contract remains read-only by default: Spacedock markdown files are the source of truth, and state-changing features must be explicit and auditable. diff --git a/crates/spacetop-core/src/domain/mod.rs b/crates/spacetop-core/src/domain/mod.rs index b2a5d4e..a8cbf70 100644 --- a/crates/spacetop-core/src/domain/mod.rs +++ b/crates/spacetop-core/src/domain/mod.rs @@ -38,7 +38,54 @@ pub enum StateCheckoutDisposition { Detached, WrongBranch { actual_branch: String }, Missing, - ProbeFailed { reason: String }, + Unverified { problem: StateTopologyProblem }, +} + +/// A typed reason why a materialized split-root checkout could not be +/// verified. Variants preserve the policy boundary separately from incidental +/// filesystem and Git probe failures. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum StateTopologyProblem { + DefinitionPathResolution { path: PathBuf, error: String }, + StatePathResolution { path: PathBuf, error: String }, + OutsideDefinition { resolved_state: PathBuf }, + GitTopLevelProbe { error: String }, + EmptyGitTopLevel, + GitTopLevelResolution { path: PathBuf, error: String }, + CheckoutRootMismatch { actual_top: PathBuf }, + BranchProbe { error: String }, + EmptyBranch, +} + +/// Final authorization decision consumed by the explicit sync boundary. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum StateSyncEligibility { + NotApplicable, + Eligible { checkout_root: PathBuf }, + Blocked { problem: StateSyncProblem }, +} + +/// A typed reason why split-root state cannot receive a sync call. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum StateSyncProblem { + Detached, + WrongBranch { + actual_branch: String, + expected_branch: String, + }, + Missing, + Topology(StateTopologyProblem), + DefinitionRootResolution { + path: PathBuf, + error: String, + }, + StateRootResolution { + path: PathBuf, + error: String, + }, + SameAsDefinition { + checkout_root: PathBuf, + }, } /// A plain RGB color owned by the core (no terminal-crate dependency). diff --git a/crates/spacetop-core/src/state_checkout.rs b/crates/spacetop-core/src/state_checkout.rs index 9664ec3..92444a8 100644 --- a/crates/spacetop-core/src/state_checkout.rs +++ b/crates/spacetop-core/src/state_checkout.rs @@ -3,7 +3,10 @@ use std::fs; use std::path::{Component, Path, PathBuf}; -use crate::domain::{StateCheckoutDisposition, WorkflowStorage}; +use crate::domain::{ + StateCheckoutDisposition, StateSyncEligibility, StateSyncProblem, StateTopologyProblem, + WorkflowStorage, +}; use crate::git::GitRunner; /// Classify the README storage declaration and, for a supported split-root, @@ -74,42 +77,60 @@ fn probe_disposition( let definition_top = match fs::canonicalize(definition_dir) { Ok(path) => path, Err(error) => { - return probe_failed(format!( - "cannot resolve workflow definition directory: {error}" - )); + return unverified(StateTopologyProblem::DefinitionPathResolution { + path: definition_dir.to_path_buf(), + error: error.to_string(), + }); } }; let expected_top = match fs::canonicalize(entity_dir) { Ok(path) => path, - Err(error) => return probe_failed(format!("cannot resolve state directory: {error}")), + Err(error) => { + return unverified(StateTopologyProblem::StatePathResolution { + path: entity_dir.to_path_buf(), + error: error.to_string(), + }); + } }; if !expected_top.starts_with(&definition_top) { - return probe_failed(format!( - "state directory resolves outside workflow definition directory: {}", - expected_top.display() - )); + return unverified(StateTopologyProblem::OutsideDefinition { + resolved_state: expected_top, + }); } let top = match runner.run(entity_dir, &["rev-parse", "--show-toplevel"]) { Ok(result) if result.status.success() => result.stdout.trim().to_string(), - Ok(result) => return probe_failed(git_failure(&result.stderr, "not a Git checkout")), - Err(error) => return probe_failed(format!("Git probe failed: {error}")), + Ok(result) => { + return unverified(StateTopologyProblem::GitTopLevelProbe { + error: git_failure(&result.stderr, "not a Git checkout"), + }); + } + Err(error) => { + return unverified(StateTopologyProblem::GitTopLevelProbe { + error: error.to_string(), + }); + } }; if top.is_empty() { - return probe_failed("Git reported an empty checkout root".to_string()); + return unverified(StateTopologyProblem::EmptyGitTopLevel); } let actual_top = match fs::canonicalize(&top) { Ok(path) => path, - Err(error) => return probe_failed(format!("cannot resolve Git checkout root: {error}")), + Err(error) => { + return unverified(StateTopologyProblem::GitTopLevelResolution { + path: PathBuf::from(top), + error: error.to_string(), + }); + } }; if actual_top != expected_top { - return probe_failed("state directory belongs to a parent Git checkout".to_string()); + return unverified(StateTopologyProblem::CheckoutRootMismatch { actual_top }); } match runner.run(entity_dir, &["symbolic-ref", "--quiet", "--short", "HEAD"]) { Ok(result) if result.status.success() => { let actual_branch = result.stdout.trim().to_string(); if actual_branch.is_empty() { - probe_failed("Git reported an empty branch name".to_string()) + unverified(StateTopologyProblem::EmptyBranch) } else if actual_branch == expected_branch { StateCheckoutDisposition::Attached } else { @@ -117,8 +138,79 @@ fn probe_disposition( } } Ok(result) if result.status.code() == Some(1) => StateCheckoutDisposition::Detached, - Ok(result) => probe_failed(git_failure(&result.stderr, "branch probe failed")), - Err(error) => probe_failed(format!("Git branch probe failed: {error}")), + Ok(result) => unverified(StateTopologyProblem::BranchProbe { + error: git_failure(&result.stderr, "branch probe failed"), + }), + Err(error) => unverified(StateTopologyProblem::BranchProbe { + error: error.to_string(), + }), + } +} + +/// Decide whether a freshly classified state checkout may receive the one +/// audited fast-forward pull. Canonical root equality blocks a second pull +/// against the definition repository even if callers construct stale or +/// synthetic `Attached` state. +pub fn state_sync_eligibility( + definition_sync_root: &Path, + storage: &WorkflowStorage, +) -> StateSyncEligibility { + let WorkflowStorage::SplitRoot { + entity_dir, + expected_branch, + disposition, + } = storage + else { + return StateSyncEligibility::NotApplicable; + }; + + match disposition { + StateCheckoutDisposition::Attached => { + let definition_root = match fs::canonicalize(definition_sync_root) { + Ok(path) => path, + Err(error) => { + return StateSyncEligibility::Blocked { + problem: StateSyncProblem::DefinitionRootResolution { + path: definition_sync_root.to_path_buf(), + error: error.to_string(), + }, + }; + } + }; + let checkout_root = match fs::canonicalize(entity_dir) { + Ok(path) => path, + Err(error) => { + return StateSyncEligibility::Blocked { + problem: StateSyncProblem::StateRootResolution { + path: entity_dir.clone(), + error: error.to_string(), + }, + }; + } + }; + if checkout_root == definition_root { + StateSyncEligibility::Blocked { + problem: StateSyncProblem::SameAsDefinition { checkout_root }, + } + } else { + StateSyncEligibility::Eligible { checkout_root } + } + } + StateCheckoutDisposition::Detached => StateSyncEligibility::Blocked { + problem: StateSyncProblem::Detached, + }, + StateCheckoutDisposition::WrongBranch { actual_branch } => StateSyncEligibility::Blocked { + problem: StateSyncProblem::WrongBranch { + actual_branch: actual_branch.clone(), + expected_branch: expected_branch.clone(), + }, + }, + StateCheckoutDisposition::Missing => StateSyncEligibility::Blocked { + problem: StateSyncProblem::Missing, + }, + StateCheckoutDisposition::Unverified { problem } => StateSyncEligibility::Blocked { + problem: StateSyncProblem::Topology(problem.clone()), + }, } } @@ -131,8 +223,8 @@ fn git_failure(stderr: &str, fallback: &str) -> String { .to_string() } -fn probe_failed(reason: String) -> StateCheckoutDisposition { - StateCheckoutDisposition::ProbeFailed { reason } +fn unverified(problem: StateTopologyProblem) -> StateCheckoutDisposition { + StateCheckoutDisposition::Unverified { problem } } #[cfg(test)] @@ -234,8 +326,10 @@ mod tests { let failed = RecordingGitRunner::new(vec![err(128, "fatal: not a git repository\n")]); assert_eq!( probe_disposition(&failed, temp.path(), &state, "spacedock-state/demo"), - StateCheckoutDisposition::ProbeFailed { - reason: "fatal: not a git repository".to_string() + StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::GitTopLevelProbe { + error: "fatal: not a git repository".to_string() + } } ); } @@ -249,8 +343,8 @@ mod tests { let runner = RecordingGitRunner::new(vec![ok(&format!("{}\n", parent.display()))]); assert_eq!( probe_disposition(&runner, temp.path(), &state, "spacedock-state/demo"), - StateCheckoutDisposition::ProbeFailed { - reason: "state directory belongs to a parent Git checkout".to_string() + StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::CheckoutRootMismatch { actual_top: parent } } ); } @@ -276,13 +370,92 @@ mod tests { None ), WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::ProbeFailed { ref reason }, + disposition: StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::OutsideDefinition { ref resolved_state } + }, .. - } if reason.contains("resolves outside workflow definition directory") + } if resolved_state == &fs::canonicalize(&external).expect("canonical external") )); assert!( runner.calls().is_empty(), "escaped state path must be rejected before any Git probe" ); } + + #[cfg(unix)] + #[test] + fn canonical_definition_alias_keeps_contained_checkout_attached() { + use std::os::unix::fs::symlink; + + let temp = tempdir().expect("tempdir"); + let real_definition = temp.path().join("real-definition"); + let alias_definition = temp.path().join("definition-alias"); + let state = real_definition.join(".spacedock-state"); + fs::create_dir_all(&state).expect("state dir"); + symlink(&real_definition, &alias_definition).expect("definition alias"); + let canonical_state = fs::canonicalize(&state).expect("canonical state"); + let runner = RecordingGitRunner::new(vec![ + ok(&format!("{}\n", canonical_state.display())), + ok("spacedock-state/definition-alias\n"), + ]); + + assert!(matches!( + classify_storage(&runner, &alias_definition, Some(".spacedock-state"), None), + WorkflowStorage::SplitRoot { + disposition: StateCheckoutDisposition::Attached, + .. + } + )); + } + + #[test] + fn sync_eligibility_requires_distinct_canonical_roots() { + let temp = tempdir().expect("tempdir"); + let definition = temp.path().join("definition"); + let state = definition.join(".spacedock-state"); + fs::create_dir_all(&state).expect("state dir"); + let storage = WorkflowStorage::SplitRoot { + entity_dir: state.clone(), + expected_branch: "spacedock-state/definition".to_string(), + disposition: StateCheckoutDisposition::Attached, + }; + + assert_eq!( + state_sync_eligibility(&definition, &storage), + StateSyncEligibility::Eligible { + checkout_root: fs::canonicalize(&state).expect("canonical state") + } + ); + + let same_top = WorkflowStorage::SplitRoot { + entity_dir: definition.clone(), + expected_branch: "spacedock-state/definition".to_string(), + disposition: StateCheckoutDisposition::Attached, + }; + assert!(matches!( + state_sync_eligibility(&definition, &same_top), + StateSyncEligibility::Blocked { + problem: StateSyncProblem::SameAsDefinition { .. } + } + )); + } + + #[test] + fn tmp_path_alias_cannot_make_one_checkout_look_distinct() { + let temp = tempfile::tempdir_in("/tmp").expect("tempdir in /tmp"); + let lexical_root = temp.path().to_path_buf(); + let canonical_root = fs::canonicalize(&lexical_root).expect("canonical /tmp root"); + let storage = WorkflowStorage::SplitRoot { + entity_dir: canonical_root, + expected_branch: "spacedock-state/alias".to_string(), + disposition: StateCheckoutDisposition::Attached, + }; + + assert!(matches!( + state_sync_eligibility(&lexical_root, &storage), + StateSyncEligibility::Blocked { + problem: StateSyncProblem::SameAsDefinition { .. } + } + )); + } } diff --git a/crates/spacetop-core/tests/state_checkout_fixtures.rs b/crates/spacetop-core/tests/state_checkout_fixtures.rs index 02b4b4f..e76f5fe 100644 --- a/crates/spacetop-core/tests/state_checkout_fixtures.rs +++ b/crates/spacetop-core/tests/state_checkout_fixtures.rs @@ -2,7 +2,7 @@ use std::fs; use std::path::Path; use std::process::Command; -use spacetop_core::domain::{StateCheckoutDisposition, WorkflowStorage}; +use spacetop_core::domain::{StateCheckoutDisposition, StateTopologyProblem, WorkflowStorage}; use spacetop_core::index::WorkflowIndex; use spacetop_core::parser::load_workflow_dir; use spacetop_core::sources::WorkflowSources; @@ -22,6 +22,24 @@ fn git(root: &Path, args: &[&str]) { ); } +fn git_stdout(root: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .arg("-C") + .arg(root) + .args(args) + .output() + .expect("run git"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout) + .expect("utf-8 git output") + .trim() + .to_string() +} + fn write_workflow(definition: &Path) { fs::create_dir_all(definition).expect("definition dir"); fs::write( @@ -138,6 +156,7 @@ fn external_symlinked_git_checkout_is_unverified_and_remains_readable() { let definition = temp.path().join("demo"); let external_state = temp.path().join("external-state"); write_workflow(&definition); + git(&definition, &["init", "--initial-branch", "main"]); write_entities(&external_state); git( &external_state, @@ -145,14 +164,48 @@ fn external_symlinked_git_checkout_is_unverified_and_remains_readable() { ); symlink(&external_state, definition.join(".spacedock-state")).expect("state symlink"); + let declared_state = definition.join(".spacedock-state"); + assert_eq!( + fs::read_link(&declared_state).expect("symlink target"), + external_state + ); + let canonical_definition = fs::canonicalize(&definition).expect("canonical definition"); + let canonical_state = fs::canonicalize(&declared_state).expect("canonical state"); + assert!(!canonical_state.starts_with(&canonical_definition)); + assert_eq!( + Path::new(&git_stdout(&definition, &["rev-parse", "--show-toplevel"])), + canonical_definition + ); + assert_eq!( + Path::new(&git_stdout( + &external_state, + &["rev-parse", "--show-toplevel"] + )), + canonical_state + ); + assert_ne!( + canonical_definition, canonical_state, + "not a duplicate pull" + ); + assert_eq!( + git_stdout( + &external_state, + &["symbolic-ref", "--quiet", "--short", "HEAD"] + ), + "spacedock-state/demo" + ); + let snapshot = load_workflow_dir(&definition, temp.path()).expect("workflow load"); assert!(matches!( snapshot.definition.storage, WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::ProbeFailed { ref reason }, + disposition: StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::OutsideDefinition { ref resolved_state } + }, .. - } if reason.contains("resolves outside workflow definition directory") + } if resolved_state == &fs::canonicalize(&external_state).expect("canonical external state") )); - assert_eq!(snapshot.items.len(), 1, "non-holder content stays readable"); + assert_eq!(snapshot.items.len(), 1, "external content stays readable"); + assert_eq!(snapshot.items[0].body.trim(), "active body"); } diff --git a/crates/spacetop/src/app/overview.rs b/crates/spacetop/src/app/overview.rs index cc154ac..d1659ec 100644 --- a/crates/spacetop/src/app/overview.rs +++ b/crates/spacetop/src/app/overview.rs @@ -8,7 +8,8 @@ use ratatui::layout::Rect; use spacetop_core::config::{DefaultScope, DefaultSort, SpacetopConfig}; use spacetop_core::discovery::resolve_scan_root; use spacetop_core::domain::{ - Entity, EntityParseError, StateCheckoutDisposition, WorkflowSnapshot, WorkflowStorage, + Entity, EntityParseError, StateCheckoutDisposition, StateTopologyProblem, WorkflowSnapshot, + WorkflowStorage, }; use spacetop_core::entity_identity::entity_slug; pub use spacetop_core::index::StageCount; @@ -105,8 +106,9 @@ pub enum StateTopologyDiagnostic { expected_branch: String, }, Missing, - ProbeFailed { - reason: String, + Unverified { + declared_path: PathBuf, + problem: StateTopologyProblem, }, } @@ -122,13 +124,63 @@ impl fmt::Display for StateTopologyDiagnostic { "State on branch {actual_branch}; expected {expected_branch}" ), Self::Missing => formatter.write_str("State checkout missing; no state loaded"), - Self::ProbeFailed { reason } => { - write!(formatter, "State topology unverified: {reason}") - } + Self::Unverified { + declared_path, + problem, + } => write_unverified_diagnostic(formatter, declared_path, problem), } } } +fn write_unverified_diagnostic( + formatter: &mut fmt::Formatter<'_>, + declared_path: &Path, + problem: &StateTopologyProblem, +) -> fmt::Result { + match problem { + StateTopologyProblem::OutsideDefinition { resolved_state } => write!( + formatter, + "State checkout resolves outside workflow at {}; snapshot is readable, sync is blocked. Materialize it at {}; Spacetop will not repair it", + resolved_state.display(), + declared_path.display() + ), + StateTopologyProblem::DefinitionPathResolution { path, error } => write!( + formatter, + "State topology unverified: cannot resolve workflow definition {}: {error}; sync blocked", + path.display() + ), + StateTopologyProblem::StatePathResolution { path, error } => write!( + formatter, + "State topology unverified: cannot resolve state directory {}: {error}; sync blocked", + path.display() + ), + StateTopologyProblem::GitTopLevelProbe { error } => write!( + formatter, + "State topology unverified: Git top-level probe failed: {error}; sync blocked" + ), + StateTopologyProblem::EmptyGitTopLevel => formatter.write_str( + "State topology unverified: Git reported an empty checkout root; sync blocked", + ), + StateTopologyProblem::GitTopLevelResolution { path, error } => write!( + formatter, + "State topology unverified: cannot resolve Git checkout root {}: {error}; sync blocked", + path.display() + ), + StateTopologyProblem::CheckoutRootMismatch { actual_top } => write!( + formatter, + "State directory belongs to checkout {}; snapshot is readable, sync is blocked", + actual_top.display() + ), + StateTopologyProblem::BranchProbe { error } => write!( + formatter, + "State topology unverified: Git branch probe failed: {error}; sync blocked" + ), + StateTopologyProblem::EmptyBranch => formatter.write_str( + "State topology unverified: Git reported an empty branch name; sync blocked", + ), + } +} + #[derive(Debug, Clone, PartialEq)] pub struct OverviewState { pub workflow_dir: PathBuf, @@ -405,6 +457,7 @@ impl OverviewState { pub fn topology_diagnostic(&self) -> Option { let WorkflowStorage::SplitRoot { + entity_dir, expected_branch, disposition, .. @@ -422,9 +475,10 @@ impl OverviewState { }) } StateCheckoutDisposition::Missing => Some(StateTopologyDiagnostic::Missing), - StateCheckoutDisposition::ProbeFailed { reason } => { - Some(StateTopologyDiagnostic::ProbeFailed { - reason: reason.clone(), + StateCheckoutDisposition::Unverified { problem } => { + Some(StateTopologyDiagnostic::Unverified { + declared_path: entity_dir.clone(), + problem: problem.clone(), }) } } diff --git a/crates/spacetop/src/lib.rs b/crates/spacetop/src/lib.rs index 080a13d..97838b7 100644 --- a/crates/spacetop/src/lib.rs +++ b/crates/spacetop/src/lib.rs @@ -22,11 +22,12 @@ use crossterm::{ use ratatui::{backend::CrosstermBackend, Terminal}; use spacetop_core::config::{self, ConfigLoad, ConfigWarning, SpacetopConfig}; use spacetop_core::discovery; -use spacetop_core::domain::{StateCheckoutDisposition, WorkflowStorage}; +use spacetop_core::domain::{StateSyncEligibility, StateSyncProblem, StateTopologyProblem}; use spacetop_core::editor::{resolve_editor, EditorLauncher, StdEnv, StdLauncher}; use spacetop_core::git_sync::{self, GitRunner, StdGitRunner, SyncOutcome}; use spacetop_core::session_activity::SessionScanState; use spacetop_core::session_state; +use spacetop_core::state_checkout::state_sync_eligibility; use spacetop_core::watcher::{self, WatcherBackend, WatcherConfig, WorkflowWatcher}; /// Result of resolving a CLI invocation into a launch decision, prior to any @@ -52,7 +53,8 @@ mod topology_sync_tests { use std::process::{Command, ExitStatus}; use spacetop_core::domain::{ - StateCheckoutDisposition, WorkflowDefinition, WorkflowSnapshot, WorkflowStorage, + StateCheckoutDisposition, StateTopologyProblem, WorkflowDefinition, WorkflowSnapshot, + WorkflowStorage, }; use spacetop_core::git::GitCmdResult; @@ -383,7 +385,7 @@ mod topology_sync_tests { assert!(matches!( app.workflow_storage(), Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::ProbeFailed { .. }, + disposition: StateCheckoutDisposition::Unverified { .. }, .. }) )); @@ -422,9 +424,11 @@ mod topology_sync_tests { assert!(matches!( app.workflow_storage(), Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::ProbeFailed { reason }, + disposition: StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::OutsideDefinition { .. } + }, .. - }) if reason.contains("resolves outside workflow definition directory") + }) )); assert!(matches!( app.sync_status(), @@ -435,6 +439,48 @@ mod topology_sync_tests { assert_no_state_calls(&runner, &external_state); } + #[test] + fn state_directory_in_definition_checkout_cannot_trigger_duplicate_pull() { + let holder = tempfile::tempdir().expect("tempdir"); + let workflow_dir = holder.path().join("demo"); + let entity_dir = workflow_dir.join(".spacedock-state"); + write_split_root_readme(&workflow_dir, ".spacedock-state"); + fs::create_dir_all(&entity_dir).expect("state dir"); + git(&workflow_dir, &["init", "--initial-branch", "main"]); + let definition_top = fs::canonicalize(&workflow_dir).expect("canonical definition"); + let mut app = app_with_storage( + workflow_dir, + WorkflowStorage::SplitRoot { + entity_dir: entity_dir.clone(), + expected_branch: "spacedock-state/demo".to_string(), + disposition: StateCheckoutDisposition::Attached, + }, + ); + let runner = RecordingGitRunner::new(successful_sync_responses()); + + apply_pending_sync(&mut app, &runner); + + assert!(matches!( + app.workflow_storage(), + Some(WorkflowStorage::SplitRoot { + disposition: StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::CheckoutRootMismatch { actual_top } + }, + .. + }) if actual_top == &definition_top + )); + assert_eq!( + runner + .calls() + .iter() + .filter(|call| { call.args == ["pull".to_string(), ["--ff-", "only"].concat()] }) + .count(), + 1, + "only the definition checkout may be pulled" + ); + assert_no_state_calls(&runner, &entity_dir); + } + #[test] fn state_pull_reload_failure_reports_partial_after_exact_pull() { let holder = tempfile::tempdir().expect("tempdir"); @@ -443,6 +489,7 @@ mod topology_sync_tests { let readme = workflow_dir.join("README.md"); write_split_root_readme(&workflow_dir, ".spacedock-state"); init_attached_state_checkout(&entity_dir); + let canonical_entity_dir = fs::canonicalize(&entity_dir).expect("canonical state root"); let mut app = app_with_storage( workflow_dir, WorkflowStorage::SplitRoot { @@ -463,7 +510,7 @@ mod topology_sync_tests { ]); let runner = RemovingReadmeGitRunner { inner: RecordingGitRunner::new(responses), - state_root: entity_dir.clone(), + state_root: canonical_entity_dir.clone(), readme, }; @@ -481,7 +528,7 @@ mod topology_sync_tests { .calls() .iter() .filter(|call| { - call.repo_root == entity_dir + call.repo_root == canonical_entity_dir && call.args == ["pull".to_string(), ["--ff-", "only"].concat()] }) .count(), @@ -497,6 +544,7 @@ mod topology_sync_tests { let entity_dir = workflow_dir.join(".spacedock-state"); write_split_root_readme(&workflow_dir, ".spacedock-state"); init_attached_state_checkout(&entity_dir); + let canonical_entity_dir = fs::canonicalize(&entity_dir).expect("canonical state root"); let mut app = app_with_storage( workflow_dir, WorkflowStorage::SplitRoot { @@ -519,7 +567,7 @@ mod topology_sync_tests { let state_calls = runner .calls() .into_iter() - .filter(|call| call.repo_root == entity_dir) + .filter(|call| call.repo_root == canonical_entity_dir) .collect::>(); assert_eq!( state_calls @@ -987,21 +1035,19 @@ pub fn apply_pending_sync(app: &mut App, runner: &R) { }); return; } - let storage = app.workflow_storage().cloned(); - let status = match storage { - Some(WorkflowStorage::SingleRoot) => SyncStatus::Succeeded { + let eligibility = app + .workflow_storage() + .map(|storage| state_sync_eligibility(&root, storage)); + let status = match eligibility { + Some(StateSyncEligibility::NotApplicable) => SyncStatus::Succeeded { new_commits: definition_commits, }, - Some(WorkflowStorage::SplitRoot { - entity_dir, - disposition: StateCheckoutDisposition::Attached, - .. - }) => match git_sync::sync(runner, &entity_dir) { - SyncOutcome::UpToDate => SyncStatus::SucceededWithState { - new_commits: definition_commits, - }, - SyncOutcome::Pulled { new_commits } => { - match app.reload() { + Some(StateSyncEligibility::Eligible { checkout_root }) => { + match git_sync::sync(runner, &checkout_root) { + SyncOutcome::UpToDate => SyncStatus::SucceededWithState { + new_commits: definition_commits, + }, + SyncOutcome::Pulled { new_commits } => match app.reload() { Ok(()) => SyncStatus::SucceededWithState { new_commits: definition_commits.saturating_add(new_commits), }, @@ -1010,38 +1056,14 @@ pub fn apply_pending_sync(app: &mut App, runner: &R) { "Definition + state synced; workflow reload failed: {error}" ), }, - } + }, + SyncOutcome::Failed { message } => SyncStatus::Partial { + message: format!("Definition synced; state sync failed: {message}"), + }, } - SyncOutcome::Failed { message } => SyncStatus::Partial { - message: format!("Definition synced; state sync failed: {message}"), - }, - }, - Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::Detached, - .. - }) => SyncStatus::Partial { - message: "Definition synced; detached state not refreshed".to_string(), - }, - Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::WrongBranch { actual_branch }, - expected_branch, - .. - }) => SyncStatus::Partial { - message: format!( - "Definition synced; state on {actual_branch}, expected {expected_branch}, not refreshed" - ), - }, - Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::Missing, - .. - }) => SyncStatus::Partial { - message: "Definition synced; missing state checkout not refreshed".to_string(), - }, - Some(WorkflowStorage::SplitRoot { - disposition: StateCheckoutDisposition::ProbeFailed { reason }, - .. - }) => SyncStatus::Partial { - message: format!("Definition synced; state not refreshed: {reason}"), + } + Some(StateSyncEligibility::Blocked { problem }) => SyncStatus::Partial { + message: blocked_state_sync_message(&problem), }, None => SyncStatus::Failed { message: "no active workflow".to_string(), @@ -1050,6 +1072,73 @@ pub fn apply_pending_sync(app: &mut App, runner: &R) { app.set_sync_status(status); } +fn blocked_state_sync_message(problem: &StateSyncProblem) -> String { + match problem { + StateSyncProblem::Detached => { + "Definition synced; detached state not refreshed".to_string() + } + StateSyncProblem::WrongBranch { + actual_branch, + expected_branch, + } => format!( + "Definition synced; state on {actual_branch}, expected {expected_branch}, not refreshed" + ), + StateSyncProblem::Missing => { + "Definition synced; missing state checkout not refreshed".to_string() + } + StateSyncProblem::Topology(problem) => format!( + "Definition synced; state not refreshed: {}", + topology_problem_for_sync(problem) + ), + StateSyncProblem::DefinitionRootResolution { path, error } => format!( + "Definition synced; state not refreshed: cannot resolve definition sync root {}: {error}", + path.display() + ), + StateSyncProblem::StateRootResolution { path, error } => format!( + "Definition synced; state not refreshed: cannot resolve state checkout {}: {error}", + path.display() + ), + StateSyncProblem::SameAsDefinition { checkout_root } => format!( + "Definition synced; state not refreshed: state checkout is the definition checkout {}; duplicate pull blocked", + checkout_root.display() + ), + } +} + +fn topology_problem_for_sync(problem: &StateTopologyProblem) -> String { + match problem { + StateTopologyProblem::DefinitionPathResolution { path, error } => format!( + "cannot resolve workflow definition directory {}: {error}", + path.display() + ), + StateTopologyProblem::StatePathResolution { path, error } => { + format!("cannot resolve state directory {}: {error}", path.display()) + } + StateTopologyProblem::OutsideDefinition { resolved_state } => format!( + "state directory resolves outside workflow definition directory: {}; sync blocked while the snapshot remains readable", + resolved_state.display() + ), + StateTopologyProblem::GitTopLevelProbe { error } => { + format!("Git top-level probe failed: {error}") + } + StateTopologyProblem::EmptyGitTopLevel => { + "Git reported an empty checkout root".to_string() + } + StateTopologyProblem::GitTopLevelResolution { path, error } => format!( + "cannot resolve Git checkout root {}: {error}", + path.display() + ), + StateTopologyProblem::CheckoutRootMismatch { actual_top } => format!( + "state directory belongs to checkout {} instead of its declared root", + actual_top.display() + ), + StateTopologyProblem::BranchProbe { error } => { + format!("Git branch probe failed: {error}") + } + StateTopologyProblem::EmptyBranch => "Git reported an empty branch name".to_string(), + } +} + fn start_watcher_for( app: &mut App, ) -> Option<( diff --git a/crates/spacetop/src/ui/tests/task_list.rs b/crates/spacetop/src/ui/tests/task_list.rs index 59d57e1..568bc4b 100644 --- a/crates/spacetop/src/ui/tests/task_list.rs +++ b/crates/spacetop/src/ui/tests/task_list.rs @@ -2,7 +2,7 @@ use super::*; #[test] fn footer_renders_stable_split_root_topology_diagnostics() { - use spacetop_core::domain::{StateCheckoutDisposition, WorkflowStorage}; + use spacetop_core::domain::{StateCheckoutDisposition, StateTopologyProblem, WorkflowStorage}; let cases = [ ( @@ -20,10 +20,12 @@ fn footer_renders_stable_split_root_topology_diagnostics() { "State checkout missing; no state loaded", ), ( - StateCheckoutDisposition::ProbeFailed { - reason: "not a Git checkout".to_string(), + StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::GitTopLevelProbe { + error: "not a Git checkout".to_string(), + }, }, - "State topology unverified: not a Git checkout", + "State topology unverified: Git top-level probe failed: not a Git checkout; sync blocked", ), ]; for (disposition, expected) in cases { @@ -39,6 +41,31 @@ fn footer_renders_stable_split_root_topology_diagnostics() { } } +#[test] +fn footer_explains_external_state_is_readable_but_not_repaired_or_synced() { + use spacetop_core::domain::{StateCheckoutDisposition, StateTopologyProblem, WorkflowStorage}; + + let app = app_with_storage(WorkflowStorage::SplitRoot { + entity_dir: PathBuf::from("/tmp/workflow/.spacedock-state"), + expected_branch: "spacedock-state/workflow".to_string(), + disposition: StateCheckoutDisposition::Unverified { + problem: StateTopologyProblem::OutsideDefinition { + resolved_state: PathBuf::from("/tmp/external-state"), + }, + }, + }); + let mut terminal = Terminal::new(TestBackend::new(320, 24)).expect("terminal"); + terminal.draw(|frame| render(frame, &app)).expect("render"); + let rendered = buffer_text(terminal.backend().buffer()); + + assert!( + rendered.contains( + "State checkout resolves outside workflow at /tmp/external-state; snapshot is readable, sync is blocked. Materialize it at /tmp/workflow/.spacedock-state; Spacetop will not repair it" + ), + "rendered={rendered}" + ); +} + #[test] fn footer_has_no_topology_warning_for_attached_or_single_root() { use spacetop_core::domain::{StateCheckoutDisposition, WorkflowStorage}; diff --git a/docs/development-policy.md b/docs/development-policy.md index facacb3..bccb949 100644 --- a/docs/development-policy.md +++ b/docs/development-policy.md @@ -88,7 +88,8 @@ Current two-crate workspace boundaries: `crates/spacetop-core/src/parser.rs` and `crates/spacetop-core/src/parser/*`. - Split-root storage classification and checkout Git probes belong in - `crates/spacetop-core/src/state_checkout.rs`; rendering consumes typed app + `crates/spacetop-core/src/state_checkout.rs`; it also owns the typed, + distinct-root state-sync eligibility decision. Rendering consumes typed app diagnostics and does not infer topology from strings. - `crates/spacetop-core/src/index.rs`, `query.rs`, and `sources.rs` own the v2 index/query spine; TUI code must consume `WorkflowIndex` through query methods @@ -98,7 +99,8 @@ Current two-crate workspace boundaries: - Filesystem watching belongs in `crates/spacetop-core/src/watcher.rs`. - The audited fast-forward helper belongs in `crates/spacetop-core/src/git_sync.rs`; `spacetop/src/lib.rs` orchestrates the - definition-first and verified-attached-state sequence. + definition-first sequence and consumes the explicit eligibility decision + before issuing at most one distinct verified-attached-state pull. - External file opening belongs in `crates/spacetop-core/src/editor.rs`. - User config and session persistence models, XDG/HOME path resolution, and YAML load/save helpers belong in `crates/spacetop-core/src/config.rs` and From 77c32327716ad0c7546615914ba7369ee55787b9 Mon Sep 17 00:00:00 2001 From: Kent Huang Date: Fri, 21 Aug 2026 20:59:33 +0800 Subject: [PATCH 2/2] Clarify checkout root mismatch guidance --- crates/spacetop/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/spacetop/src/lib.rs b/crates/spacetop/src/lib.rs index 97838b7..8d89c05 100644 --- a/crates/spacetop/src/lib.rs +++ b/crates/spacetop/src/lib.rs @@ -479,6 +479,14 @@ mod topology_sync_tests { "only the definition checkout may be pulled" ); assert_no_state_calls(&runner, &entity_dir); + assert!(matches!( + app.sync_status(), + Some(SyncStatus::Partial { message }) + if message == &format!( + "Definition synced; state not refreshed: state directory belongs to checkout {}; declared state directory is not a checkout root", + definition_top.display() + ) + )); } #[test] @@ -1129,7 +1137,7 @@ fn topology_problem_for_sync(problem: &StateTopologyProblem) -> String { path.display() ), StateTopologyProblem::CheckoutRootMismatch { actual_top } => format!( - "state directory belongs to checkout {} instead of its declared root", + "state directory belongs to checkout {}; declared state directory is not a checkout root", actual_top.display() ), StateTopologyProblem::BranchProbe { error } => {