Move the Read tool's line numbering and the shell quoting wrapper out of fabro-sandbox

format_lines_numbered renders a file the way the agent's Read tool shows
it: every line prefixed with its number, from an offset for a limit. That
is the agent's presentation, not a sandbox concern, and it only lived in
fabro-sandbox so RunSandbox::read_file(path, offset, limit) could call
it. The function now lives in fabro-agent next to the Read tool, with its
tests, and the Read, ReadManyFiles, and Kimi ReadFile tools number the
text they get from read_file_text themselves. RunSandbox::read_file goes
away; the sandbox returns bytes or text and nothing else.

fabro-sandbox also re-exported shell_quote through a one-line wrapper so
callers could reach it from the sandbox crate or from fabro-agent. The
audited implementation is fabro_util:🐚:shell_quote; the six
importers now use it directly and the wrapper and both re-exports are
gone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-09-11 15:00:17 -06:00
parent f69a6779c8
commit 8771ef6d8e
No known key found for this signature in database
15 changed files with 109 additions and 111 deletions

View file

@ -35,9 +35,10 @@ use fabro_api::types::{
RunFilesMeta, RunFilesMetaDegradedReason, RunFilesMetaScope, RunFilesMetaSource,
RunFilesMetaToSha,
};
use fabro_sandbox::Termination;
use fabro_sandbox::reconnect::reconnect_for_run;
use fabro_sandbox::{Termination, shell_quote};
use fabro_types::RunId;
use fabro_util::shell;
use fabro_workflow::sandbox_git::{
DiffError, DiffNumstat, RawDiffEntry, SubmoduleChange, SymlinkChange, list_changed_files_raw,
list_diff_numstat, stream_blob_metadata, stream_blobs,
@ -1204,7 +1205,7 @@ async fn resolve_ref_sha_and_time(
sandbox: &RunSandbox,
git_ref: &str,
) -> std::result::Result<(String, Option<chrono::DateTime<chrono::Utc>>), ApiError> {
let ref_q = shell_quote(git_ref);
let ref_q = shell::shell_quote(git_ref);
let res = sandbox
.exec_command(
&format!("git -c core.hooksPath=/dev/null show -s --format=%H\\ %cI {ref_q}"),

View file

@ -10,9 +10,10 @@ use fabro_acp::{
run_acp_turn,
};
use fabro_sandbox::test_support::{MockSandbox, MockStdioProcess};
use fabro_sandbox::{RunSandbox, local_sandbox, shell_quote};
use fabro_sandbox::{RunSandbox, local_sandbox};
use fabro_types::SteeringMessage;
use fabro_util::error::collect_chain;
use fabro_util::shell;
use tokio::fs::{read_to_string, write};
use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader, DuplexStream};
use tokio::process::Command;
@ -104,7 +105,10 @@ async fn session_lifecycle_initializes_sends_prompt_and_aggregates_text() {
.await
.expect("write fake ACP agent");
let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy()));
let raw_command = format!(
"python3 {}",
shell::shell_quote(&script_path.to_string_lossy())
);
let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command");
let sandbox: Arc<RunSandbox> = Arc::new(
local_sandbox(tempdir.path().to_path_buf())
@ -149,7 +153,10 @@ async fn steering_sends_followup_session_prompt_over_acp() {
.await
.expect("write fake ACP agent");
let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy()));
let raw_command = format!(
"python3 {}",
shell::shell_quote(&script_path.to_string_lossy())
);
let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command");
let sandbox: Arc<RunSandbox> = Arc::new(
local_sandbox(tempdir.path().to_path_buf())
@ -216,7 +223,10 @@ async fn interrupt_then_steer_sends_cancel_then_followup_session_prompt_over_acp
.await
.expect("write fake ACP agent");
let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy()));
let raw_command = format!(
"python3 {}",
shell::shell_quote(&script_path.to_string_lossy())
);
let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command");
let sandbox: Arc<RunSandbox> = Arc::new(
local_sandbox(tempdir.path().to_path_buf())
@ -295,7 +305,10 @@ async fn inline_interrupt_terminates_agent_that_ignores_cancel() {
.await
.expect("write fake ACP agent");
let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy()));
let raw_command = format!(
"python3 {}",
shell::shell_quote(&script_path.to_string_lossy())
);
let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command");
let sandbox: Arc<RunSandbox> = Arc::new(
local_sandbox(tempdir.path().to_path_buf())
@ -686,7 +699,10 @@ async fn run_fake_agent_with_activity(
write(&script_path, fake_acp_agent_script())
.await
.expect("write fake ACP agent");
let raw_command = format!("python3 {}", shell_quote(&script_path.to_string_lossy()));
let raw_command = format!(
"python3 {}",
shell::shell_quote(&script_path.to_string_lossy())
);
let command = AcpProcessSpec::from_command_attr(&raw_command).expect("parse ACP command");
let sandbox: Arc<RunSandbox> = Arc::new(
local_sandbox(tempdir.to_path_buf())

View file

@ -58,8 +58,7 @@ pub use sandbox::{
CaptureStats, DirEntry, DriverSpec, ExecControls, ExecResult, ExecResultExt, ExecSpec,
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,
TokenProvenance, TokenSnapshot, WalkOptions, command_termination, program_exit_code,
};
pub use session::{
CompletionCoordinator, Session, SessionControlHandle, SessionInputTiming,

View file

@ -27,11 +27,12 @@ use serde_json::Value;
use strum::EnumString;
use crate::native_tool::NativeTool;
use crate::sandbox::{ExecResultExt, GrepOptions, Termination, format_lines_numbered};
use crate::sandbox::{ExecResultExt, GrepOptions, Termination};
use crate::tool_registry::{RegisteredTool, ToolSource};
use crate::tools::{
DEFAULT_READ_LINES, emit_shell_process_completed, execute_grep, execute_shell_command,
grep_result_path, make_edit_file_tool, optional_usize_arg, required_str, retain_shell_output,
format_lines_numbered, grep_result_path, make_edit_file_tool, optional_usize_arg, required_str,
retain_shell_output,
};
const DEFAULT_GREP_RESULTS: usize = 250;
@ -230,9 +231,16 @@ depends on an exact file, API, or output shape, inspect the final result before
Some(offset) => {
let start = usize::try_from(offset)
.map_err(|_| "line_offset must fit in usize".to_string())?;
ctx.env.read_file(path, Some(start), Some(n_lines)).await
ctx.env
.read_file_text(path)
.await
.map(|text| format_lines_numbered(&text, Some(start), Some(n_lines)))
}
None => ctx.env.read_file(path, None, Some(n_lines)).await,
None => ctx
.env
.read_file_text(path)
.await
.map(|text| format_lines_numbered(&text, None, Some(n_lines))),
}
.map_err(|e| e.display_with_causes())?;

View file

@ -3,6 +3,5 @@ pub use fabro_sandbox::{
CaptureStats, DirEntry, DriverSpec, ExecControls, ExecResult, ExecResultExt, ExecSpec,
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,
TokenProvenance, TokenSnapshot, WalkOptions, command_termination, program_exit_code,
};

View file

@ -24,6 +24,27 @@ const MAX_WEB_FETCH_BYTES: usize = 100 * 1024;
const MAX_READ_MANY_FILES_CONCURRENCY: usize = 8;
pub(crate) const DEFAULT_READ_LINES: usize = 2000;
/// The Read tool's rendering of a file: each line prefixed with its 1-based
/// number, right-aligned, from `offset` (1-based) for `limit` lines.
#[must_use]
pub(crate) fn format_lines_numbered(
content: &str,
offset: Option<usize>,
limit: Option<usize>,
) -> String {
let all_lines: Vec<&str> = content.lines().collect();
let skip = offset.unwrap_or(1).saturating_sub(1);
let take = limit.unwrap_or(all_lines.len());
let selected: Vec<&str> = all_lines.into_iter().skip(skip).take(take).collect();
let width = (skip + selected.len()).to_string().len().max(1);
let mut result = String::new();
for (i, line) in selected.iter().enumerate() {
let line_num = skip + i + 1;
let _ = writeln!(result, "{line_num:>width$} | {line}");
}
result
}
/// Configuration for the optional LLM-based summarizer used by `web_fetch`.
#[derive(Clone)]
pub struct WebFetchSummarizer {
@ -137,12 +158,12 @@ pub fn make_read_file_tool() -> RegisteredTool {
let offset_usize = optional_usize_arg(&args, "offset")?;
let limit_usize = optional_usize_arg(&args, "limit")?.or(Some(DEFAULT_READ_LINES));
let content = ctx
let text = ctx
.env
.read_file(file_path, offset_usize, limit_usize)
.read_file_text(file_path)
.await
.map_err(|e| e.display_with_causes())?;
Ok(content)
Ok(format_lines_numbered(&text, offset_usize, limit_usize))
})
}),
source: ToolSource::Native,
@ -578,7 +599,10 @@ pub(crate) fn make_read_many_files_tool() -> RegisteredTool {
.map(|path| {
let env = Arc::clone(&ctx.env);
async move {
let result = env.read_file(&path, None, None).await;
let result = env
.read_file_text(&path)
.await
.map(|text| format_lines_numbered(&text, None, None));
(path, result)
}
})
@ -758,6 +782,18 @@ mod tests {
use crate::config::{NativeToolOptions, SessionOptions, ToolSecrets};
use crate::event::{Emitter, SessionBoundEmitter};
use crate::sandbox::*;
#[test]
fn format_lines_numbered_numbers_every_line() {
let result = format_lines_numbered("hello\nworld\nfoo", None, None);
assert_eq!(result, "1 | hello\n2 | world\n3 | foo\n");
}
#[test]
fn format_lines_numbered_honors_offset_and_limit() {
let result = format_lines_numbered("a\nb\nc\nd\ne", Some(2), Some(2));
assert_eq!(result, "2 | b\n3 | c\n");
}
use crate::test_support::MockSandbox;
use crate::tool_registry::{ToolContext, ToolDefinitionExt};
use crate::types::SessionEvent;

View file

@ -1,3 +1,5 @@
use fabro_util::shell;
use crate::sandbox;
#[derive(Clone, Debug, PartialEq, Eq)]
@ -68,8 +70,8 @@ fn validate_path_component(label: &str, component: &str) -> crate::Result<()> {
pub(crate) fn repo_symlink_command(layout: &GitHubRepoLayout) -> String {
format!(
"ln -s {} {}",
sandbox::shell_quote(&layout.primary_repo_path),
sandbox::shell_quote(&layout.primary_repo_link),
shell::shell_quote(&layout.primary_repo_path),
shell::shell_quote(&layout.primary_repo_link),
)
}

View file

@ -19,6 +19,7 @@ use std::time::{Duration, Instant};
use fabro_github::GitHubCredentials;
use fabro_github::token_source::{InstallationTokenSource, TokenSnapshot};
use fabro_types::SandboxProviderKind;
use fabro_util::shell;
use fabro_util::workspace_glob::WorkspaceGlob;
use sandbox_driver::{
DirEntry, EventContext, ExecControls, ExecResult, ExecSpec, ExecStreamingResult, FileKind,
@ -659,20 +660,6 @@ impl RunSandbox {
.map_err(|err| crate::Error::context("File is not valid UTF-8", err))
}
/// A file's text with line numbers, from `offset` for `limit` lines.
pub async fn read_file(
&self,
path: &str,
offset: Option<usize>,
limit: Option<usize>,
) -> crate::Result<String> {
Ok(sandbox::format_lines_numbered(
&self.read_file_text(path).await?,
offset,
limit,
))
}
pub async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> {
self.handle()?
.fs()
@ -1006,8 +993,8 @@ impl RunSandbox {
}
vec![format!(
"git fetch origin {} && git checkout {}",
sandbox::shell_quote(run_branch),
sandbox::shell_quote(run_branch)
shell::shell_quote(run_branch),
shell::shell_quote(run_branch)
)]
}
@ -1186,7 +1173,7 @@ mod tests {
assert!(missing.to_string().contains("does not exist"), "{missing}");
let read = f
.sandbox
.read_file("nonexistent.txt", None, None)
.read_file_text("nonexistent.txt")
.await
.unwrap_err();
assert!(

View file

@ -56,8 +56,7 @@ pub use reconnect::{
};
pub use sandbox::{
DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport,
SandboxFile, SandboxWorkspaceLayout, format_lines_numbered, redacted_output_tail, setup_git,
shell_quote,
SandboxFile, SandboxWorkspaceLayout, redacted_output_tail, setup_git,
};
/// 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

View file

@ -1,9 +1,7 @@
use std::fmt::Write;
use std::time::Duration;
use chrono::{DateTime, Utc};
use fabro_github::token_source::TokenSnapshot;
use fabro_util::shell;
use sandbox_driver::{
Git as _, GitAttempt, GitCheckoutOptions, GitFetchOptions, GitPushOptions, GitRetryError,
GitRetryPolicy, retry_git,
@ -50,25 +48,6 @@ pub enum GitSetupIntent {
},
}
/// Formats file content with line numbers for display.
///
/// Applies optional offset (1-based starting line number) and limit (max lines
/// to return). Line numbers are 1-based and right-aligned.
#[must_use]
pub fn format_lines_numbered(content: &str, offset: Option<usize>, limit: Option<usize>) -> String {
let all_lines: Vec<&str> = content.lines().collect();
let skip = offset.unwrap_or(1).saturating_sub(1);
let take = limit.unwrap_or(all_lines.len());
let selected: Vec<&str> = all_lines.into_iter().skip(skip).take(take).collect();
let width = (skip + selected.len()).to_string().len().max(1);
let mut result = String::new();
for (i, line) in selected.iter().enumerate() {
let line_num = skip + i + 1;
let _ = writeln!(result, "{line_num:>width$} | {line}");
}
result
}
/// Build a redacted `ExecOutputTail` from stdout/stderr text without
/// fabricating a synthetic `ExecResult`. Each stream is redacted, then
/// capped to its newest `max_bytes_per_stream`. Terminal control sequences
@ -140,13 +119,6 @@ pub(crate) fn join_sandbox_path(base: &str, relative_path: &str) -> String {
format!("{}/{relative_path}", base.trim_end_matches('/'))
}
/// Shell-quote a string using `shlex::try_quote`, with a fallback for edge
/// cases. Re-exported from [`fabro_util::shell::shell_quote`] so sandbox code
/// and the config resolve layer share one audited implementation.
pub fn shell_quote(s: &str) -> String {
shell::shell_quote(s)
}
/// Creates the run branch in the sandbox's checkout through the driver's
/// git facet: a new run branches from `HEAD`, a fork from the source run's
/// checkpoint. The branch is created at that base, or moved to it when an
@ -896,8 +868,6 @@ mod push_tests {
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn sandbox_tracing_events_do_not_log_raw_command_or_stdin_fields() {
let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src");
@ -910,27 +880,6 @@ mod tests {
);
}
#[test]
fn format_lines_numbered_basic() {
let result = format_lines_numbered("hello\nworld\nfoo", None, None);
assert_eq!(result, "1 | hello\n2 | world\n3 | foo\n");
}
#[test]
fn format_lines_numbered_with_offset_limit() {
let result = format_lines_numbered("a\nb\nc\nd\ne", Some(2), Some(2));
assert!(result.contains("2 | b"));
assert!(result.contains("3 | c"));
assert!(!result.contains("1 | a"));
assert!(!result.contains("4 | d"));
}
#[test]
fn shell_quote_basic() {
assert_eq!(shell_quote("hello"), "hello");
assert_eq!(shell_quote("hello world"), "'hello world'");
}
#[expect(
clippy::disallowed_methods,
reason = "unit test performs a small synchronous source scan of local Rust files"

View file

@ -636,10 +636,11 @@ mod tests {
use fabro_acp::test_support::fake_acp_agent_script;
use fabro_acp::{AcpError, AcpProcessExit};
use fabro_agent::{RunSandbox, TokenProvenance, TokenSnapshot, local_sandbox, shell_quote};
use fabro_agent::{RunSandbox, TokenProvenance, TokenSnapshot, local_sandbox};
use fabro_graphviz::graph::{AttrValue, Node};
use fabro_sandbox::test_support::MockSandbox;
use fabro_types::{CommandTermination, EventBody, ExecOutputTail};
use fabro_util::shell;
use tokio_util::sync::CancellationToken;
use super::{
@ -931,7 +932,7 @@ mod tests {
"acp.command".to_string(),
AttrValue::String(format!(
"python3 {}",
shell_quote(&script_path.to_string_lossy())
shell::shell_quote(&script_path.to_string_lossy())
)),
);
@ -1037,7 +1038,7 @@ mod tests {
"acp.command".to_string(),
AttrValue::String(format!(
"python3 {}",
shell_quote(&script_path.to_string_lossy())
shell::shell_quote(&script_path.to_string_lossy())
)),
);
@ -1100,7 +1101,7 @@ mod tests {
"acp.command".to_string(),
AttrValue::String(format!(
"python3 {}",
shell_quote(&script_path.to_string_lossy())
shell::shell_quote(&script_path.to_string_lossy())
)),
);
@ -1184,7 +1185,7 @@ mod tests {
"acp.command".to_string(),
AttrValue::String(format!(
"python3 {}",
shell_quote(&script_path.to_string_lossy())
shell::shell_quote(&script_path.to_string_lossy())
)),
);

View file

@ -1,7 +1,8 @@
use std::collections::HashSet;
use std::sync::Arc;
use fabro_agent::{RunSandbox, shell_quote};
use fabro_agent::RunSandbox;
use fabro_util::shell;
use sandbox_driver::{Git as _, GitDiffOptions, GitRevisionRange};
/// The paths the working tree changed against `HEAD`, plus the untracked
@ -43,8 +44,10 @@ pub async fn files_touched_since(
let last_file_touched = if files_touched.is_empty() {
None
} else {
let quoted_files: Vec<String> =
files_touched.iter().map(|file| shell_quote(file)).collect();
let quoted_files: Vec<String> = files_touched
.iter()
.map(|file| shell::shell_quote(file))
.collect();
let cmd = format!("ls -t {} | head -1", quoted_files.join(" "));
sandbox
.exec_command(&cmd, 5_000, None, None, None)

View file

@ -1357,7 +1357,7 @@ mod tests {
"acp.command".to_string(),
AttrValue::String(format!(
"python3 {}",
fabro_sandbox::shell_quote(&script_path.to_string_lossy())
fabro_util::shell::shell_quote(&script_path.to_string_lossy())
)),
);
let mut exit = Node::new("exit");

View file

@ -1,6 +1,7 @@
use fabro_agent::RunSandbox;
use fabro_sandbox::{ExecResult, ExecResultExt, Termination, shell_quote};
use fabro_sandbox::{ExecResult, ExecResultExt, Termination};
use fabro_util::error::SharedError;
use fabro_util::shell;
use tokio::sync::OnceCell;
use crate::sandbox_git::GitCommandError;
@ -66,9 +67,9 @@ async fn probe_sandbox_git(sandbox: &RunSandbox) -> Result<(), SharedError> {
GIT_INDEX_FILE={index_q} {git} update-index --add --cacheinfo 100644,$blob,probe.txt\n\
GIT_INDEX_FILE={index_q} {git} write-tree >/dev/null\n\
rm -rf {temp_q}",
temp_q = shell_quote(&temp),
probe_file_q = shell_quote(&probe_file),
index_q = shell_quote(&index),
temp_q = shell::shell_quote(&temp),
probe_file_q = shell::shell_quote(&probe_file),
index_q = shell::shell_quote(&index),
git = "git -c maintenance.auto=0 -c gc.auto=0",
);
exec_ok(sandbox, &command).await

View file

@ -376,7 +376,7 @@ async fn daytona_file_round_trip() {
assert!(env.file_exists(test_path).await.unwrap());
// Read
let read_back = env.read_file(test_path, None, None).await.unwrap();
let read_back = env.read_file_text(test_path).await.unwrap();
assert!(read_back.contains(content));
// Delete
@ -491,7 +491,7 @@ async fn daytona_artifact_sync_uploads_and_rewrites_pointer() {
"artifact file should exist in Daytona sandbox at {remote_path}"
);
let remote_content = env.read_file(remote_path, None, None).await.unwrap();
let remote_content = env.read_file_text(remote_path).await.unwrap();
assert!(
remote_content.len() > 100 * 1024,
"remote artifact should be >100KB, got {} bytes",
@ -1593,10 +1593,7 @@ async fn daytona_cp_upload_download_round_trip() {
env.file_exists("cp_test_upload.txt").await.unwrap(),
"uploaded file should exist in the sandbox"
);
let remote_content = env
.read_file("cp_test_upload.txt", None, None)
.await
.unwrap();
let remote_content = env.read_file_text("cp_test_upload.txt").await.unwrap();
assert!(
remote_content.contains("hello from fabro cp e2e test"),
"expected uploaded content in sandbox, got: {remote_content}"