Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/src/services/app_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ fn write_stdout_payload<W: Write>(writer: &mut W, payload: &str) -> Result<(), C
})
}

fn write_error_diagnostic<W: Write>(writer: &mut W, error: &CliError) {
pub(crate) fn write_error_diagnostic<W: Write>(writer: &mut W, error: &CliError) {
write_error_diagnostic_with_color_policy(
writer,
error,
Expand All @@ -184,7 +184,7 @@ fn write_error_diagnostic_with_color_policy<W: Write>(
}
CliError::User {
error: user_error, ..
} => user_error.message().to_string(),
} => user_error.message().clone(),
};
let styled_message = services::style::error_text_with_color_policy(
&services::security::redact_sensitive_text(&rendered),
Expand Down Expand Up @@ -328,7 +328,7 @@ mod tests {

let rendered = String::from_utf8(stderr).expect("stderr is valid utf8");
let redacted_message =
services::security::redact_sensitive_text(UserError::NotAuthenticated.message());
services::security::redact_sensitive_text(&UserError::NotAuthenticated.message());
assert!(rendered.contains(&redacted_message));
}

Expand Down
1 change: 1 addition & 0 deletions cli/src/services/command_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ pub fn default_runtime_command(name: &str) -> Option<RuntimeCommand> {
services::sync::NAME => Some(RuntimeCommand::Sync(services::sync::command::SyncCommand {
request: services::sync::SyncRequest {
format: services::output_format::OutputFormat::Text,
invocation: services::sync::SyncInvocation::Manual,
},
})),
_ => None,
Expand Down
68 changes: 63 additions & 5 deletions cli/src/services/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,33 +48,91 @@ impl FailureClass {
}
}

/// The typed origin of an automatic synchronization failure.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum AutomaticSyncFailureKind {
Authentication,
ControlPlane,
Stream,
Runtime,
}

/// Catalog of expected, deliberately-explained failures presented to the user
/// as a friendly diagnostic instead of a technical error chain.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum UserError {
#[allow(dead_code)]
NotAuthenticated,
AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind,
reason: String,
},
}

impl UserError {
pub fn class(self) -> FailureClass {
pub fn class(&self) -> FailureClass {
match self {
Self::NotAuthenticated => FailureClass::Runtime,
Self::NotAuthenticated | Self::AutomaticSyncFailed { .. } => FailureClass::Runtime,
}
}

#[allow(dead_code)]
pub fn key(self) -> &'static str {
pub fn key(&self) -> &'static str {
match self {
Self::NotAuthenticated => "auth.not_authenticated",
Self::AutomaticSyncFailed { failure_kind, .. } => match failure_kind {
AutomaticSyncFailureKind::Authentication => "sync.automatic.authentication_failed",
AutomaticSyncFailureKind::ControlPlane => "sync.automatic.control_plane_failed",
AutomaticSyncFailureKind::Stream => "sync.automatic.stream_failed",
AutomaticSyncFailureKind::Runtime => "sync.automatic.runtime_failed",
},
}
}

pub fn message(self) -> &'static str {
pub fn message(&self) -> String {
match self {
Self::NotAuthenticated => {
"You are not logged in. Please log in using the `sce auth login` command."
.to_string()
}
Self::AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind::Authentication,
..
} => "Automatic synchronization failed: authentication is required. Run `sce auth login`, then manually retry with `sce sync`.".to_string(),
Self::AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind::ControlPlane,
reason,
} => format!(
"Automatic synchronization failed: {reason}. Check control-plane connectivity and availability, then manually retry with `sce sync`."
),
Self::AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind::Stream,
reason,
} => format!(
"Automatic synchronization failed: {reason}. Check Agent Trace data and connectivity, then manually retry with `sce sync`."
),
Self::AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind::Runtime,
reason,
} => format!(
"Automatic synchronization failed: {reason}. Check the local repository and Agent Trace configuration, then manually retry with `sce sync`."
),
}
}

#[allow(dead_code)]
pub fn automatic_sync_failure_kind(&self) -> Option<AutomaticSyncFailureKind> {
match self {
Self::AutomaticSyncFailed { failure_kind, .. } => Some(*failure_kind),
Self::NotAuthenticated => None,
}
}

#[allow(dead_code)]
pub fn reason(&self) -> Option<&str> {
match self {
Self::AutomaticSyncFailed { reason, .. } => Some(reason),
Self::NotAuthenticated => None,
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion cli/src/services/parse/command_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,10 @@ fn convert_clap_command(command: cli_schema::Commands) -> Result<RuntimeCommand,
)),
cli_schema::Commands::Sync { format } => {
Ok(RuntimeCommand::Sync(services::sync::command::SyncCommand {
request: services::sync::SyncRequest { format },
request: services::sync::SyncRequest {
format,
invocation: services::sync::SyncInvocation::from_environment(),
},
}))
}
}
Expand Down
85 changes: 71 additions & 14 deletions cli/src/services/sync/auto_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@ use std::io;
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};

use crate::services::app_support;
use crate::services::error::{AutomaticSyncFailureKind, CliError, UserError};
use crate::services::sync::{AUTOMATIC_SYNC_INVOCATION_ENV, AUTOMATIC_SYNC_INVOCATION_VALUE};

const SYNC_ARGS: &[&str] = &["sync", "--format", "json"];

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum StdioMode {
Null,
Inherit,
}

#[derive(Clone, Debug, Eq, PartialEq)]
Expand All @@ -19,6 +24,7 @@ struct AutoSyncCommand {
stdin: StdioMode,
stdout: StdioMode,
stderr: StdioMode,
environment: Vec<(String, String)>,
}

impl AutoSyncCommand {
Expand All @@ -29,41 +35,85 @@ impl AutoSyncCommand {
current_dir: repository_root.to_path_buf(),
stdin: StdioMode::Null,
stdout: StdioMode::Null,
stderr: StdioMode::Null,
stderr: StdioMode::Inherit,
environment: vec![(
AUTOMATIC_SYNC_INVOCATION_ENV.to_string(),
AUTOMATIC_SYNC_INVOCATION_VALUE.to_string(),
)],
}
}
}

#[derive(Debug)]
enum AutoSyncLaunchError {
CurrentExecutable(io::Error),
Spawn(io::Error),
}

impl std::fmt::Display for AutoSyncLaunchError {
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::CurrentExecutable(error) => {
write!(formatter, "failed to resolve current executable: {error}")
}
Self::Spawn(error) => write!(formatter, "failed to spawn detached sync: {error}"),
}
}
}

impl std::error::Error for AutoSyncLaunchError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
Self::CurrentExecutable(error) | Self::Spawn(error) => Some(error),
}
}
}

fn launcher_failure_diagnostic(error: AutoSyncLaunchError) -> CliError {
let reason = error.to_string();
CliError::user_with_source(
UserError::AutomaticSyncFailed {
failure_kind: AutomaticSyncFailureKind::Runtime,
reason,
},
error,
)
}

/// Launches the current executable to synchronize the repository in the
/// background. Launcher failures are intentionally ignored by the caller.
/// background. Launcher failures are reported on stderr but remain fail-open
/// to the post-commit caller.
pub fn launch(repository_root: &Path) {
let _ = launch_with(repository_root, std::env::current_exe, spawn_command);
if let Err(error) = launch_with(repository_root, std::env::current_exe, spawn_command) {
let diagnostic = launcher_failure_diagnostic(error);
let mut stderr = io::stderr();
app_support::write_error_diagnostic(&mut stderr, &diagnostic);
}
}

fn launch_with<FCurrentExe, FSpawn>(
repository_root: &Path,
current_exe: FCurrentExe,
spawn: FSpawn,
) -> bool
) -> Result<(), AutoSyncLaunchError>
where
FCurrentExe: FnOnce() -> io::Result<PathBuf>,
FSpawn: FnOnce(AutoSyncCommand) -> io::Result<()>,
{
let Ok(executable) = current_exe() else {
return false;
};
let executable = current_exe().map_err(AutoSyncLaunchError::CurrentExecutable)?;

spawn(AutoSyncCommand::new(executable, repository_root)).is_ok()
spawn(AutoSyncCommand::new(executable, repository_root)).map_err(AutoSyncLaunchError::Spawn)
}

fn spawn_command(spec: AutoSyncCommand) -> io::Result<()> {
let mut command = Command::new(spec.executable);
command
.args(spec.args)
.current_dir(spec.current_dir)
.envs(spec.environment)
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::null());
.stderr(Stdio::inherit());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very suspicious.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

detached execution and inherited stderr are a nono


// Dropping Child does not wait for it; the spawned sync continues
// independently of the post-commit caller.
Expand All @@ -78,7 +128,10 @@ mod tests {
use std::path::{Path, PathBuf};
use std::rc::Rc;

use super::{launch_with, AutoSyncCommand, StdioMode, SYNC_ARGS};
use super::{
launch_with, AutoSyncCommand, StdioMode, AUTOMATIC_SYNC_INVOCATION_ENV,
AUTOMATIC_SYNC_INVOCATION_VALUE, SYNC_ARGS,
};

#[test]
fn launch_builds_the_expected_detached_command() {
Expand All @@ -94,7 +147,7 @@ mod tests {
},
);

assert!(launched);
assert!(launched.is_ok());
assert_eq!(
captured.borrow().clone(),
Some(AutoSyncCommand {
Expand All @@ -103,7 +156,11 @@ mod tests {
current_dir: PathBuf::from("/repo/root"),
stdin: StdioMode::Null,
stdout: StdioMode::Null,
stderr: StdioMode::Null,
stderr: StdioMode::Inherit,
environment: vec![(
AUTOMATIC_SYNC_INVOCATION_ENV.to_string(),
AUTOMATIC_SYNC_INVOCATION_VALUE.to_string(),
)],
})
);
}
Expand All @@ -122,7 +179,7 @@ mod tests {
},
);

assert!(!launched);
assert!(launched.is_err());
assert!(!*spawn_called.borrow());
}

Expand All @@ -134,6 +191,6 @@ mod tests {
|_| Err(io::Error::other("spawn unavailable")),
);

assert!(!launched);
assert!(launched.is_err());
}
}
34 changes: 29 additions & 5 deletions cli/src/services/sync/command.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::io::Write;

use crate::app::ContextWithRepoRoot;
use crate::services::error::{CliError, UserError};
use crate::services::error::{AutomaticSyncFailureKind, CliError, UserError};
use crate::services::sync::progress::{
IndicatifProgressReporter, NoopProgressReporter, ProgressReporter,
};
Expand Down Expand Up @@ -32,7 +32,30 @@ where
}

#[allow(clippy::needless_pass_by_value)]
fn classify_sync_error(err: TraceSyncError) -> CliError {
fn classify_sync_error(
err: TraceSyncError,
invocation: crate::services::sync::SyncInvocation,
) -> CliError {
if invocation == crate::services::sync::SyncInvocation::Automatic {
let failure_kind = if err.is_authentication_failure() {
AutomaticSyncFailureKind::Authentication
} else {
match &err {
TraceSyncError::ControlPlane(_) => AutomaticSyncFailureKind::ControlPlane,
TraceSyncError::Stream { .. } => AutomaticSyncFailureKind::Stream,
TraceSyncError::Runtime(_) => AutomaticSyncFailureKind::Runtime,
}
};
let reason = err.to_string();
return CliError::user_with_source(
UserError::AutomaticSyncFailed {
failure_kind,
reason,
},
err,
);
}

if err.is_authentication_failure() {
CliError::user_with_source(UserError::NotAuthenticated, err)
} else {
Expand Down Expand Up @@ -87,7 +110,7 @@ impl SyncCommand {
run_current_sync_with_progress_and_clock(&repo_root, &mut progress, clock)
}
}
.map_err(classify_sync_error)?;
.map_err(|error| classify_sync_error(error, self.request.invocation))?;

render_sync::render(&report, self.request.format)
.map_err(|error| CliError::runtime(anyhow::Error::msg(format!("{error:#}"))))
Expand All @@ -101,9 +124,10 @@ mod tests {
use crate::services::agent_trace_sync::StreamSyncError;
use crate::services::error::CliError;
use crate::services::sync::sync::TraceSyncError;
use crate::services::sync::SyncInvocation;

fn assert_user_not_authenticated(err: TraceSyncError) {
match classify_sync_error(err) {
match classify_sync_error(err, SyncInvocation::Manual) {
CliError::User { error, source } => {
assert_eq!(error.key(), "auth.not_authenticated");
assert!(source.is_some());
Expand All @@ -113,7 +137,7 @@ mod tests {
}

fn assert_internal(err: TraceSyncError) {
match classify_sync_error(err) {
match classify_sync_error(err, SyncInvocation::Manual) {
CliError::Internal { .. } => {}
other @ CliError::User { .. } => panic!("expected CliError::Internal, got {other:?}"),
}
Expand Down
Loading
Loading