From 370a6c96d5f4612fa45ac7ecf065d52ec20d8cf0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 11 Sep 2026 10:02:01 -0600 Subject: [PATCH] Give the checkout its GitHub credentials through the driver's store The GitHub App token reached the agent's git commands through the origin URL: after the clone fabro ran `git remote set-url origin` with the token embedded, then tracked which generation the URL carried, held an embed lease across every push so a refresh could not rewrite the URL mid-operation, re-embedded on the first auth-shaped push failure in case the agent had rewritten origin, and redacted the URL out of every log line and output tail. The token showed in `git remote -v` and `.git/config`. The driver now installs ambient credentials for a checkout: one credential-store line beside the checkout and a `credential.helper` entry pointing at it, with the remote URL untouched. Fabro's part is `credentials.rs`: the token source, one mint for the clone, one resolve per push operation, and the facet call. The clone carries the token per call and installs it afterwards; the ACP refresh tick rewrites the store instead of the URL; fabro's own pushes pin one resolved token for the whole operation and pass it per call, so nothing is ever re-embedded and a retry after replication lag presents the same token by construction. Gone with the URL: `push_credentials.rs`, `redact.rs`, the lease and drift repair in `git_push`, `RefreshOutcome`, and the `credential_action` and `refresh_error` fields on push attempt events. Stored events that carry those keys still read. A failed store install after the clone now fails setup, where a failed `set-url` used to be logged and repaired by the first push. The one remaining caller of the URL redactor, the server's repository probe, uses `DisplaySafeUrl::redact_in`. Co-Authored-By: Claude Fable 5.1 --- docs/public/integrations/github.mdx | 2 +- lib/apps/fabro-server/src/run_manifest.rs | 6 +- lib/components/fabro-agent/src/lib.rs | 8 +- lib/components/fabro-agent/src/sandbox.rs | 8 +- lib/components/fabro-sandbox/src/clone.rs | 242 ++++--- .../fabro-sandbox/src/credentials.rs | 645 +++--------------- .../fabro-sandbox/src/driver_sandbox.rs | 47 +- lib/components/fabro-sandbox/src/exec.rs | 35 - lib/components/fabro-sandbox/src/lib.rs | 9 +- lib/components/fabro-sandbox/src/sandbox.rs | 588 +++++----------- .../fabro-workflow/src/event/convert.rs | 67 +- .../fabro-workflow/src/handler/llm/acp.rs | 163 ++--- .../fabro-workflow/src/pipeline/publish.rs | 24 +- lib/foundation/fabro-redact/src/safe_url.rs | 17 + .../fabro-types/src/run_event/misc.rs | 31 - 15 files changed, 579 insertions(+), 1313 deletions(-) diff --git a/docs/public/integrations/github.mdx b/docs/public/integrations/github.mdx index 4533f416f..d28b954e0 100644 --- a/docs/public/integrations/github.mdx +++ b/docs/public/integrations/github.mdx @@ -303,7 +303,7 @@ Behavior notes: Workflow authors may name any repository reachable by the server's GitHub App installation; Fabro applies no second server-side repository intersection. The token is scoped server-side to exactly the declared set — a request to an undeclared repository fails at GitHub, and Fabro never mints an unscoped installation-wide token. With `contents = "write"`, **any stage can push to any declared repository**. Declare the smallest repository set and the weakest permissions that work. -Installation Access Tokens are short-lived. Fabro refreshes its own credentials before checkpoint pushes. For ACP/CLI agent turns launched with GitHub App push credentials, Fabro also re-mints the token and rewrites the sandbox's `origin` URL before the ACP process starts, then every 45 minutes for the lifetime of that turn. Refresh failures are logged and do not fail the stage. +Installation Access Tokens are short-lived. Fabro's own pushes present a fresh token on each call. Git commands the agent runs inside the sandbox read the token through a credential store the sandbox driver configures for the checkout; the token never appears in the repository's remote URL or configuration. For ACP/CLI agent turns launched with GitHub App push credentials, Fabro re-mints the token and rewrites that store before the ACP process starts, then every 45 minutes for the lifetime of that turn. Refresh failures are logged and do not fail the stage. `FABRO_PUSH_CRED_REFRESH_AHEAD` defaults to enabled; set it to `0`, `false`, `off`, `no`, or an empty value to disable both turn-entry and background refresh. `FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS` overrides the background interval, and `0` disables only the background loop. This refresh loop is ACP-specific; command and native/API agent stages do not run it. Reconnected sandboxes for resumed or parked runs currently lack the App credentials needed for ACP refresh, so the refresh is skipped there. diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 0811e565d..a864caf18 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -17,7 +17,6 @@ use fabro_graphviz::render::apply_direction; use fabro_llm::FabroClient; use fabro_llm::lithos_catalog::Catalog; use fabro_llm::probe::{self, ModelTestStatus}; -use fabro_sandbox::redact::redact_auth_url; use fabro_sandbox::{ CloneRequest, ProviderAccess, ProviderSandboxSpec, RunSandbox, SandboxSpec, sandbox_spec_for_environment, @@ -874,7 +873,10 @@ async fn check_git_remote_ref( run_ls_remote(command) .await - .map_err(|message| redact_auth_url(&message, auth_url.as_ref())) + .map_err(|message| match &auth_url { + Some(auth_url) => auth_url.redact_in(&message), + None => message, + }) } /// Run a prepared `git ls-remote` invocation with a 10s timeout, reducing a diff --git a/lib/components/fabro-agent/src/lib.rs b/lib/components/fabro-agent/src/lib.rs index 8a03ced4f..82392dbbf 100644 --- a/lib/components/fabro-agent/src/lib.rs +++ b/lib/components/fabro-agent/src/lib.rs @@ -56,10 +56,10 @@ pub use question_tools::{ }; pub use sandbox::{ CaptureStats, DirEntry, DriverSpec, ExecControls, ExecResult, ExecResultExt, ExecSpec, - ExecStreamingResult, FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, - RefreshOutcome, RemoteCredentialAction, RunSandbox, SandboxFile, SandboxSource, StderrTail, - StdioProcess, StdioProcessHandle, Termination, TokenProvenance, TokenSnapshot, WalkOptions, - command_termination, format_lines_numbered, program_exit_code, shell_quote, + ExecStreamingResult, FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, RunSandbox, + SandboxFile, SandboxSource, StderrTail, StdioProcess, StdioProcessHandle, Termination, + TokenProvenance, TokenSnapshot, WalkOptions, command_termination, format_lines_numbered, + program_exit_code, shell_quote, }; pub use session::{ CompletionCoordinator, Session, SessionControlHandle, SessionInputTiming, diff --git a/lib/components/fabro-agent/src/sandbox.rs b/lib/components/fabro-agent/src/sandbox.rs index 462659125..9cb365649 100644 --- a/lib/components/fabro-agent/src/sandbox.rs +++ b/lib/components/fabro-agent/src/sandbox.rs @@ -1,8 +1,8 @@ // Re-export the sandbox types the agent works with from fabro-sandbox. pub use fabro_sandbox::{ CaptureStats, DirEntry, DriverSpec, ExecControls, ExecResult, ExecResultExt, ExecSpec, - ExecStreamingResult, FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, - RefreshOutcome, RemoteCredentialAction, RunSandbox, SandboxFile, SandboxSource, StderrTail, - StdioProcess, StdioProcessHandle, Termination, TokenProvenance, TokenSnapshot, WalkOptions, - command_termination, format_lines_numbered, program_exit_code, shell_quote, + ExecStreamingResult, FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, RunSandbox, + SandboxFile, SandboxSource, StderrTail, StdioProcess, StdioProcessHandle, Termination, + TokenProvenance, TokenSnapshot, WalkOptions, command_termination, format_lines_numbered, + program_exit_code, shell_quote, }; diff --git a/lib/components/fabro-sandbox/src/clone.rs b/lib/components/fabro-sandbox/src/clone.rs index c12a1b976..facb639a1 100644 --- a/lib/components/fabro-sandbox/src/clone.rs +++ b/lib/components/fabro-sandbox/src/clone.rs @@ -7,28 +7,26 @@ //! run works in `/`, a symlink to the checkout. An //! exact commit or a tag is pinned by the driver's clone options, which //! fetch the pin directly and attach the branch to it; an unavailable pin -//! fails the clone and never falls back to the branch head. +//! fails the clone and never falls back to the branch head. The GitHub App +//! token travels with the clone per call and is then installed as the +//! checkout's ambient credentials, so the agent's own git commands can +//! push; the remote URL never carries it. use std::time::Duration; -use fabro_github::token_source::ResolvedToken; -use fabro_redact::DisplaySafeUrl; use fabro_types::SandboxProviderKind; use sandbox_driver::{ - ExecResult, Git as _, GitCloneOptions, GitCredentials, GitFailureKind, Sandbox as DriverHandle, + ExecResult, Git as _, GitCloneOptions, GitFailureKind, Sandbox as DriverHandle, }; use tokio::time; use crate::clone_source::{self, GitHubRepoLayout}; +use crate::credentials::{self, RepoCredentials}; use crate::exec::{ExecResultExt, SandboxExec}; use crate::git_retry::{self, CredentialContext, GitRetryReason, RetryPlan}; -use crate::push_credentials::PushCredentialState; -use crate::redact::redact_auth_url; -use crate::sandbox::shell_quote; /// Whole-clone budget, shared by every network and local step. pub(crate) const GIT_CLONE_TIMEOUT: Duration = Duration::from_mins(5); -const STEP_TIMEOUT: Duration = Duration::from_secs(10); /// What the operator hears when the image has no `git`: the driver classifies /// the failing command, and fabro names the fix. @@ -46,8 +44,7 @@ pub(crate) struct GitHubClone { pub(crate) depth: Option, } -/// What the clone left behind: the layout and the token now embedded in -/// `origin`, if any. +/// What the clone left behind: the layout it checked out into. pub(crate) struct CloneOutcome { pub(crate) layout: GitHubRepoLayout, } @@ -66,8 +63,9 @@ struct CloneFailure { } /// Clone `plan` into `handle`, laid out under `workspace_root` and -/// `repos_root`, embedding a GitHub App token from `credentials` when one -/// is available. +/// `repos_root`, with a GitHub App token from `credentials` when one is +/// available: the clone carries it per call, and the checkout keeps it as +/// ambient credentials afterwards. pub(crate) async fn clone_github_repo( kind: &SandboxProviderKind, handle: &dyn DriverHandle, @@ -75,33 +73,12 @@ pub(crate) async fn clone_github_repo( plan: &GitHubClone, workspace_root: &str, repos_root: &str, - credentials: &PushCredentialState, + credentials: &RepoCredentials, ) -> crate::Result { let layout = clone_source::github_repo_layout(&plan.origin_url, workspace_root, repos_root)?; - // The clone mints its own token (never a warm-cache reuse) and seeds the - // shared source, so the first refresh compares against the clone token - // instead of believing nothing was ever embedded. - let resolved_token = match credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|err| { - crate::Error::context_anyhow("Failed to get GitHub App credentials for clone", err) - })?), - None => None, - }; + let token = credentials.mint_for_clone().await?; let credential_context = - CredentialContext::from_snapshot(resolved_token.as_ref().map(|token| &token.snapshot)); - let auth_url = match &resolved_token { - Some(token) => Some( - fabro_github::embed_token_in_url(&plan.origin_url, token.token.expose()).map_err( - |err| { - crate::Error::context_anyhow( - "Failed to build authenticated GitHub clone URL", - err, - ) - }, - )?, - ), - None => None, - }; + CredentialContext::from_snapshot(token.as_ref().map(|token| &token.snapshot)); let fs = handle.fs(); for dir in [workspace_root, layout.repos_owner_path.as_str()] { @@ -111,7 +88,7 @@ pub(crate) async fn clone_github_repo( } let deadline = time::Instant::now() + GIT_CLONE_TIMEOUT; - let has_app = credentials.source().is_some(); + let has_app = credentials.managed(); let git = handle.git().ok_or_else(|| { crate::Error::message(format!( "sandbox provider `{kind}` does not support git operations" @@ -128,9 +105,7 @@ pub(crate) async fn clone_github_repo( options.commit = plan.commit_sha.clone(); options.tag = plan.tag.clone().filter(|_| plan.commit_sha.is_none()); options.depth = plan.depth; - options.credentials = resolved_token - .as_ref() - .map(|token| GitCredentials::new("x-access-token", token.token.expose())); + options.credentials = token.as_ref().map(credentials::git_credentials); let retry_plan = RetryPlan::clone_default(Some(deadline)); let target = layout.primary_repo_path.clone(); git_retry::retry_git_operation( @@ -168,13 +143,12 @@ pub(crate) async fn clone_github_repo( &clone_source::repo_symlink_command(&layout), "create workspace repo symlink", deadline, - auth_url.as_ref(), has_app, ) .await?; - if let Some(token) = resolved_token { - embed_origin_credentials(exec, &layout, auth_url.as_ref(), token, credentials).await; + if let Some(token) = &token { + RepoCredentials::install(&git, &layout.primary_repo_path, token).await?; } Ok(CloneOutcome { layout }) } @@ -189,7 +163,6 @@ async fn run_local_step( command: &str, label: &'static str, deadline: time::Instant, - auth_url: Option<&DisplaySafeUrl>, has_app: bool, ) -> crate::Result { let remaining = deadline.saturating_duration_since(time::Instant::now()); @@ -206,7 +179,7 @@ async fn run_local_step( return Ok(result); } Err(clone_failure_error( - result.into_exec_error_with_redactor(label, |output| redact_auth_url(output, auth_url)), + result.into_exec_error(label), CloneStep::Local, has_app, )) @@ -227,55 +200,6 @@ fn clone_failure_error(error: crate::Error, step: CloneStep, has_app: bool) -> c crate::Error::context(message, error) } -/// Point `origin` at the authenticated URL so pushes from the checkout -/// carry the clone token, and record that generation for refreshes. A -/// failure here is logged, not fatal: the checkout is complete, and the -/// first push will re-embed. -async fn embed_origin_credentials( - exec: &SandboxExec<'_>, - layout: &GitHubRepoLayout, - auth_url: Option<&DisplaySafeUrl>, - token: ResolvedToken, - credentials: &PushCredentialState, -) { - credentials.record_embedded(token).await; - let Some(auth_url) = auth_url else { - return; - }; - let command = format!( - "git -c maintenance.auto=0 remote set-url origin {}", - shell_quote(auth_url.as_raw_url().as_str()) - ); - match exec - .run( - &command, - Some(STEP_TIMEOUT), - Some(&layout.execution_directory), - None, - None, - ) - .await - { - Ok(result) if result.success() => {} - Ok(result) => { - let err = result - .into_exec_error_with_redactor("git remote set-url origin (post-clone)", |s| { - redact_auth_url(s, Some(auth_url)) - }); - tracing::warn!( - error = %err, - "Failed to set sandbox push credentials on origin; git push from this sandbox will fail" - ); - } - Err(err) => { - tracing::warn!( - error = %redact_auth_url(&crate::display_for_log(&err), Some(auth_url)), - "Failed to set sandbox push credentials on origin; git push from this sandbox will fail" - ); - } - } -} - /// Whether the driver found no usable `git` in the sandbox. fn git_unavailable(error: &crate::Error) -> bool { matches!( @@ -287,9 +211,139 @@ fn git_unavailable(error: &crate::Error) -> bool { #[cfg(test)] mod tests { + use fabro_github::token_source::InstallationTokenSource; use sandbox_driver::{ExecFailure, GitFailure, Termination}; + use sandbox_driver_testing::ScriptedSandbox; use super::*; + use crate::exec::ExplicitEnvPolicy; + + const ORIGIN: &str = "https://github.com/acme/widgets"; + + fn ok() -> ExecResult { + ExecResult::new(Termination::Exited, Some(0), Duration::from_millis(1)) + } + + /// A scripted sandbox whose `origin` answers with the fixture URL and + /// whose every other command succeeds. + fn scripted_handle() -> ScriptedSandbox { + let handle = ScriptedSandbox::with_id_and_working_dir("scripted", "/workspace") + .runtime_directory("/tmp/sandbox-driver/runtime"); + handle.scripted_exec().respond_with(|spec| { + let script = spec.args.last().map(String::as_str).unwrap_or_default(); + script.contains("'remote' 'get-url' 'origin'").then(|| { + let mut result = ok(); + result.stdout = format!("{ORIGIN}\n").into_bytes(); + result + }) + }); + handle.scripted_exec().set_default(ok()); + handle + } + + fn plan() -> GitHubClone { + GitHubClone { + origin_url: ORIGIN.to_owned(), + branch: Some("main".to_owned()), + tag: None, + commit_sha: None, + depth: Some(1), + } + } + + async fn clone_with(handle: &ScriptedSandbox, credentials: &RepoCredentials) -> CloneOutcome { + let exec = SandboxExec::new(handle.exec(), ExplicitEnvPolicy::TrustCaller); + clone_github_repo( + &SandboxProviderKind::DOCKER, + handle, + &exec, + &plan(), + "/workspace", + "/repos", + credentials, + ) + .await + .expect("clone succeeds") + } + + #[tokio::test] + async fn a_clone_carries_the_token_per_call_and_installs_it_for_the_checkout() { + let handle = scripted_handle(); + let credentials = + RepoCredentials::new(Some(InstallationTokenSource::pat("ghp_test".to_owned()))); + + let outcome = clone_with(&handle, &credentials).await; + assert_eq!(outcome.layout.primary_repo_path, "/repos/acme/widgets"); + + let commands = handle.scripted_exec().commands(); + assert!( + commands + .iter() + .all(|command| !command.contains("git --version")), + "no probe runs ahead of the clone: {commands:#?}" + ); + assert!( + commands.iter().all(|command| !command.contains("set-url")), + "the remote URL is never rewritten: {commands:#?}" + ); + let clone = commands + .iter() + .find(|command| command.contains("'clone'")) + .expect("the clone ran"); + assert!( + clone.contains( + "x-access-token:ghp_test@github.com/acme/widgets.insteadOf=https://github.com/acme/widgets" + ), + "the clone carries the token per call: {clone}" + ); + assert!( + commands.iter().any(|command| command.starts_with("ln -s ")), + "{commands:#?}" + ); + let install = commands + .iter() + .find(|command| command.contains("--add credential.helper")) + .expect("the checkout's credential store is installed"); + assert!( + install.contains("/tmp/sandbox-driver/runtime/git-credentials/"), + "{install}" + ); + assert!( + commands + .iter() + .all(|command| !command.contains("ghp_test") || command.contains("insteadOf")), + "the secret enters no command but the clone's own rewrite: {commands:#?}" + ); + assert!( + handle.scripted_exec().recorded().iter().any(|spec| { + spec.env + .get("SANDBOX_DRIVER_GIT_CREDENTIAL") + .map(String::as_str) + == Some("https://x-access-token:ghp_test@github.com") + }), + "the store line travels in the environment" + ); + } + + #[tokio::test] + async fn a_clone_without_managed_credentials_installs_nothing() { + let handle = scripted_handle(); + + clone_with(&handle, &RepoCredentials::none()).await; + + let commands = handle.scripted_exec().commands(); + assert!( + commands.iter().any(|command| command.contains("'clone'")), + "{commands:#?}" + ); + assert!( + commands + .iter() + .all(|command| !command.contains("insteadOf") + && !command.contains("credential.helper")), + "{commands:#?}" + ); + } fn git_failure(exit_code: i32, stderr: &str) -> crate::Error { crate::Error::driver_error(sandbox_driver::Error::Git(GitFailure::from_command( diff --git a/lib/components/fabro-sandbox/src/credentials.rs b/lib/components/fabro-sandbox/src/credentials.rs index 1fb4834ce..3d6d727a8 100644 --- a/lib/components/fabro-sandbox/src/credentials.rs +++ b/lib/components/fabro-sandbox/src/credentials.rs @@ -1,25 +1,23 @@ -//! Shared push-credential state for clone-based sandbox providers. +//! GitHub credentials for a clone-based sandbox's repository. //! -//! Docker and Daytona embed GitHub credentials into the cloned repository's -//! `origin` remote and refresh them before pushes. Both providers hold this -//! state so the compare → `set-url` → record sequence, the generation -//! tracking, and the refresh-error logging behave identically across -//! providers. The token cache itself sits below the providers, in -//! [`fabro_github::token_source::InstallationTokenSource`]. +//! Fabro decides which credential a checkout works with and when it is +//! renewed; the sandbox driver applies it. The facet's own network +//! operations, fabro's clone and pushes, take the token per call and never +//! write it into the repository. The agent's own git commands read it from +//! the credential store the driver installs beside the checkout, which the +//! workflow's refresh tick rewrites as the token is renewed. The remote URL +//! is never touched, so no secret shows in `git remote -v` or in +//! `.git/config`. The token cache itself sits below, in +//! [`InstallationTokenSource`]. -use std::future::Future; use std::sync::Arc; use fabro_github::GitHubCredentials; -use fabro_github::token_source::{InstallationTokenSource, ResolvedToken, TokenSnapshot}; -use fabro_redact::DisplaySafeUrl; -pub use fabro_types::run_event::GitCredentialRefreshError as RefreshErrorKind; -use sandbox_driver::Termination; -use tokio::sync::{Mutex, MutexGuard}; +use fabro_github::token_source::{InstallationTokenSource, ResolvedToken}; +use sandbox_driver::{Git as _, GitCredentials, GitFacet}; -use crate::exec::ExecResultExt; -use crate::redact; -use crate::sandbox::{RefreshOutcome, RemoteCredentialAction}; +/// The username GitHub expects with an installation token or PAT. +pub(crate) const GITHUB_TOKEN_USERNAME: &str = "x-access-token"; /// Build the shared installation-token source for a clone-based sandbox. /// @@ -52,579 +50,98 @@ pub(crate) fn build_token_source( .map_err(|err| crate::Error::context_anyhow("Failed to build GitHub token source", err)) } -/// Push-credential state one provider instance tracks for its `origin` -/// remote. -pub(crate) struct PushCredentialState { - source: Option>, - /// Serializes compare → `set-url` → record. The token source's - /// single-flight ends before the sandbox exec, so without this lock a - /// refresh-ahead tick and a push could both see the old embedded - /// generation and race on `.git/config.lock`. Holds the last - /// successfully embedded token: its secret is already in the remote URL - /// inside the sandbox, so retaining it adds no exposure, and it is what - /// a push falls back to when a refresh fails. The tracked value is local - /// belief, not ground truth — agent code inside the sandbox can rewrite - /// `origin`. - embedded: Mutex>, +/// The GitHub credentials a run's checkout works with: a token source when +/// fabro manages them, nothing when the repository was cloned without a +/// GitHub App or the sandbox was reattached by a later process. +pub(crate) struct RepoCredentials { + source: Option>, } -impl PushCredentialState { +impl RepoCredentials { pub(crate) fn new(source: Option>) -> Self { - Self { - source, - embedded: Mutex::new(None), - } + Self { source } + } + + /// No managed credentials: pushes and the agent's git commands use + /// whatever the checkout already has. + pub(crate) fn none() -> Self { + Self::new(None) } pub(crate) fn source(&self) -> Option<&Arc> { self.source.as_ref() } - /// Record the token embedded in `origin` outside the refresh path — the - /// clone is the first operation to embed a token, and it seeds this - /// state so the first refresh compares against the clone token instead - /// of believing nothing was ever embedded. - pub(crate) async fn record_embedded(&self, token: ResolvedToken) { - *self.embedded.lock().await = Some(token); + pub(crate) fn managed(&self) -> bool { + self.source.is_some() } - /// Refresh the credentials embedded in `origin`. - /// - /// Resolves through the shared source, skips the `set-url` exec when the - /// resolved generation is already embedded, and records the new - /// generation only after `set_url` succeeds. `set_url` receives the - /// authenticated URL to embed and runs under the embed lock. - pub(crate) async fn refresh( - &self, - origin_url: &str, - set_url: F, - ) -> crate::Result - where - F: FnOnce(DisplaySafeUrl) -> Fut, - Fut: Future>, - { + /// Mint the clone token. Never a warm-cache reuse: a clone retried on + /// replication lag must hold the token minted for it. The mint seeds + /// the source, so later resolves reuse this token until it nears + /// expiry. + pub(crate) async fn mint_for_clone(&self) -> crate::Result> { let Some(source) = &self.source else { - return Ok(RefreshOutcome::none()); + return Ok(None); }; - let mut embedded = self.embedded.lock().await; - let resolved = match source.resolve().await { - Ok(resolved) => resolved, - Err(err) => { - // The refresh-error path is defined, not incidental: the push - // proceeds with the last embedded token, so log which one - // that is instead of losing the credential state. - if let Some(prev) = embedded.as_ref() { - tracing::warn!( - error = %format!("{err:#}"), - generation = prev.snapshot.generation, - provenance = %prev.snapshot.provenance, - token_age_ms = prev.snapshot.age_ms(), - "GitHub token refresh failed; origin keeps the last embedded credentials" - ); - } else { - tracing::warn!( - error = %format!("{err:#}"), - "GitHub token refresh failed and no credentials were ever embedded" - ); - } - return Err(crate::Error::context_anyhow( - "Failed to refresh push credentials", - err, - )); - } + source.mint_for_clone().await.map(Some).map_err(|err| { + crate::Error::context_anyhow("Failed to get GitHub App credentials for clone", err) + }) + } + + /// The token one operation works with, reused from the cache until it + /// nears expiry. A refresh that fails while the cached token is still + /// valid returns that token. + pub(crate) async fn resolve(&self) -> crate::Result> { + let Some(source) = &self.source else { + return Ok(None); }; - if embedded - .as_ref() - .is_some_and(|prev| prev.snapshot.generation == resolved.snapshot.generation) - { - return Ok(RefreshOutcome::unchanged(resolved.snapshot)); - } - let auth_url = fabro_github::embed_token_in_url(origin_url, resolved.token.expose()) - .map_err(|err| { - crate::Error::context_anyhow("Failed to build authenticated origin URL", err) - })?; - set_url(auth_url).await?; - let snapshot = resolved.snapshot; - *embedded = Some(resolved); - Ok(RefreshOutcome::embedded(snapshot)) + source.resolve().await.map(Some).map_err(|err| { + crate::Error::context_anyhow("Failed to refresh GitHub App credentials", err) + }) + } + + /// Install `token` as the credentials every git command run inside the + /// sandbox picks up for the checkout at `repo_path`. The driver keeps + /// them in a credential store beside the checkout and points the + /// repository's helper configuration at it; calling again replaces + /// them in place. + pub(crate) async fn install( + git: &GitFacet<'_>, + repo_path: &str, + token: &ResolvedToken, + ) -> crate::Result<()> { + git.set_ambient_credentials(repo_path, Some(&git_credentials(token))) + .await + .map_err(|error| { + crate::Error::context("Failed to install the checkout's GitHub credentials", error) + }) } } -/// What [`CredentialLease::ensure_embedded`] did for one push attempt. -#[derive(Debug, Clone, Copy)] -pub(crate) struct EnsureOutcome { - pub action: RemoteCredentialAction, - /// The token embedded in the remote right now — never an unembedded mint. - pub token: Option, - pub refresh_error: Option, +/// The per-call form of `token` for the driver's network operations. +pub(crate) fn git_credentials(token: &ResolvedToken) -> GitCredentials { + GitCredentials::new(GITHUB_TOKEN_USERNAME, token.token.expose()) } -/// Scoped pin of push credentials for one push operation. -/// -/// Holds the provider's embed mutex until dropped, so no other refresh can -/// re-embed mid-operation — a refresh-ahead tick crossing the cache margin -/// during a retrying push waits here instead of swapping the remote out from -/// under the pin. Internally retains up to two secrets: the last successfully -/// embedded token (the fallback) and the operation's resolved target, so both -/// drift re-embedding and the refresh-error fallback work. Only non-secret -/// snapshots leave the lease. -/// -/// A successful resolve happens at most once per operation and is never -/// replaced; the pin transitions to the target only through a successful -/// embed. The token source's refresh margin exceeds every push plan's elapsed -/// bound, so the pinned token always outlives the operation. -pub(crate) struct CredentialLease<'a> { - source: Option<&'a InstallationTokenSource>, - /// Embed-mutex guard: the last successfully embedded token. - embedded: MutexGuard<'a, Option>, - /// The operation's resolved target, including a cached fallback when a - /// refresh mint failed. - target: Option, - /// Skip an immediate duplicate resolve after lease acquisition already - /// failed. A later push attempt can retry after backoff. - defer_resolve_once: bool, -} - -impl PushCredentialState { - /// Acquire the push-credential lease for one push operation. - /// - /// Resolves the operation's target token up front. A failed refresh can - /// return a valid cached token; the first attempt uses it, and - /// [`CredentialLease::ensure_embedded`] retries the refresh after push - /// backoff. A resolve with no cached or embedded token fails acquisition. - pub(crate) async fn lease(&self) -> crate::Result> { - let embedded = self.embedded.lock().await; - let Some(source) = self.source.as_deref() else { - return Ok(CredentialLease { - source: None, - embedded, - target: None, - defer_resolve_once: false, - }); - }; - match source.resolve().await { - Ok(resolved) => { - let defer_resolve_once = resolved.refresh_failed; - Ok(CredentialLease { - source: Some(source), - embedded, - target: Some(resolved), - defer_resolve_once, - }) - } - Err(err) => { - if let Some(prev) = embedded.as_ref() { - tracing::warn!( - error = %format!("{err:#}"), - generation = prev.snapshot.generation, - provenance = %prev.snapshot.provenance, - token_age_ms = prev.snapshot.age_ms(), - "token resolve failed; push pins the last embedded credentials" - ); - Ok(CredentialLease { - source: Some(source), - embedded, - target: None, - defer_resolve_once: true, - }) - } else { - tracing::warn!( - error = %format!("{err:#}"), - "token resolve failed and no credentials were ever embedded" - ); - Err(crate::Error::message( - "Failed to refresh push credentials: token_mint_failed", - )) - } - } - } - } -} - -impl CredentialLease<'_> { - /// Non-secret description of the token embedded in the remote right now. - pub(crate) fn snapshot(&self) -> Option { - self.embedded.as_ref().map(|token| token.snapshot) - } - - /// Embed the pinned generation if the remote does not carry it. - /// - /// One call covers the initial embed, a deferred embed after an earlier - /// failure, and drift repair (`force` re-embeds even when the tracked - /// generation matches, for remotes rewritten inside the sandbox). While - /// the lease has no target, this retries the failed `resolve()` first — - /// retrying a failed resolve discards no fresh token, so it cannot - /// restart any replication clock. Refresh failures are recorded, never - /// propagated: the push proceeds with the last embedded token. - pub(crate) async fn ensure_embedded( - &mut self, - sandbox: &crate::RunSandbox, - origin_url: &str, - force: bool, - ) -> crate::Result { - let Some(source) = self.source else { - return Ok(EnsureOutcome { - action: RemoteCredentialAction::None, - token: None, - refresh_error: None, - }); - }; - let mut refresh_error = self.defer_resolve_once.then_some(RefreshErrorKind::Mint); - if self.defer_resolve_once { - self.defer_resolve_once = false; - } else if self - .target - .as_ref() - .is_none_or(|resolved| resolved.refresh_failed) - { - match source.resolve().await { - Ok(resolved) => { - refresh_error = resolved.refresh_failed.then_some(RefreshErrorKind::Mint); - self.target = Some(resolved); - } - Err(err) => { - tracing::warn!( - error = %format!("{err:#}"), - "token resolve retry failed; pushing with the last embedded token" - ); - refresh_error = Some(RefreshErrorKind::Mint); - } - } - } - let Some(desired) = self.target.as_ref().or(self.embedded.as_ref()).cloned() else { - // Managed credentials with nothing resolved or embedded: - // acquisition fails before any attempt runs, so pushes never see - // this state. - return Ok(EnsureOutcome { - action: RemoteCredentialAction::None, - token: None, - refresh_error, - }); - }; - let embedded_generation = self - .embedded - .as_ref() - .map(|token| token.snapshot.generation); - if !force && embedded_generation == Some(desired.snapshot.generation) { - return Ok(EnsureOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(desired.snapshot), - refresh_error, - }); - } - match set_url_via_exec(sandbox, origin_url, &desired).await { - Ok(()) => { - let snapshot = desired.snapshot; - *self.embedded = Some(desired); - Ok(EnsureOutcome { - action: RemoteCredentialAction::Embedded, - token: Some(snapshot), - refresh_error, - }) - } - Err(err) => { - if err - .exec_failure() - .is_some_and(|failure| failure.termination() != Termination::Exited) - { - return Err(err); - } - tracing::warn!( - error = %crate::display_for_log(&err), - "embedding push credentials in origin failed; pushing with the last embedded token" - ); - Ok(EnsureOutcome { - action: RemoteCredentialAction::Unchanged, - token: self.snapshot(), - refresh_error: Some(RefreshErrorKind::SetUrl), - }) - } - } - } -} - -/// Rewrite `origin` with the token embedded, through the sandbox's uniform -/// exec surface. -async fn set_url_via_exec( - sandbox: &crate::RunSandbox, - origin_url: &str, - token: &ResolvedToken, -) -> crate::Result<()> { - let auth_url = - fabro_github::embed_token_in_url(origin_url, token.token.expose()).map_err(|err| { - crate::Error::context( - "Failed to build authenticated origin URL", - RedactedSetUrlError(fabro_redact::redact_string(&format!("{err:#}"))), - ) - })?; - set_auth_url_via_exec(sandbox, auth_url).await -} - -pub(crate) async fn set_auth_url_via_exec( - sandbox: &crate::RunSandbox, - auth_url: DisplaySafeUrl, -) -> crate::Result<()> { - let command = format!( - "git -c maintenance.auto=0 remote set-url origin {}", - crate::shell_quote(auth_url.as_raw_url().as_str()) - ); - let result = sandbox - .exec_command(&command, 10_000, None, None, None) - .await - .map_err(|err| { - let message = redact::redact_auth_url(&crate::display_for_log(&err), Some(&auth_url)); - crate::Error::context( - "Failed to refresh push credentials: set_url_exec_failed", - RedactedSetUrlError(message), - ) - })?; - if !result.success() { - return Err(result.into_exec_error_with_redactor( - "git remote set-url origin (refresh push credentials)", - |s| redact::redact_auth_url(s, Some(&auth_url)), - )); - } - Ok(()) -} - -#[derive(Debug, thiserror::Error)] -#[error("{0}")] -struct RedactedSetUrlError(String); - #[cfg(test)] mod tests { - use std::sync::atomic::{AtomicUsize, Ordering}; - - use chrono::Utc; - use fabro_github::InstallationToken; - use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; - use tokio::time::sleep; - use super::*; - use crate::sandbox::RemoteCredentialAction; - struct FixedMinter { - calls: AtomicUsize, - ttl: chrono::Duration, - } - - #[async_trait::async_trait] - impl InstallationTokenMinter for FixedMinter { - async fn mint(&self) -> anyhow::Result { - let call = self.calls.fetch_add(1, Ordering::SeqCst) + 1; - Ok(InstallationToken { - token: format!("ghs_gen{call}"), - expires_at: Utc::now() + self.ttl, - }) - } - } - - struct FailingMinter; - - #[async_trait::async_trait] - impl InstallationTokenMinter for FailingMinter { - async fn mint(&self) -> anyhow::Result { - Err(anyhow::anyhow!("mint failed")) - } - } - - fn minting_state(ttl: chrono::Duration) -> PushCredentialState { - PushCredentialState::new(Some(installation_token_source( - "owner/repo", - Arc::new(FixedMinter { - calls: AtomicUsize::new(0), - ttl, - }), - ))) - } - - const ORIGIN: &str = "https://github.com/owner/repo"; - /// Long enough for a blocked task to be observably pending on paused time. - const SHORT_WAIT: std::time::Duration = std::time::Duration::from_secs(5); - - /// A refresh-ahead tick crossing the cache margin during a push waits on - /// the embed mutex until the operation releases the lease, so the remote - /// can never be swapped out from under the pinned generation. - #[tokio::test(start_paused = true)] - async fn refresh_waits_for_the_lease_to_release() { - let state = std::sync::Arc::new(minting_state(chrono::Duration::minutes(60))); - - let lease = state.lease().await.expect("lease acquires"); - - let refresh_task = { - let state = std::sync::Arc::clone(&state); - tokio::spawn(async move { - state - .refresh(ORIGIN, |_| async { Ok(()) }) - .await - .expect("refresh succeeds after the lease releases") - }) - }; - - // The refresh must be blocked while the lease holds the embed mutex. - sleep(SHORT_WAIT).await; - assert!( - !refresh_task.is_finished(), - "refresh must wait on the embed mutex" - ); - - drop(lease); - let outcome = refresh_task.await.expect("refresh task completes"); - // The lease's resolve minted generation 1; the deferred refresh - // reuses it (the operation never embedded, so the refresh embeds). - assert_eq!(outcome.token().unwrap().generation, 1); + #[tokio::test] + async fn unmanaged_credentials_resolve_to_nothing() { + let credentials = RepoCredentials::none(); + assert!(!credentials.managed()); + assert!(credentials.mint_for_clone().await.unwrap().is_none()); + assert!(credentials.resolve().await.unwrap().is_none()); } #[tokio::test] - async fn refresh_without_managed_credentials_reports_none() { - let state = PushCredentialState::new(None); - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome, RefreshOutcome::none()); - } - - #[tokio::test] - async fn refresh_embeds_a_new_generation_and_skips_matching_ones() { - let state = minting_state(chrono::Duration::minutes(60)); - let set_url_calls = AtomicUsize::new(0); - - let first = state - .refresh(ORIGIN, |auth_url| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - assert!(auth_url.as_raw_url().as_str().contains("ghs_gen1")); - async { Ok(()) } - }) - .await - .unwrap(); - assert_eq!(first.action(), RemoteCredentialAction::Embedded); - assert_eq!(first.token().unwrap().generation, 1); - - // The cached token is fresh, so the second refresh must skip set-url. - let second = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - assert_eq!(second.action(), RemoteCredentialAction::Unchanged); - assert_eq!(second.token().unwrap().generation, 1); - assert_eq!(set_url_calls.load(Ordering::SeqCst), 1); - } - - #[tokio::test] - async fn refresh_embeds_again_when_the_source_mints_a_new_generation() { - // Tokens expire inside the margin, so every resolve re-mints. - let state = minting_state(chrono::Duration::minutes(5)); - let set_url_calls = AtomicUsize::new(0); - - let first = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - let second = state - .refresh(ORIGIN, |_| { - set_url_calls.fetch_add(1, Ordering::SeqCst); - async { Ok(()) } - }) - .await - .unwrap(); - - assert_eq!(first.token().unwrap().generation, 1); - assert_eq!(second.action(), RemoteCredentialAction::Embedded); - assert_eq!(second.token().unwrap().generation, 2); - assert_eq!(set_url_calls.load(Ordering::SeqCst), 2); - } - - #[tokio::test] - async fn clone_seed_makes_the_first_refresh_a_no_op() { - let state = minting_state(chrono::Duration::minutes(60)); - let clone_token = state.source().unwrap().mint_for_clone().await.unwrap(); - state.record_embedded(clone_token).await; - - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); - assert_eq!(outcome.token().unwrap().generation, 1); - } - - #[tokio::test] - async fn failed_set_url_does_not_record_the_new_generation() { - let state = minting_state(chrono::Duration::minutes(60)); - - let err = state - .refresh(ORIGIN, |_| async { - Err(crate::Error::message("set-url failed")) - }) - .await - .unwrap_err(); - assert!(err.to_string().contains("set-url failed")); - - // The generation was not recorded, so the retry embeds again instead - // of wrongly skipping. - let retried = state.refresh(ORIGIN, |_| async { Ok(()) }).await.unwrap(); - assert_eq!(retried.action(), RemoteCredentialAction::Embedded); - assert_eq!(retried.token().unwrap().generation, 1); - } - - #[tokio::test] - async fn static_credentials_seeded_at_clone_skip_set_url() { - let source = InstallationTokenSource::for_origin( - &GitHubCredentials::Pat("ghp_pat".to_string()), - ORIGIN, - serde_json::json!({ "contents": "write" }), - ) - .unwrap(); - let state = PushCredentialState::new(Some(source)); - let clone_token = state.source().unwrap().mint_for_clone().await.unwrap(); - state.record_embedded(clone_token).await; - - let outcome = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap(); - assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); - assert!(outcome.token().unwrap().is_static()); - } - - #[tokio::test] - async fn mint_failure_preserves_the_mint_error_chain() { - let state = PushCredentialState::new(Some(installation_token_source( - "owner/repo", - Arc::new(FailingMinter), - ))); - - let err = state - .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) - .await - .unwrap_err(); - assert_eq!(err.causes(), vec![ - "minting GitHub installation access token", - "mint failed" - ]); - } - - #[test] - fn token_source_requires_managed_credentials_and_a_github_origin() { - assert!(build_token_source(None, Some(ORIGIN)).unwrap().is_none()); - let pat = GitHubCredentials::Pat("ghp_pat".to_string()); - assert!(build_token_source(Some(&pat), None).unwrap().is_none()); - assert!( - build_token_source(Some(&pat), Some("https://gitlab.com/owner/repo")) - .unwrap() - .is_none() - ); - assert!( - build_token_source(Some(&pat), Some(ORIGIN)) - .unwrap() - .is_some() - ); + async fn a_pat_becomes_per_call_credentials_under_the_github_username() { + let source = InstallationTokenSource::pat("ghp_static".to_owned()); + let token = source.resolve().await.unwrap(); + let credentials = git_credentials(&token); + assert_eq!(credentials.username, GITHUB_TOKEN_USERNAME); + assert_eq!(credentials.password, "ghp_static"); } } diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 4dc99ad02..932c0e3e2 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -19,7 +19,7 @@ use std::time::{Duration, Instant}; use async_trait::async_trait; use fabro_github::GitHubCredentials; -use fabro_github::token_source::InstallationTokenSource; +use fabro_github::token_source::{InstallationTokenSource, TokenSnapshot}; use fabro_types::SandboxProviderKind; use fabro_util::workspace_glob::WorkspaceGlob; use sandbox_driver::{ @@ -35,9 +35,9 @@ use tokio_util::sync::CancellationToken; use crate::clone::{self, GitHubClone}; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; +use crate::credentials::{self, RepoCredentials}; use crate::environment::CloneRequest; -use crate::push_credentials::{self, PushCredentialState}; -use crate::{GitRunInfo, GitSetupIntent, RefreshOutcome, RetryPlan}; +use crate::{GitRunInfo, GitSetupIntent, RetryPlan}; /// A sandbox on the worker host at `working_directory`, the fabro `local` /// kind, served by the driver's in-process Host provider. @@ -118,7 +118,7 @@ enum WorkspacePlan { pub(crate) struct RepoWorkspace { layout: OnceLock, plan: WorkspacePlan, - credentials: PushCredentialState, + credentials: RepoCredentials, repo_cloned: OnceLock, origin_url: OnceLock, /// The directory the run works in once known: the repository link for a @@ -145,7 +145,7 @@ impl RepoWorkspace { clone.tag.as_deref(), clone.commit_sha.as_deref(), )?; - let credentials = PushCredentialState::new(push_credentials::build_token_source( + let credentials = RepoCredentials::new(credentials::build_token_source( github_app, clone.origin_url.as_deref(), )?); @@ -177,7 +177,7 @@ impl RepoWorkspace { /// A workspace prepared by an earlier process, described by the run /// record. Pushes from a reattached sandbox use whatever credentials the - /// checkout's `origin` already carries. + /// checkout's credential store already carries. pub(crate) fn attached( layout: LayoutSource, repo_cloned: bool, @@ -187,7 +187,7 @@ impl RepoWorkspace { let workspace = Self { layout: layout.into_cell(), plan: WorkspacePlan::Attached, - credentials: PushCredentialState::new(None), + credentials: RepoCredentials::none(), repo_cloned: OnceLock::new(), origin_url: OnceLock::new(), execution_directory: OnceLock::new(), @@ -1085,11 +1085,7 @@ impl RunSandbox { if !workspace.repo_cloned() { return Ok(PushReport::default()); } - let credentials = workspace - .origin_url - .get() - .map(|origin_url| (&workspace.credentials, origin_url.as_str())); - sandbox::git_push(self, credentials, refspec, plan).await + sandbox::git_push(self, Some(&workspace.credentials), refspec, plan).await } pub fn origin_url(&self) -> Option<&str> { @@ -1100,23 +1096,24 @@ impl RunSandbox { workspace.origin_url.get().map(String::as_str) } + /// Renew the credentials the agent's own git commands read for the + /// checkout: resolve the current token and rewrite the checkout's + /// credential store with it. Returns the token's non-secret description, + /// or `None` when this sandbox has no managed credentials or no + /// checkout to install them in. #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] - pub async fn refresh_push_credentials(&self) -> crate::Result { + pub async fn refresh_ambient_credentials(&self) -> crate::Result> { let Some(workspace) = &self.workspace else { - return Ok(RefreshOutcome::none()); + return Ok(None); }; - if !workspace.repo_cloned() { - return Ok(RefreshOutcome::none()); - } - let Some(origin_url) = workspace.origin_url.get() else { - return Ok(RefreshOutcome::none()); + let Some(checkout) = workspace.checkout_path.get() else { + return Ok(None); }; - workspace - .credentials - .refresh(origin_url, |auth_url| { - push_credentials::set_auth_url_via_exec(self, auth_url) - }) - .await + let Some(token) = workspace.credentials.resolve().await? else { + return Ok(None); + }; + RepoCredentials::install(&self.git()?, checkout, &token).await?; + Ok(Some(token.snapshot)) } pub fn push_token_source(&self) -> Option> { diff --git a/lib/components/fabro-sandbox/src/exec.rs b/lib/components/fabro-sandbox/src/exec.rs index 463f9c7c0..49b5d9266 100644 --- a/lib/components/fabro-sandbox/src/exec.rs +++ b/lib/components/fabro-sandbox/src/exec.rs @@ -271,14 +271,6 @@ pub trait ExecResultExt { /// carries only the label and the classified metadata. fn into_exec_error(self, label: impl Into) -> crate::Error; - /// [`Self::into_exec_error`] with `redactor` applied to both streams - /// first, for output that can carry a credentialed URL. - fn into_exec_error_with_redactor( - self, - label: impl Into, - redactor: impl Fn(&str) -> String, - ) -> crate::Error; - /// `Ok(self)` for a clean exit, the failure under `label` otherwise. fn into_result(self, label: impl Into) -> crate::Result; } @@ -316,16 +308,6 @@ impl ExecResultExt for ExecResult { crate::Error::driver_error(failure.into()) } - fn into_exec_error_with_redactor( - mut self, - label: impl Into, - redactor: impl Fn(&str) -> String, - ) -> crate::Error { - self.stdout = redactor(&self.stdout_lossy()).into_bytes(); - self.stderr = redactor(&self.stderr_lossy()).into_bytes(); - self.into_exec_error(label) - } - fn into_result(self, label: impl Into) -> crate::Result { if self.success() { Ok(self) @@ -730,23 +712,6 @@ mod tests { assert!(ok.into_result("true").is_ok()); } - #[test] - fn redactor_applies_to_stderr_and_stdout() { - let result = exec_result( - "stdout https://token@example.com", - "stderr https://token@example.com", - Some(1), - Termination::Exited, - 1, - ); - let error = result.into_exec_error_with_redactor("git set-url", |s| { - s.replace("https://token@example.com", "https://****@example.com") - }); - let failure = error.exec_failure().expect("exec failure"); - assert_eq!(failure.stderr(), b"stderr https://****@example.com"); - assert_eq!(failure.stdout(), b"stdout https://****@example.com"); - } - #[test] fn output_tail_redacts_before_truncating() { let secret = "sk-ant-api03-xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index 268b3c94c..dae6df64e 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -10,9 +10,7 @@ mod git_retry; mod managed_labels; -mod push_credentials; - -pub mod redact; +mod credentials; pub mod details; @@ -51,15 +49,14 @@ pub use git_retry::{ }; pub use provider::{SandboxInventory, SandboxLookupError}; pub use provider_sandbox::{attach_provider_sandbox, provider_sandbox}; -pub use push_credentials::RefreshErrorKind; pub use reconnect::{ open_terminal_for_run, reconnect, reconnect_driver_for_run, reconnect_for_run, reconnect_for_run_with_events, }; pub use sandbox::{ DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport, - RefreshOutcome, RemoteCredentialAction, SandboxFile, SandboxWorkspaceLayout, - format_lines_numbered, redacted_output_tail, setup_git, shell_quote, + SandboxFile, SandboxWorkspaceLayout, format_lines_numbered, redacted_output_tail, setup_git, + shell_quote, }; /// Driver types a run sandbox speaks: what a command is and how it ended, /// what the file and search operations return, and what an environment diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 07fdf9805..8acc2419d 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -2,16 +2,15 @@ use std::fmt::Write; use std::time::Duration; use fabro_github::token_source::TokenSnapshot; -pub use fabro_types::run_event::GitCredentialAction as RemoteCredentialAction; use fabro_util::shell; -use sandbox_driver::{Git as _, GitCheckoutOptions, GitFailureKind, GitPushOptions, Termination}; +use sandbox_driver::{Git as _, GitCheckoutOptions, GitPushOptions, Termination}; use serde::{Deserialize, Serialize}; use tokio::time; +use crate::credentials::{self, RepoCredentials}; use crate::driver_sandbox::RunSandbox; use crate::exec::ExecResultExt; use crate::git_retry::{self, CredentialContext, GitRetryReason, RetryPlan}; -use crate::push_credentials::{CredentialLease, PushCredentialState, RefreshErrorKind}; /// Git command prefix that disables background maintenance. pub(crate) const GIT: &str = "git -c maintenance.auto=0 -c gc.auto=0"; @@ -158,56 +157,6 @@ pub struct SandboxFile { pub size: u64, } -/// Outcome of -/// [`RunSandbox::refresh_push_credentials`](crate::RunSandbox::refresh_push_credentials): -/// what this call did to the remote, and the non-secret description of the -/// token embedded in it. `token` is `None` only when `action` is -/// [`RemoteCredentialAction::None`]. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum RefreshOutcome { - /// No managed credentials exist for this sandbox. - None, - /// The remote already carried this token generation. - Unchanged(TokenSnapshot), - /// The remote was updated to carry this token generation. - Embedded(TokenSnapshot), -} - -impl RefreshOutcome { - /// No managed credentials to refresh. - #[must_use] - pub const fn none() -> Self { - Self::None - } - - #[must_use] - pub const fn unchanged(token: TokenSnapshot) -> Self { - Self::Unchanged(token) - } - - #[must_use] - pub const fn embedded(token: TokenSnapshot) -> Self { - Self::Embedded(token) - } - - #[must_use] - pub const fn action(self) -> RemoteCredentialAction { - match self { - Self::None => RemoteCredentialAction::None, - Self::Unchanged(_) => RemoteCredentialAction::Unchanged, - Self::Embedded(_) => RemoteCredentialAction::Embedded, - } - } - - #[must_use] - pub const fn token(self) -> Option { - match self { - Self::None => None, - Self::Unchanged(token) | Self::Embedded(token) => Some(token), - } - } -} - pub(crate) fn resolve_path(path: &str, working_dir: &str) -> String { if std::path::Path::new(path).is_absolute() { path.to_string() @@ -337,21 +286,18 @@ pub(crate) async fn fetch_source_run_ref( #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PushAttempt { /// 1-based attempt number within this operation. - pub attempt: u32, - pub started_at: chrono::DateTime, - pub success: bool, + pub attempt: u32, + pub started_at: chrono::DateTime, + pub success: bool, /// The classifier's verdict for a failed attempt — recorded on the /// terminal attempt too; whether a retry actually followed is positional /// (every entry except the last). - pub retry_reason: Option, + pub retry_reason: Option, /// Redacted, bounded output tail; failed attempts only. - pub exec_output_tail: Option, - /// The token embedded in the remote during this attempt. - pub token: Option, - /// What `ensure_embedded` did to the remote this attempt. - pub credential_action: Option, - /// A mint or `set-url` failure this attempt pushed through. - pub refresh_error: Option, + pub exec_output_tail: Option, + /// The token this attempt pushed with; `None` without managed + /// credentials. + pub token: Option, } /// The attempt history of one push operation. @@ -387,30 +333,17 @@ fn classify_push_error(error: &crate::Error, cred: CredentialContext) -> Option< git_retry::classify_driver_failure(driver, cred) } -/// Whether a failed push attempt was rejected as unauthenticated, the shape -/// a drifted or missing embedded token also produces. -fn push_failure_looks_auth_shaped(error: &crate::Error) -> bool { - matches!( - error.driver(), - Some(sandbox_driver::Error::Git(failure)) if failure.kind() == GitFailureKind::AuthRejected - ) -} - /// Pushes a refspec to origin through the driver's git facet, retrying per -/// `plan` with one pinned credential generation for the whole operation. -/// `credentials` is the provider's push-credential state plus the origin -/// URL; `None` pushes with whatever the remote already carries (the local -/// sandbox, or a workspace without managed credentials). +/// `plan` with one token for the whole operation. `credentials` is the +/// checkout's managed credentials; `None` pushes with whatever the checkout +/// already has (the local sandbox, or a workspace without a GitHub App). #[tracing::instrument(name = "git_op", skip_all, fields(op = "push"))] pub(crate) async fn git_push( sandbox: &RunSandbox, - credentials: Option<(&PushCredentialState, &str)>, + credentials: Option<&RepoCredentials>, refspec: &str, plan: &RetryPlan, ) -> Result { - use CredentialContext; - use CredentialLease; - let start = time::Instant::now(); let deadline = plan.effective_deadline(start); let git = match sandbox.git() { @@ -424,37 +357,38 @@ pub(crate) async fn git_push( }; let repo = sandbox.working_directory().to_owned(); - // The lease pins one token generation and owns the embed mutex for the - // whole operation; no concurrent refresh can re-embed mid-operation, and - // no attempt can cross the refresh margin and restart the replication - // clock. - let mut lease: Option<(CredentialLease<'_>, &str)> = match credentials { - Some((state, origin_url)) => match match deadline { - Some(deadline) => match time::timeout_at(deadline, state.lease()).await { - Ok(result) => result, - Err(_) => { - return Err(push_deadline_error( - Vec::new(), - "while acquiring credentials", - )); + // One token for the whole operation. A retry after replication lag must + // present the same token, because replication of a given token only + // makes progress, and a fresh mint would restart that clock. + let token = match credentials { + Some(credentials) => { + let resolved = match deadline { + Some(deadline) => match time::timeout_at(deadline, credentials.resolve()).await { + Ok(resolved) => resolved, + Err(_) => { + return Err(push_deadline_error( + Vec::new(), + "while acquiring credentials", + )); + } + }, + None => credentials.resolve().await, + }; + match resolved { + Ok(token) => token, + Err(error) => { + return Err(PushError { + report: PushReport::default(), + error, + }); } - }, - None => state.lease().await, - } { - Ok(lease) => Some((lease, origin_url)), - Err(error) => { - return Err(PushError { - report: PushReport::default(), - error, - }); } - }, + } None => None, }; + let snapshot = token.as_ref().map(|token| token.snapshot); let mut attempts: Vec = Vec::new(); - let mut force_reembed = false; - let mut drift_repaired = false; let label = format!("git push origin {refspec}"); loop { @@ -466,43 +400,11 @@ pub(crate) async fn git_push( if attempt_timeout.is_zero() { return Err(push_deadline_error(attempts, "before the next attempt")); } - let attempt_deadline = time::Instant::now() + attempt_timeout; - let (token, credential_action, refresh_error) = match lease.as_mut() { - Some((lease, origin_url)) => { - let ensured = match time::timeout_at( - attempt_deadline, - lease.ensure_embedded(sandbox, origin_url, force_reembed), - ) - .await - { - Ok(Ok(ensured)) => ensured, - Ok(Err(error)) => { - return Err(PushError { - report: PushReport { attempts }, - error, - }); - } - Err(_) => { - return Err(push_deadline_error( - attempts, - "while refreshing credentials", - )); - } - }; - force_reembed = false; - (ensured.token, Some(ensured.action), ensured.refresh_error) - } - None => (None, None, None), - }; - - let remaining = attempt_deadline.saturating_duration_since(time::Instant::now()); - if remaining.is_zero() { - return Err(push_deadline_error(attempts, "before running git push")); - } let mut options = GitPushOptions::default(); options.remote = Some("origin".to_owned()); options.refspec = Some(refspec.to_owned()); - options.timeout = Some(remaining); + options.timeout = Some(attempt_timeout); + options.credentials = token.as_ref().map(credentials::git_credentials); let push_result = git .push(&repo, &options) .await @@ -516,29 +418,19 @@ pub(crate) async fn git_push( success: true, retry_reason: None, exec_output_tail: None, - token, - credential_action, - refresh_error, + token: snapshot, }); tracing::info!( refspec = %refspec, attempt = attempt_number, - token_generation = token.map(|token| token.generation), - token_age_ms = token.and_then(|token| token.age_ms()), + token_generation = snapshot.map(|token| token.generation), + token_age_ms = snapshot.and_then(|token| token.age_ms()), "Pushed git ref to origin" ); return Ok(PushReport { attempts }); } Err(error) => { - // Drift recovery: the tracked generation is local belief, and - // agent code inside the sandbox can rewrite `origin`. The - // first auth/not-found failure earns one forced re-embed of - // the pinned token, inside the same retry budget. - if !drift_repaired && lease.is_some() && push_failure_looks_auth_shaped(&error) { - drift_repaired = true; - force_reembed = true; - } - let cred = CredentialContext::from_snapshot(token.as_ref()); + let cred = CredentialContext::from_snapshot(snapshot.as_ref()); let retry_reason = classify_push_error(&error, cred); attempts.push(PushAttempt { attempt: attempt_number, @@ -546,9 +438,7 @@ pub(crate) async fn git_push( success: false, retry_reason, exec_output_tail: error.default_redacted_output_tail(), - token, - credential_action, - refresh_error, + token: snapshot, }); let exhausted = attempt_number >= plan.max_attempts.max(1); @@ -571,8 +461,8 @@ pub(crate) async fn git_push( attempt = attempt_number, max_attempts = plan.max_attempts, reason = %reason, - token_generation = token.map(|token| token.generation), - token_age_ms = token.and_then(|token| token.age_ms()), + token_generation = snapshot.map(|token| token.generation), + token_age_ms = snapshot.and_then(|token| token.age_ms()), delay_ms = u64::try_from(delay.as_millis()).unwrap_or(u64::MAX), "Git push failed, retrying with the same token" ); @@ -606,8 +496,8 @@ mod push_tests { use tokio::sync::Mutex as AsyncMutex; use super::*; + use crate::credentials::RepoCredentials; use crate::git_retry::{GitRetryReason, RetryPlan}; - use crate::push_credentials::{PushCredentialState, RefreshErrorKind}; const ORIGIN: &str = "https://github.com/fabro-testing/repo"; const REFSPEC: &str = "refs/heads/fabro/run/01M0DH033P2XSTHAGVBHG6922F"; @@ -628,9 +518,9 @@ mod push_tests { result } - /// A run sandbox over a scripted driver double: `git push` answers come - /// from a script, `git remote set-url` succeeds unless scripted - /// otherwise, and every command is recorded. + /// A run sandbox over a scripted driver double. The driver's push reads + /// `origin`'s URL when it carries credentials and then runs `git push`; + /// push answers come from a script, and every command is recorded. struct ScriptedGitSandbox { run: RunSandbox, driver: Arc, @@ -638,23 +528,17 @@ mod push_tests { impl ScriptedGitSandbox { fn new(push_results: Vec) -> Self { - Self::with_set_url_results(push_results, Vec::new()) - } - - fn with_set_url_results( - push_results: Vec, - set_url_results: Vec, - ) -> Self { let driver = Arc::new(ScriptedSandbox::with_id_and_working_dir( "scripted-git", "/workspace", )); let pushes = Mutex::new(VecDeque::from(push_results)); - let set_urls = Mutex::new(VecDeque::from(set_url_results)); driver.scripted_exec().respond_with(move |spec| { let script = spec.args.last().map(String::as_str).unwrap_or_default(); - if script.contains("remote set-url") { - return Some(set_urls.lock().unwrap().pop_front().unwrap_or_else(ok_exec)); + if script.contains("'remote' 'get-url' 'origin'") { + let mut url = ok_exec(); + url.stdout = format!("{ORIGIN}\n").into_bytes(); + return Some(url); } assert!( script.contains("'push' 'origin'"), @@ -676,17 +560,28 @@ mod push_tests { self.driver.scripted_exec().commands() } - fn push_count(&self) -> usize { - self.commands() - .iter() - .filter(|command| command.contains("'push' 'origin'")) - .count() - } - - fn set_url_commands(&self) -> Vec { + /// The `git push` commands that ran, in order. + fn pushes(&self) -> Vec { self.commands() .into_iter() - .filter(|command| command.contains("remote set-url")) + .filter(|command| command.contains("'push' 'origin'")) + .collect() + } + + fn push_count(&self) -> usize { + self.pushes().len() + } + + /// The token each push carried in its per-call rewrite; `None` for + /// a push without credentials. + fn push_tokens(&self) -> Vec> { + self.pushes() + .iter() + .map(|push| { + let start = push.find("x-access-token:")? + "x-access-token:".len(); + let end = push[start..].find('@')? + start; + Some(push[start..end].to_owned()) + }) .collect() } } @@ -702,8 +597,8 @@ mod push_tests { } impl ScriptedMinter { - fn new(script: Vec) -> std::sync::Arc { - std::sync::Arc::new(Self { + fn new(script: Vec) -> Arc { + Arc::new(Self { calls: AtomicUsize::new(0), script: AsyncMutex::new(script.into()), }) @@ -741,25 +636,23 @@ mod push_tests { } } - fn minting_state( - script: Vec, - ) -> (PushCredentialState, std::sync::Arc) { + fn minting_credentials(script: Vec) -> (RepoCredentials, Arc) { let minter = ScriptedMinter::new(script); let source = installation_token_source( "fabro-testing/repo", - std::sync::Arc::clone(&minter) as std::sync::Arc, + Arc::clone(&minter) as Arc, ); - (PushCredentialState::new(Some(source)), minter) + (RepoCredentials::new(Some(source)), minter) } - async fn seed_clone_token(state: &PushCredentialState) { - let clone_token = state - .source() - .expect("state has a source") + /// Mint the clone token first, the way `initialize` does, so the push + /// resolves the cached token instead of minting one. + async fn seed_clone_token(credentials: &RepoCredentials) { + credentials .mint_for_clone() .await - .expect("clone mint succeeds"); - state.record_embedded(clone_token).await; + .expect("clone mint succeeds") + .expect("managed credentials mint"); } /// Regression for run `01M0DH033P2XSTHAGVBHG6922F` (the push variant of @@ -769,7 +662,7 @@ mod push_tests { /// token only makes progress — and recover inside the plan's budget. #[tokio::test(start_paused = true)] async fn push_not_found_after_a_successful_mint_is_retried_with_the_same_token() { - let (state, minter) = minting_state(vec![MintAction::Token( + let (credentials, minter) = minting_credentials(vec![MintAction::Token( "ghs_gen1", chrono::Duration::minutes(60), )]); @@ -781,7 +674,7 @@ mod push_tests { let report = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::checkpoint_push(), ) @@ -798,21 +691,20 @@ mod push_tests { Some(GitRetryReason::TokenReplication) ); assert!(report.attempts[0].exec_output_tail.is_some()); - assert_eq!( - report.attempts[0].credential_action, - Some(RemoteCredentialAction::Embedded), - "first attempt embeds the resolved token" - ); assert!(report.attempts[2].success); assert!(report.attempts[2].exec_output_tail.is_none()); - assert_eq!(sandbox.push_count(), 3); + assert_eq!( + sandbox.push_tokens(), + vec![Some("ghs_gen1".to_owned()); 3], + "every attempt presents the same token" + ); } /// The publish plan gives the terminal push a real budget: four /// replication-lag failures still recover on the fifth attempt. #[tokio::test(start_paused = true)] async fn publish_plan_survives_four_not_found_failures() { - let (state, minter) = minting_state(vec![MintAction::Token( + let (credentials, minter) = minting_credentials(vec![MintAction::Token( "ghs_gen1", chrono::Duration::minutes(60), )]); @@ -826,7 +718,7 @@ mod push_tests { let report = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::publish_push(), ) @@ -845,10 +737,10 @@ mod push_tests { #[tokio::test(start_paused = true)] async fn token_resolved_just_above_the_margin_stays_pinned_through_retries() { let ttl = REFRESH_MARGIN + Duration::from_secs(5); - let (state, minter) = minting_state(vec![MintAction::Token( - "ghs_gen1", - chrono::Duration::from_std(ttl).unwrap(), - )]); + let (credentials, minter) = minting_credentials(vec![ + MintAction::Token("ghs_gen1", chrono::Duration::from_std(ttl).unwrap()), + MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), + ]); let sandbox = ScriptedGitSandbox::new(vec![ failed_exec("remote: Repository not found."), failed_exec("remote: Repository not found."), @@ -857,39 +749,32 @@ mod push_tests { let report = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::checkpoint_push(), ) .await - .expect("push should recover"); + .expect("push recovers"); - assert_eq!(minter.calls(), 1, "no mid-operation mint"); - let generations: Vec = report - .attempts - .iter() - .map(|attempt| attempt.token.expect("token recorded").generation) - .collect(); - assert_eq!(generations, vec![1, 1, 1]); + assert_eq!(minter.calls(), 1, "the operation never re-resolves"); + assert_eq!(sandbox.push_tokens(), vec![Some("ghs_gen1".to_owned()); 3]); + assert!( + report + .attempts + .iter() + .all(|attempt| attempt.token.map(|token| token.generation) == Some(1)) + ); } #[tokio::test(start_paused = true)] async fn static_credential_auth_failure_fails_fast() { - let source = InstallationTokenSource::for_origin( - &fabro_github::GitHubCredentials::Pat("ghp_pat".to_string()), - ORIGIN, - serde_json::json!({ "contents": "write" }), - ) - .unwrap(); - let state = PushCredentialState::new(Some(source)); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![failed_exec( - "fatal: Authentication failed for 'https://github.com/fabro-testing/repo'", - )]); + let credentials = + RepoCredentials::new(Some(InstallationTokenSource::pat("ghp_static".to_owned()))); + let sandbox = ScriptedGitSandbox::new(vec![failed_exec("remote: Repository not found.")]); let push_error = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::publish_push(), ) @@ -898,221 +783,114 @@ mod push_tests { assert_eq!(push_error.report.attempts.len(), 1); assert_eq!(push_error.report.attempts[0].retry_reason, None); - assert!(push_error.report.attempts[0].token.unwrap().is_static()); + assert_eq!( + push_error.report.attempts[0] + .token + .map(|token| token.generation), + Some(0) + ); + assert_eq!(sandbox.push_tokens(), vec![Some("ghp_static".to_owned())]); } - /// Clone seeding closes the "nothing was ever embedded" hole: when the - /// first refresh mint fails, the push falls back to the clone token - /// recorded as last-embedded instead of aborting. + /// A refresh that fails while the cached token is still valid pushes + /// with the cached token. #[tokio::test(start_paused = true)] - async fn mint_failure_falls_back_to_the_clone_token() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_clone", chrono::Duration::minutes(5)), - // The clone token is inside the margin, so lease acquisition - // re-mints and fails. - MintAction::Error("mint failed"), + async fn mint_failure_falls_back_to_the_cached_token() { + // The clone token is already inside the refresh margin, so the + // push's resolve tries to re-mint and fails. + let (credentials, minter) = minting_credentials(vec![ + MintAction::Token( + "ghs_clone", + chrono::Duration::from_std( + REFRESH_MARGIN + .checked_sub(Duration::from_mins(1)) + .expect("the margin is longer than a minute"), + ) + .unwrap(), + ), + MintAction::Error("github unavailable"), ]); - seed_clone_token(&state).await; + seed_clone_token(&credentials).await; let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); let report = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::checkpoint_push(), ) .await - .expect("push proceeds with the still-valid clone token"); + .expect("the cached token still pushes"); - assert_eq!(minter.calls(), 2); - let attempt = &report.attempts[0]; - assert!(attempt.success); - assert_eq!(attempt.refresh_error, Some(RefreshErrorKind::Mint)); + assert_eq!(minter.calls(), 2, "the push tried to refresh once"); + assert_eq!(sandbox.push_tokens(), vec![Some("ghs_clone".to_owned())]); assert_eq!( - attempt.token.expect("fallback token recorded").generation, - 1, - "attempts classify against the embedded clone token, never None" - ); - assert_eq!( - attempt.credential_action, - Some(RemoteCredentialAction::Unchanged) + report.attempts[0].token.map(|token| token.generation), + Some(1) ); } #[tokio::test(start_paused = true)] - async fn acquisition_fails_when_mint_fails_and_nothing_was_embedded() { - let (state, _minter) = minting_state(vec![MintAction::Error("mint failed")]); + async fn mint_failure_without_a_cached_token_fails_before_any_push() { + let (credentials, minter) = + minting_credentials(vec![MintAction::Error("github unavailable")]); let sandbox = ScriptedGitSandbox::new(vec![]); let push_error = git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::checkpoint_push(), ) .await - .expect_err("there is nothing to push with"); + .expect_err("no token to push with"); assert!(push_error.report.attempts.is_empty()); - assert!(push_error.error.to_string().contains("token_mint_failed")); assert_eq!(sandbox.push_count(), 0); - } - - /// Late-mint recovery: the fallback push fails on the expired-ish old - /// token, a later attempt's resolve retry succeeds, the target embeds, - /// and the push recovers — all inside one operation's budget. - #[tokio::test(start_paused = true)] - async fn late_mint_recovery_lands_the_target_inside_the_operation() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::minutes(5)), - MintAction::Error("mint failed"), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec("fatal: Authentication failed for 'https://github.com'"), - ok_exec(), - ]); - - let report = git_push( - &sandbox.run, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("late mint should recover the push"); - - assert_eq!(minter.calls(), 3); - let first = &report.attempts[0]; - assert_eq!(first.refresh_error, Some(RefreshErrorKind::Mint)); - assert_eq!(first.token.unwrap().generation, 1); - let second = &report.attempts[1]; - assert!(second.success); - assert_eq!(second.refresh_error, None); - assert_eq!(second.token.unwrap().generation, 2); - assert_eq!( - second.credential_action, - Some(RemoteCredentialAction::Embedded), - "the report shows the single generation transition" + assert_eq!(minter.calls(), 1); + assert!( + push_error + .error + .to_string() + .contains("Failed to refresh GitHub App credentials"), + "{}", + push_error.error ); } - /// A failed `set-url` defers the embed: attempt 1 records the old - /// generation with the refresh error, attempt 2 lands the target, and the - /// report shows the one generation transition via `credential_action`. + /// The token reaches git through the driver's per-call rewrite and never + /// through the remote URL. #[tokio::test(start_paused = true)] - async fn set_url_failure_defers_the_embed_until_the_next_attempt() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::minutes(5)), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::with_set_url_results( - vec![ - failed_exec("error: RPC failed; connection reset by peer"), - ok_exec(), - ], - vec![failed_exec("error: could not lock config file")], - ); - - let report = git_push( - &sandbox.run, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect("deferred embed should land on the retry"); - - assert_eq!( - minter.calls(), - 2, - "the successful resolve is never repeated" - ); - let first = &report.attempts[0]; - assert_eq!(first.refresh_error, Some(RefreshErrorKind::SetUrl)); - assert_eq!( - first.token.unwrap().generation, - 1, - "pin stays on the old token" - ); - assert_eq!( - first.credential_action, - Some(RemoteCredentialAction::Unchanged) - ); - let second = &report.attempts[1]; - assert_eq!(second.token.unwrap().generation, 2); - assert_eq!( - second.credential_action, - Some(RemoteCredentialAction::Embedded) - ); - assert!(second.success); - } - - #[tokio::test(start_paused = true)] - async fn timed_out_set_url_stops_before_push_while_it_may_still_run() { - let (state, minter) = minting_state(vec![ - MintAction::Token("ghs_gen1", chrono::Duration::minutes(5)), - MintAction::Token("ghs_gen2", chrono::Duration::minutes(60)), - ]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::with_set_url_results(vec![], vec![timed_out_exec()]); - - let push_error = git_push( - &sandbox.run, - Some((&state, ORIGIN)), - REFSPEC, - &RetryPlan::checkpoint_push(), - ) - .await - .expect_err("a timed-out set-url can still rewrite origin later"); - - assert_eq!(minter.calls(), 2); - assert!(push_error.report.attempts.is_empty()); - assert_eq!(sandbox.push_count(), 0); - } - - /// Remote drift: agent code rewrote `origin`, so the push fails on auth - /// even though the tracked generation looks current. The first - /// auth-shaped failure earns one forced re-embed of the pinned token. - #[tokio::test(start_paused = true)] - async fn remote_drift_gets_one_forced_reembed_of_the_pinned_token() { - let (state, minter) = minting_state(vec![MintAction::Token( + async fn credentials_travel_per_call_and_never_touch_the_remote() { + let (credentials, _minter) = minting_credentials(vec![MintAction::Token( "ghs_gen1", chrono::Duration::minutes(60), )]); - seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ - failed_exec( - "fatal: could not read Username for 'https://github.com': No such device or address\nremote: Repository not found.", - ), - ok_exec(), - ]); + let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); - let report = git_push( + git_push( &sandbox.run, - Some((&state, ORIGIN)), + Some(&credentials), REFSPEC, &RetryPlan::checkpoint_push(), ) .await - .expect("drift repair should restore the pinned credentials"); + .expect("push succeeds"); - assert_eq!(minter.calls(), 1, "drift repair re-embeds, never re-mints"); - assert_eq!( - report.attempts[0].credential_action, - Some(RemoteCredentialAction::Unchanged), - "before the failure the tracked generation matched" + let commands = sandbox.commands(); + assert!( + commands.iter().all(|command| !command.contains("set-url")), + "{commands:#?}" ); - assert_eq!( - report.attempts[1].credential_action, - Some(RemoteCredentialAction::Embedded), - "the retry force-re-embeds the pinned token" + let push = &sandbox.pushes()[0]; + assert!( + push.contains("insteadOf=https://github.com/fabro-testing/repo"), + "{push}" + ); + assert!( + push.contains("'push' 'origin' 'refs/heads/fabro/run/"), + "{push}" ); - let set_urls = sandbox.set_url_commands(); - assert_eq!(set_urls.len(), 1); - assert!(set_urls[0].contains("ghs_gen1")); } #[tokio::test(start_paused = true)] @@ -1125,7 +903,7 @@ mod push_tests { assert_eq!(report.attempts.len(), 1); assert_eq!(report.attempts[0].token, None); - assert_eq!(report.attempts[0].credential_action, None); + assert_eq!(sandbox.push_tokens(), vec![None]); } #[tokio::test(start_paused = true)] @@ -1156,16 +934,16 @@ mod push_tests { } #[tokio::test(start_paused = true)] - async fn retry_deadline_includes_credential_lease_acquisition() { + async fn retry_deadline_includes_credential_resolution() { let source = installation_token_source("fabro-testing/repo", Arc::new(SlowMinter)); - let state = PushCredentialState::new(Some(source)); + let credentials = RepoCredentials::new(Some(source)); let sandbox = ScriptedGitSandbox::new(vec![]); let mut plan = RetryPlan::checkpoint_push(); plan.max_elapsed = Some(Duration::from_secs(1)); - let push_error = git_push(&sandbox.run, Some((&state, ORIGIN)), REFSPEC, &plan) + let push_error = git_push(&sandbox.run, Some(&credentials), REFSPEC, &plan) .await - .expect_err("credential acquisition must stop at the operation deadline"); + .expect_err("credential resolution must stop at the operation deadline"); assert!(push_error.report.attempts.is_empty()); assert_eq!(sandbox.push_count(), 0); diff --git a/lib/components/fabro-workflow/src/event/convert.rs b/lib/components/fabro-workflow/src/event/convert.rs index 9daeb097c..14246d6d7 100644 --- a/lib/components/fabro-workflow/src/event/convert.rs +++ b/lib/components/fabro-workflow/src/event/convert.rs @@ -58,8 +58,6 @@ fn git_push_attempt_props( .token .and_then(|token| token.age_at(attempt.started_at)) .map(|age| u64::try_from(age.as_millis()).unwrap_or(u64::MAX)), - credential_action: attempt.credential_action, - refresh_error: attempt.refresh_error, }) .collect() } @@ -2213,26 +2211,22 @@ mod tests { expires_at, }, }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Embedded), - refresh_error: None, }, // Terminal classified failure with a refresh error: the last // attempt carries its classification too. fabro_sandbox::PushAttempt { - attempt: 2, - started_at: started_at + chrono::Duration::seconds(3), - success: false, - retry_reason: Some(fabro_sandbox::GitRetryReason::TransientInfra), - exec_output_tail: Some(exec_tail()), - token: Some(fabro_sandbox::TokenSnapshot { + attempt: 2, + started_at: started_at + chrono::Duration::seconds(3), + success: false, + retry_reason: Some(fabro_sandbox::GitRetryReason::TransientInfra), + exec_output_tail: Some(exec_tail()), + token: Some(fabro_sandbox::TokenSnapshot { generation: 14, provenance: fabro_sandbox::TokenProvenance::Reused { minted_at, expires_at, }, }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Unchanged), - refresh_error: Some(fabro_sandbox::RefreshErrorKind::SetUrl), }, ]; let expected_attempts = git_push_attempt_props(&runtime_attempts); @@ -2251,11 +2245,8 @@ mod tests { assert_eq!(serialized[0]["token_generation"], 14); assert_eq!(serialized[0]["token_provenance"], "minted"); assert_eq!(serialized[0]["token_age_ms"], 180); - assert_eq!(serialized[0]["credential_action"], "embedded"); - assert!(serialized[0].get("refresh_error").is_none()); assert_eq!(serialized[1]["classified_reason"], "transient_infra"); assert_eq!(serialized[1]["token_provenance"], "reused"); - assert_eq!(serialized[1]["refresh_error"], "set_url"); // The provenance enum never nests in stored events. assert!(serialized[0].get("token").is_none()); @@ -2269,20 +2260,43 @@ mod tests { } } + /// Attempts stored by earlier releases carried `credential_action` and + /// `refresh_error` from the origin-URL credential design. The fields are + /// gone; the stored events still read. + #[test] + fn stored_attempts_with_retired_credential_fields_still_deserialize() { + let json = serde_json::json!({ + "attempt": 1, + "started_at": "2026-03-30T12:00:01.000Z", + "success": true, + "token_generation": 3, + "token_provenance": "reused", + "token_age_ms": 120, + "credential_action": "embedded", + "refresh_error": "set_url" + }); + let props: ::fabro_types::run_event::GitPushAttemptProps = + serde_json::from_value(json).unwrap(); + assert_eq!(props.attempt, 1); + assert_eq!(props.token_generation, Some(3)); + assert_eq!( + props.token_provenance, + Some(::fabro_types::run_event::GitTokenProvenance::Reused) + ); + } + #[test] fn successful_single_attempt_push_omits_failure_fields() { let attempts = vec![fabro_sandbox::PushAttempt { - attempt: 1, - started_at: Utc::now(), - success: true, - retry_reason: None, - exec_output_tail: None, - token: Some(fabro_sandbox::TokenSnapshot { + attempt: 1, + started_at: Utc::now(), + success: true, + retry_reason: None, + exec_output_tail: None, + token: Some(fabro_sandbox::TokenSnapshot { generation: 0, provenance: fabro_sandbox::TokenProvenance::Static, }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Unchanged), - refresh_error: None, }]; let stored = to_run_event(&fixtures::RUN_1, &Event::GitPush { branch: "fabro/run/run-1".to_string(), @@ -2295,12 +2309,7 @@ mod tests { let attempt = &json["properties"]["attempts"][0]; assert_eq!(attempt["success"], true); assert_eq!(attempt["token_provenance"], "static"); - for absent in [ - "classified_reason", - "exec_output_tail", - "token_age_ms", - "refresh_error", - ] { + for absent in ["classified_reason", "exec_output_tail", "token_age_ms"] { assert!(attempt.get(absent).is_none(), "{absent} should be omitted"); } } diff --git a/lib/components/fabro-workflow/src/handler/llm/acp.rs b/lib/components/fabro-workflow/src/handler/llm/acp.rs index 6f42d4361..65f137668 100644 --- a/lib/components/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/components/fabro-workflow/src/handler/llm/acp.rs @@ -11,11 +11,10 @@ use fabro_acp::{ AcpCommandError, AcpControlHandle, AcpError, AcpLiveControl, AcpProcessSpec, AcpRunRequest, render_stop_reason, }; -use fabro_agent::{ - AgentEvent, RefreshOutcome, RunSandbox, StaticEnvProvider, SteeringItem, ToolEnvProvider, -}; +use fabro_agent::{AgentEvent, RunSandbox, StaticEnvProvider, SteeringItem, ToolEnvProvider}; use fabro_github::token_source::REFRESH_MARGIN; use fabro_graphviz::graph::Node; +use fabro_sandbox::TokenSnapshot; use fabro_static::EnvVars; use fabro_types::{ AgentBackend, Principal, SessionCapability, StageId, StageTiming, SteeringMessage, @@ -41,8 +40,9 @@ const REFRESH_INTERVAL_DEFAULT: Duration = Duration::from_mins(45); /// Floor for expiry-driven rescheduling, so a token already inside the cache /// margin cannot pin the loop in a hot cycle. const REFRESH_RESCHEDULE_FLOOR: Duration = Duration::from_secs(30); -/// Upper bound on a single push-credential refresh (token mint + `git remote -/// set-url` exec). The turn-entry refresh runs before the ACP process spawns +/// Upper bound on a single credential refresh (token mint + rewriting the +/// checkout's credential store). The turn-entry refresh runs before the ACP +/// process spawns /// and the ACP node uses `NodeTimeoutPolicy::HandlerManaged`, so without this /// bound a stalled GitHub API call would hang node entry indefinitely. const REFRESH_MINT_TIMEOUT: Duration = Duration::from_secs(30); @@ -114,10 +114,10 @@ fn push_cred_refresh_interval() -> Option { /// 45-minute sleep would leave the embedded token expired until the next /// tick. Schedule from the token's own `expires_at` instead: wake when the /// cache margin opens, so that tick re-mints. `None` disables the loop — -/// static credentials cannot be re-minted by waiting. -fn next_refresh_delay(outcome: &RefreshOutcome) -> Option { - let token = outcome.token()?; - let expires_at = token.expires_at()?; +/// static credentials cannot be re-minted by waiting, and a sandbox without +/// managed credentials has nothing to renew. +fn next_refresh_delay(token: Option<&TokenSnapshot>) -> Option { + let expires_at = token?.expires_at()?; let margin = chrono::Duration::from_std(REFRESH_MARGIN).unwrap_or(chrono::Duration::MAX); let until_margin = ((expires_at - margin) - chrono::Utc::now()) .to_std() @@ -125,11 +125,11 @@ fn next_refresh_delay(outcome: &RefreshOutcome) -> Option { Some(until_margin.max(REFRESH_RESCHEDULE_FLOOR)) } -/// Background loop that keeps the sandbox's push credentials fresh for the +/// Background loop that keeps the checkout's git credentials fresh for the /// duration of one ACP turn, so a single turn that outlives the /// installation-token TTL still pushes with a fresh token. Bounded by /// `cancel` (the drop-guard cancels it at turn end). Each successful tick -/// reschedules from the embedded token's expiry ([`next_refresh_delay`]); a +/// reschedules from the installed token's expiry ([`next_refresh_delay`]); a /// failed or timed-out tick retries after a shorter delay so a transient /// error does not leave a longer-than-interval window with an expired token. async fn refresh_ahead_loop( @@ -138,7 +138,7 @@ async fn refresh_ahead_loop( interval: Duration, initial_delay: Duration, ) where - Fut: Future> + Send, + Fut: Future>> + Send, { let retry_delay = interval.min(Duration::from_mins(1)); let mut delay = initial_delay; @@ -147,27 +147,21 @@ async fn refresh_ahead_loop( () = cancel.cancelled() => break, () = sleep(delay) => { match timeout(REFRESH_MINT_TIMEOUT, refresh()).await { - Ok(Ok(outcome)) => { - match outcome { - RefreshOutcome::Embedded(token) => { + Ok(Ok(token)) => { + match &token { + Some(token) => { tracing::info!( generation = token.generation, - "refresh-ahead re-embedded push credentials mid-turn" + "refresh-ahead renewed the checkout's git credentials mid-turn" ); } - RefreshOutcome::Unchanged(token) => { + None => { tracing::debug!( - generation = token.generation, - "refresh-ahead tick: embedded push credentials still fresh" - ); - } - RefreshOutcome::None => { - tracing::debug!( - "refresh-ahead tick: no managed push credentials to refresh" + "refresh-ahead tick: no managed git credentials to renew" ); } } - if let Some(next) = next_refresh_delay(&outcome) { + if let Some(next) = next_refresh_delay(token.as_ref()) { delay = next; } else { tracing::debug!( @@ -314,24 +308,15 @@ impl AgentAcpBackend { let refresh_enabled = push_cred_refresh_enabled(); let refresh_interval = refresh_enabled.then(push_cred_refresh_interval).flatten(); let refresh_schedule = if refresh_enabled { - match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_push_credentials()).await { - Ok(Ok(outcome)) => { - match outcome { - RefreshOutcome::Embedded(token) => { - tracing::debug!( - generation = token.generation, - "refreshed sandbox push credentials at ACP turn entry" - ); - } - RefreshOutcome::Unchanged(token) => { - tracing::debug!( - generation = token.generation, - "sandbox push credentials already fresh at ACP turn entry" - ); - } - RefreshOutcome::None => {} + match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_ambient_credentials()).await { + Ok(Ok(token)) => { + if let Some(token) = &token { + tracing::debug!( + generation = token.generation, + "refreshed the checkout's git credentials at ACP turn entry" + ); } - refresh_interval.zip(next_refresh_delay(&outcome)) + refresh_interval.zip(next_refresh_delay(token.as_ref())) } Ok(Err(e)) => { tracing::warn!( @@ -359,7 +344,7 @@ impl AgentAcpBackend { AbortOnDrop(tokio::spawn(refresh_ahead_loop( move || { let sandbox = Arc::clone(&sandbox); - async move { sandbox.refresh_push_credentials().await } + async move { sandbox.refresh_ambient_credentials().await } }, cancel_token.child_token(), interval, @@ -651,10 +636,7 @@ mod tests { use fabro_acp::test_support::fake_acp_agent_script; use fabro_acp::{AcpError, AcpProcessExit}; - use fabro_agent::{ - RefreshOutcome, RemoteCredentialAction, RunSandbox, TokenProvenance, TokenSnapshot, - local_sandbox, shell_quote, - }; + use fabro_agent::{RunSandbox, TokenProvenance, TokenSnapshot, local_sandbox, shell_quote}; use fabro_graphviz::graph::{AttrValue, Node}; use fabro_sandbox::test_support::MockSandbox; use fabro_types::{CommandTermination, EventBody, ExecOutputTail}; @@ -715,25 +697,21 @@ mod tests { } #[tokio::test] - async fn refresh_reports_no_action_without_managed_credentials() { - // A mock sandbox has no cloned workspace and so no managed push - // credentials: refresh is a no-op that must report no remote action - // and no token — the signal the refresh-ahead loop relies on to log - // at debug rather than falsely claim a re-embed. + async fn refresh_reports_no_token_without_managed_credentials() { + // A mock sandbox has no cloned workspace and so no managed + // credentials: refresh is a no-op that must report no token — the + // signal the refresh-ahead loop relies on to stop rather than claim + // a renewal. let sandbox = MockSandbox::linux().sandbox(); - assert_eq!( - sandbox.refresh_push_credentials().await.unwrap(), - RefreshOutcome::none() - ); + assert_eq!(sandbox.refresh_ambient_credentials().await.unwrap(), None); } - fn minted_outcome( - action: RemoteCredentialAction, + fn minted_token( generation: u64, minted_ago: chrono::Duration, expires_in: chrono::Duration, reused: bool, - ) -> RefreshOutcome { + ) -> TokenSnapshot { let now = chrono::Utc::now(); let minted_at = now - minted_ago; let expires_at = now + expires_in; @@ -748,34 +726,28 @@ mod tests { expires_at, } }; - let token = TokenSnapshot { + TokenSnapshot { generation, provenance, - }; - match action { - RemoteCredentialAction::Embedded => RefreshOutcome::embedded(token), - RemoteCredentialAction::Unchanged => RefreshOutcome::unchanged(token), - RemoteCredentialAction::None => RefreshOutcome::none(), } } - fn static_outcome() -> RefreshOutcome { - RefreshOutcome::unchanged(TokenSnapshot { + fn static_token() -> TokenSnapshot { + TokenSnapshot { generation: 0, provenance: TokenProvenance::Static, - }) + } } #[test] fn next_refresh_delay_schedules_from_token_expiry_minus_margin() { - let outcome = minted_outcome( - RemoteCredentialAction::Embedded, + let outcome = minted_token( 1, chrono::Duration::zero(), chrono::Duration::minutes(60), false, ); - let delay = next_refresh_delay(&outcome).unwrap(); + let delay = next_refresh_delay(Some(&outcome)).unwrap(); // Expiry minus the 10-minute refresh margin: ~50 minutes out. assert!(delay > Duration::from_mins(49), "{delay:?}"); assert!(delay <= Duration::from_mins(50), "{delay:?}"); @@ -783,35 +755,37 @@ mod tests { #[test] fn next_refresh_delay_floors_when_the_margin_is_already_open() { - let outcome = minted_outcome( - RemoteCredentialAction::Unchanged, + let outcome = minted_token( 1, chrono::Duration::minutes(55), chrono::Duration::minutes(5), true, ); - assert_eq!(next_refresh_delay(&outcome), Some(REFRESH_RESCHEDULE_FLOOR)); + assert_eq!( + next_refresh_delay(Some(&outcome)), + Some(REFRESH_RESCHEDULE_FLOOR) + ); } #[test] fn next_refresh_delay_disables_the_loop_for_static_credentials() { - assert_eq!(next_refresh_delay(&static_outcome()), None); + assert_eq!(next_refresh_delay(Some(&static_token())), None); } #[test] fn next_refresh_delay_disables_the_loop_without_managed_credentials() { - assert_eq!(next_refresh_delay(&RefreshOutcome::none()), None); + assert_eq!(next_refresh_delay(None), None); } /// Scripted refresh outcomes, recording when each refresh tick lands on /// the (paused) tokio clock. struct ScriptedRefresh { - script: Mutex>, + script: Mutex>, ticks: Mutex>, } impl ScriptedRefresh { - fn new(script: Vec) -> Arc { + fn new(script: Vec) -> Arc { Arc::new(Self { script: Mutex::new(script.into()), ticks: Mutex::new(Vec::new()), @@ -825,19 +799,21 @@ mod tests { /// The refresh the loop calls: answers the next scripted outcome. fn refresher( self: &Arc, - ) -> impl Fn() -> std::future::Ready> + Send { + ) -> impl Fn() -> std::future::Ready>> + Send + { let this = Arc::clone(self); move || { this.ticks .lock() .expect("ticks lock") .push(tokio::time::Instant::now()); - std::future::ready(Ok(this - .script - .lock() - .expect("script lock") - .pop_front() - .expect("refresh script exhausted"))) + std::future::ready(Ok(Some( + this.script + .lock() + .expect("script lock") + .pop_front() + .expect("refresh script exhausted"), + ))) } } } @@ -854,24 +830,21 @@ mod tests { let sandbox = ScriptedRefresh::new(vec![ // Minute 45: cache still fresh (expires minute 60, margin opens // minute 50). - minted_outcome( - RemoteCredentialAction::Unchanged, + minted_token( 1, chrono::Duration::minutes(45), chrono::Duration::minutes(15), true, ), // Minute ~50: margin open → the source minted generation 2. - minted_outcome( - RemoteCredentialAction::Embedded, + minted_token( 2, chrono::Duration::zero(), chrono::Duration::minutes(60), false, ), // Minute ~100: generation 2 still fresh. - minted_outcome( - RemoteCredentialAction::Unchanged, + minted_token( 2, chrono::Duration::minutes(50), chrono::Duration::minutes(10), @@ -909,16 +882,14 @@ mod tests { #[tokio::test(start_paused = true)] async fn refresh_ahead_honors_the_expiry_based_initial_delay() { let interval = Duration::from_mins(45); - let entry_outcome = minted_outcome( - RemoteCredentialAction::Unchanged, + let entry_outcome = minted_token( 1, chrono::Duration::minutes(45), chrono::Duration::minutes(15), true, ); - let initial_delay = next_refresh_delay(&entry_outcome).unwrap(); - let sandbox = ScriptedRefresh::new(vec![minted_outcome( - RemoteCredentialAction::Embedded, + let initial_delay = next_refresh_delay(Some(&entry_outcome)).unwrap(); + let sandbox = ScriptedRefresh::new(vec![minted_token( 2, chrono::Duration::zero(), chrono::Duration::minutes(60), diff --git a/lib/components/fabro-workflow/src/pipeline/publish.rs b/lib/components/fabro-workflow/src/pipeline/publish.rs index 6455ec3f1..b9f896328 100644 --- a/lib/components/fabro-workflow/src/pipeline/publish.rs +++ b/lib/components/fabro-workflow/src/pipeline/publish.rs @@ -100,9 +100,6 @@ fn push_attempt_cause(attempt: &fabro_sandbox::PushAttempt) -> String { { let _ = write!(line, " (token age {age_ms}ms)"); } - if let Some(refresh_error) = attempt.refresh_error { - let _ = write!(line, ", refresh error: {refresh_error}"); - } line } @@ -285,7 +282,6 @@ mod tests { attempt: u32, retry_reason: Option, token_age_ms: Option, - refresh_error: Option, ) -> fabro_sandbox::PushAttempt { let started_at = Utc::now(); fabro_sandbox::PushAttempt { @@ -302,8 +298,6 @@ mod tests { expires_at: started_at + chrono::Duration::hours(1), }, }), - credential_action: Some(fabro_sandbox::RemoteCredentialAction::Unchanged), - refresh_error, } } @@ -314,14 +308,12 @@ mod tests { .iter() .enumerate() .map(|(index, reason)| fabro_sandbox::PushAttempt { - attempt: u32::try_from(index).unwrap() + 1, - started_at: Utc::now(), - success: false, - retry_reason: *reason, - exec_output_tail: None, - token: None, - credential_action: None, - refresh_error: None, + attempt: u32::try_from(index).unwrap() + 1, + started_at: Utc::now(), + success: false, + retry_reason: *reason, + exec_output_tail: None, + token: None, }) .collect() } @@ -362,13 +354,11 @@ mod tests { 1, Some(fabro_sandbox::GitRetryReason::TokenReplication), Some(180), - None, ), push_attempt( 2, Some(fabro_sandbox::GitRetryReason::TokenReplication), Some(3320), - Some(fabro_sandbox::RefreshErrorKind::SetUrl), ), ]; let last_push = Utc::now() - chrono::Duration::seconds(67); @@ -401,7 +391,7 @@ mod tests { "{attempt_lines:?}" ); assert!( - attempt_lines[1].contains("refresh error: set_url"), + attempt_lines[1].contains("(token age 3320ms)"), "{attempt_lines:?}" ); assert_eq!( diff --git a/lib/foundation/fabro-redact/src/safe_url.rs b/lib/foundation/fabro-redact/src/safe_url.rs index 0661f0667..635e95927 100644 --- a/lib/foundation/fabro-redact/src/safe_url.rs +++ b/lib/foundation/fabro-redact/src/safe_url.rs @@ -89,6 +89,12 @@ impl DisplaySafeUrl { self.0.to_string() } + /// Replace every occurrence of this URL's raw form in `text` with its + /// redacted display form, for output that may echo a credentialed URL. + pub fn redact_in(&self, text: &str) -> String { + text.replace(&self.raw_string(), &self.redacted_string()) + } + /// Remove credentials from this URL, preserving the SSH `git` username. #[inline] pub fn remove_credentials(&mut self) { @@ -511,4 +517,15 @@ mod tests { formatter.debug_struct("CapturedTraceWriter").finish() } } + + #[test] + fn redact_in_replaces_the_raw_url_with_its_display_form() { + let url = DisplaySafeUrl::parse("https://x-access-token:ghs_secret@github.com/o/r.git") + .expect("valid url"); + let text = format!("fatal: unable to access '{}': 403", url.raw_string()); + let redacted = url.redact_in(&text); + assert!(!redacted.contains("ghs_secret"), "{redacted}"); + assert!(redacted.contains("github.com/o/r.git"), "{redacted}"); + assert_eq!(url.redact_in("nothing to see"), "nothing to see"); + } } diff --git a/lib/foundation/fabro-types/src/run_event/misc.rs b/lib/foundation/fabro-types/src/run_event/misc.rs index bb3116992..19406b57b 100644 --- a/lib/foundation/fabro-types/src/run_event/misc.rs +++ b/lib/foundation/fabro-types/src/run_event/misc.rs @@ -136,30 +136,6 @@ pub enum GitTokenProvenance { Static, } -/// What credential preparation changed before a git push attempt. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display)] -#[serde(rename_all = "snake_case")] -#[strum(serialize_all = "snake_case")] -pub enum GitCredentialAction { - /// Fabro wrote a token generation into the remote URL. - Embedded, - /// The remote already tracked the selected token generation. - Unchanged, - /// No managed credential was available. - None, -} - -/// Which credential preparation step failed before a git push attempt. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display)] -#[serde(rename_all = "snake_case")] -#[strum(serialize_all = "snake_case")] -pub enum GitCredentialRefreshError { - /// Token resolution or minting failed. - Mint, - /// Rewriting the remote URL failed. - SetUrl, -} - /// One attempt of a retried git push, nested inside [`GitPushProps`]. /// /// The durable projection of the sandbox layer's runtime attempt record. @@ -188,13 +164,6 @@ pub struct GitPushAttemptProps { /// Token age at the attempt; absent for static credentials. #[serde(default, skip_serializing_if = "Option::is_none")] pub token_age_ms: Option, - /// What the credential refresh did to the remote this attempt: - /// `embedded`, `unchanged`, or `none`. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub credential_action: Option, - /// A credential `mint` or `set_url` failure this attempt pushed through. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub refresh_error: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]