codewhale-hq/Codewhale
1
0 Fork 0
mirror of https://github.com/codewhale-hq/Codewhale.git synced 2026-11-09 09:59:45 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix: harden project config, repository reads, fetch refspecs and downloads

Browse Source 
Cause and fix, per area:

- Project config: a repository's .codewhale/config.toml could set
  notes_path, the append target of the auto-approved note tool.
  notes_path joins DENY_AT_PROJECT_SCOPE. The note tool and /note also
  refuse a symlinked notes file or a notes directory that resolves outside
  the workspace, and the note tool flushes its write before reporting
  success (a tokio file finishes writes in the background).
- Repository reads: diff/show collections passed --no-ext-diff and
  --no-textconv, but a submodule dirty check or diff.submodule=diff spawns
  a child git inside the submodule that inherits neither flag, so the
  submodule's clean filter and external diff driver ran. Git::
  REVIEW_DIFF_ARGS (--no-ext-diff --no-textconv --submodule=short
  --ignore-submodules=dirty) is now the one flag set used by codewhale
  review, review_pr, the git_diff/git_show tools, commit planning, verify,
  delivery, classifier-approved git reads and the runtime API diff routes
  (which previously lacked --no-textconv). git_diff, git_show and commit
  planning now run through Git::review_command.
- git_fetch: a src:dst refspec could write refs/heads/* or refs/tags/*.
  The destination must now be under refs/remotes/.
- Windows URL opening used `cmd /C start`, which parses & | ^ % in the
  URL. It now uses rundll32 url.dll,FileProtocolHandler.
- Skills install/sync buffered the whole body before checking the size
  cap and the registry index had no cap. utils::read_response_body_capped
  (declared-length check plus a streaming cap) now backs skills downloads,
  the registry index (4 MiB) and the MCP Streamable HTTP reader.
- Runtime thread/turn/item ids used 8 hex chars (32 bits) in flat
  per-kind directories where saves replace by id. runtime_record_id uses
  the full UUID.

Tests (codewhale-tui --lib, CARGO_BUILD_JOBS=4):
- new project_overlay_ignores_notes_path, diff_and_show_reads_run_no_
  repository_configured_commands, download_with_cap_stops_reading_at_the_cap,
  runtime_record_ids_carry_a_full_uuid and the refspec cases in
  git_fetch_rejects_malformed_refspecs each fail with its fix reverted.
- project_overlay 22 passed; 0 failed. tools::git:: 16/0.
  tools::git_history:: 16/0. tools::git_tool:: 5/0. skills::install:: 21/0.
  tools::shell:: 155/0 (1 ignored). memory::note:: 15/0.
  mcp::tests::streamable_http 3/0. tools::review 65/0. tools::verify:: 16/0.
  subagent::delivery 19/0. review_pr 18/0. runtime_api:: 334/0.
  runtime_threads:: 238/1 in a parallel run; the failure,
  approval_wait_heartbeat_is_never_sequenced_after_the_decision, passed
  3/3 run alone.
- cargo clippy -p codewhale-tui --lib --tests: only too_many_arguments,
  which CI allows.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
CodeWhale BotandClaude Opus 5.5 committed 2026-09-27 00:34:29 -07:00
1 parent 94130d91ae
commit 6fae32453f
21 files changed
+688 -167
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

CHANGELOG.md
+22
View File
@@ -160,6 +160,28 @@ quieter, and Fleet runs can be checked before they spend anything.
resend without the image and a message saying so
([#6396](https://github.com/Hmbown/Codewhale/issues/6396)).
### Security
- A project's `.codewhale/config.toml` can no longer set `notes_path`; it is
ignored with the other user-only keys. The `note` tool and `/note` refuse a
notes file that is a symlink or whose directory resolves outside the
workspace, and a note is flushed to disk before the tool reports success.
- Diffs and shows that read repository content (`codewhale review`, the
`git_diff`/`git_show` tools, verification, delivery and the runtime API diff
routes) share one flag set: no external diff, no textconv, and submodules
compared by commit only, so a submodule's configured filters and diff
drivers never run. `git_diff`, `git_show` and commit planning also read
through the hardened review command.
- `git_fetch` accepts a `src:dst` refspec only when the destination is under
`refs/remotes/`; local branches and tags are never written.
- Opening a URL on Windows goes through the URL protocol handler instead of
`cmd /C start`, so characters in the URL are not interpreted by the shell.
- Skill downloads and the skills registry index are read through a streaming
size cap shared with the MCP HTTP transport, instead of buffering the whole
body before checking its size.
- Durable runtime thread, turn and item ids carry a full UUID instead of 32
random bits, so two records can no longer collide and overwrite each other.
### Removed
- Flags, settings and tool parameters that did nothing are gone
crates/tui/CHANGELOG.md
+22
View File
@@ -160,6 +160,28 @@ quieter, and Fleet runs can be checked before they spend anything.
resend without the image and a message saying so
([#6396](https://github.com/Hmbown/Codewhale/issues/6396)).
### Security
- A project's `.codewhale/config.toml` can no longer set `notes_path`; it is
ignored with the other user-only keys. The `note` tool and `/note` refuse a
notes file that is a symlink or whose directory resolves outside the
workspace, and a note is flushed to disk before the tool reports success.
- Diffs and shows that read repository content (`codewhale review`, the
`git_diff`/`git_show` tools, verification, delivery and the runtime API diff
routes) share one flag set: no external diff, no textconv, and submodules
compared by commit only, so a submodule's configured filters and diff
drivers never run. `git_diff`, `git_show` and commit planning also read
through the hardened review command.
- `git_fetch` accepts a `src:dst` refspec only when the destination is under
`refs/remotes/`; local branches and tags are never written.
- Opening a URL on Windows goes through the URL protocol handler instead of
`cmd /C start`, so characters in the URL are not interpreted by the shell.
- Skill downloads and the skills registry index are read through a streaming
size cap shared with the MCP HTTP transport, instead of buffering the whole
body before checking its size.
- Durable runtime thread, turn and item ids carry a full UUID instead of 32
random bits, so two records can no longer collide and overwrite each other.
### Removed
- Flags, settings and tool parameters that did nothing are gone
crates/tui/src/commands/groups/memory/note.rs
+61 -1
View File
@@ -26,8 +26,14 @@ fn note(workspace: &Path, content: Option<&str>) -> CommandResult {
let notes_path = notes_path(workspace);
let (command, rest) = split_command(input);
let command = command.to_ascii_lowercase();
if !matches!(command.as_str(), "path" | "help")
&& let Err(error) = ensure_notes_target_in_workspace(&notes_path, workspace)
{
return CommandResult::error(error);
}
match command.to_ascii_lowercase().as_str() {
match command.as_str() {
"add" => append_note_command(&notes_path, rest),
"list" => list_notes_command(&notes_path),
"show" => show_note_command(&notes_path, rest),
@@ -51,6 +57,39 @@ fn notes_path(workspace: &Path) -> PathBuf {
workspace.join(".deepseek").join("notes.md")
}
/// The notes file lives in the workspace, which may be a cloned repository:
/// a committed `notes.md -> ~/.zshrc` or a symlinked `.codewhale/` must not
/// let `/note clear` empty (or `/note list` print) a file outside it. Same
/// rule as the `note` tool.
fn ensure_notes_target_in_workspace(notes_path: &Path, workspace: &Path) -> Result<(), String> {
if let Ok(meta) = fs::symlink_metadata(notes_path)
&& (meta.file_type().is_symlink() || !meta.is_file())
{
return Err(format!(
"Refusing to use {}: the notes path is a symlink or not a regular file.",
notes_path.display()
));
}
let (Some(parent), Ok(root)) = (notes_path.parent(), fs::canonicalize(workspace)) else {
return Ok(());
};
// Only existing ancestors can redirect; the rest is created as real
// directories.
let mut existing = parent.to_path_buf();
while !existing.exists() {
if !existing.pop() {
return Ok(());
}
}
match fs::canonicalize(&existing) {
Ok(resolved) if !resolved.starts_with(&root) => Err(format!(
"Refusing to use {}: its directory resolves outside the workspace.",
notes_path.display()
)),
_ => Ok(()),
}
}
fn split_command(input: &str) -> (&str, Option<&str>) {
match input.find(char::is_whitespace) {
Some(index) => (&input[..index], Some(input[index..].trim())),
@@ -449,6 +488,27 @@ mod tests {
assert!(!listed.contains("Second note"));
}
#[cfg(unix)]
#[test]
fn note_commands_refuse_notes_that_leave_the_workspace() {
let tmpdir = TempDir::new().unwrap();
let outside = TempDir::new().unwrap();
let rc = outside.path().join("zshrc");
std::fs::write(&rc, "export PATH=keep\n").unwrap();
std::fs::create_dir_all(tmpdir.path().join(".deepseek")).unwrap();
std::os::unix::fs::symlink(&rc, notes_path(&tmpdir)).unwrap();
for command in ["clear", "remove 1", "edit 1 x", "list", "hello"] {
let result = note(tmpdir.path(), Some(command));
assert!(result.is_error, "/note {command} must be refused");
}
assert_eq!(std::fs::read_to_string(&rc).unwrap(), "export PATH=keep\n");
let linked = TempDir::new().unwrap();
std::os::unix::fs::symlink(outside.path(), linked.path().join(".deepseek")).unwrap();
assert!(note(linked.path(), Some("clear")).is_error);
assert!(!outside.path().join("notes.md").exists());
}
#[test]
fn test_note_clear_empties_file() {
let tmpdir = TempDir::new().unwrap();
crates/tui/src/dependencies.rs
+13 -1
View File
@@ -539,8 +539,20 @@ pub(crate) fn apply_git_noninteractive_env(cmd: &mut Command) {
}
impl Git {
/// Flags every `diff`, `show` or patch `log` that collects repository
/// content passes, so the output never runs a repository-configured
/// command. `--no-ext-diff`/`--no-textconv` skip diff drivers; a dirty
/// check or `diff.submodule=diff` spawns a child git inside each submodule
/// that inherits neither flag, so submodules compare by commit only.
pub(crate) const REVIEW_DIFF_ARGS: [&'static str; 4] = [
"--no-ext-diff",
"--no-textconv",
"--submodule=short",
"--ignore-submodules=dirty",
];
/// Construct a read-only review command with content conversion disabled.
/// Review callers also pass `--no-ext-diff` and `--no-textconv` for diffs.
/// Review callers also pass [`Self::REVIEW_DIFF_ARGS`] for diffs.
/// Configured filters otherwise execute even when those flags are present.
pub(crate) fn review_command(workspace: &Path) -> anyhow::Result<Command> {
use anyhow::{Context, bail};
crates/tui/src/lib.rs
+25 -11
View File
@@ -10030,7 +10030,8 @@ fn collect_diff(
let mut cmd = crate::dependencies::Git::review_command(workspace)?;
// Review repository content without executing its diff drivers.
cmd.current_dir(workspace)
.args(["diff", "--no-ext-diff", "--no-textconv"]);
.arg("diff")
.args(crate::dependencies::Git::REVIEW_DIFF_ARGS);
if args.staged {
cmd.arg("--cached");
}
@@ -11246,6 +11247,9 @@ fn merge_project_config_with_approval_baseline(
"mcp_config_path",
"mcp_oauth_callback_port",
"mcp_oauth_callback_url",
// The auto-approved `note` tool appends to `notes_path`, so a
// project value would be a write target the user never reviewed.
"notes_path",
];
for key in DENY_AT_PROJECT_SCOPE {
if table.contains_key(*key) {
@@ -11258,7 +11262,7 @@ fn merge_project_config_with_approval_baseline(
}
// String fields a project may legitimately override (model,
// approval/sandbox tightening, notes path, reasoning effort).
// approval/sandbox tightening, reasoning effort).
if !config.environment_model_applied
&& let Some(model) = table.get("model").and_then(toml::Value::as_str)
&& !model.is_empty()
@@ -11267,15 +11271,10 @@ fn merge_project_config_with_approval_baseline(
config.set_provider_model_override(config.api_provider(), Some(model.to_string()));
config.remembered_selection_scope = Some(false);
}
for (key, field) in [
("reasoning_effort", &mut config.reasoning_effort),
("notes_path", &mut config.notes_path),
] {
if let Some(v) = table.get(key).and_then(toml::Value::as_str)
&& !v.is_empty()
{
*field = Some(v.to_string());
}
if let Some(v) = table.get("reasoning_effort").and_then(toml::Value::as_str)
&& !v.is_empty()
{
config.reasoning_effort = Some(v.to_string());
}
if let Some(v) = table.get("approval_policy").and_then(toml::Value::as_str)
@@ -19497,6 +19496,21 @@ sandbox_mode = "read-only"
assert_eq!(config.sandbox_mode.as_deref(), Some("read-only"));
}
#[test]
fn project_overlay_ignores_notes_path() {
// The auto-approved `note` tool appends to `notes_path`; a project
// config must not choose that write target.
for value in ["~/.zshrc", "/etc/profile", "../../.bashrc", "docs/notes.md"] {
let tmp = workspace_with_project_config(&format!("notes_path = {value:?}\n"));
let mut config = Config::default();
merge_project_config(&mut config, tmp.path());
assert_eq!(
config.notes_path, None,
"project notes_path {value:?} must be ignored"
);
}
}
#[test]
fn project_overlay_denies_approval_auto_and_sandbox_danger_values() {
// #417 value-deny: the loosest values (`approval_policy = "auto"`,
crates/tui/src/mcp/streamable_http.rs
+3 -11
View File
@@ -218,17 +218,9 @@ pub(super) async fn read_body_capped(
response: reqwest::Response,
max_bytes: usize,
) -> Result<String> {
use futures_util::StreamExt;
let mut stream = response.bytes_stream();
let mut buf: Vec<u8> = Vec::new();
while let Some(chunk) = stream.next().await {
let chunk = chunk.context("failed to read MCP response body")?;
if buf.len().saturating_add(chunk.len()) > max_bytes {
anyhow::bail!("MCP response body exceeds {max_bytes} bytes — aborting");
}
buf.extend_from_slice(&chunk);
}
let buf = crate::utils::read_response_body_capped(response, max_bytes)
.await
.map_err(|error| anyhow::anyhow!("MCP {error:#}"))?;
Ok(String::from_utf8_lossy(&buf).into_owned())
}
crates/tui/src/runtime_api/git.rs
+20 -15
View File
@@ -385,18 +385,10 @@ pub(super) async fn git_diff(
require_repo(&workspace)?;
let base = diff_base(&workspace).await?;
let path_arg = path.to_string_lossy().into_owned();
let run = git_read(
&workspace,
&[
"diff",
"--no-color",
"--no-ext-diff",
&base,
"--",
&path_arg,
],
)
.await?;
let mut args = vec!["diff", "--no-color"];
args.extend(Git::REVIEW_DIFF_ARGS);
args.extend([base.as_str(), "--", &path_arg]);
let run = git_read(&workspace, &args).await?;
if !run.status_success {
return Err(ApiError::internal(format!(
"git diff failed: {}",
@@ -407,7 +399,14 @@ pub(super) async fn git_diff(
let untracked = if diff.is_empty() {
let status = git_read(
&workspace,
&["status", "--porcelain=v1", "-z", "--", &path_arg],
&[
"status",
"--porcelain=v1",
"-z",
"--ignore-submodules=dirty",
"--",
&path_arg,
],
)
.await?;
status
@@ -451,7 +450,10 @@ pub(super) async fn workspace_diff(
require_repo(&workspace)?;
let base = diff_base(&workspace).await?;
let numstat = git_read(&workspace, &["diff", "--numstat", &base]).await?;
let mut numstat_args = vec!["diff", "--numstat"];
numstat_args.extend(Git::REVIEW_DIFF_ARGS);
numstat_args.push(&base);
let numstat = git_read(&workspace, &numstat_args).await?;
if !numstat.status_success {
return Err(ApiError::internal(format!(
"git diff --numstat failed: {}",
@@ -475,7 +477,10 @@ pub(super) async fn workspace_diff(
})
.collect();
let run = git_read(&workspace, &["diff", "--no-color", "--no-ext-diff", &base]).await?;
let mut diff_args = vec!["diff", "--no-color"];
diff_args.extend(Git::REVIEW_DIFF_ARGS);
diff_args.push(&base);
let run = git_read(&workspace, &diff_args).await?;
if !run.status_success {
return Err(ApiError::internal(format!(
"git diff failed: {}",
crates/tui/src/runtime_threads.rs
+38 -31
View File
@@ -291,6 +291,13 @@ fn strip_summary_section(base: &str) -> String {
out
}
/// A fresh durable record id: `<prefix>_<32 hex>`. Records live in flat
/// per-kind directories and saves replace by id, so ids carry a full UUID; an
/// 8-hex suffix (32 bits) collides often enough to overwrite another record.
fn runtime_record_id(prefix: &str) -> String {
format!("{prefix}_{}", Uuid::new_v4().simple())
}
fn validated_record_id<'a>(id: &'a str, label: &str) -> Result<&'a str> {
let trimmed = id.trim();
if trimmed.is_empty() {
@@ -1410,7 +1417,7 @@ fn unaccepted_routed_usage_turn_id(
)
.collect::<Vec<_>>();
if identities.is_empty() {
return format!("turn_unaccepted_{}", &Uuid::new_v4().to_string()[..8]);
return runtime_record_id("turn_unaccepted");
}
identities.sort_unstable();
identities.dedup();
@@ -7942,7 +7949,7 @@ impl RuntimeThreadManager {
let thread = ThreadRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("thr_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("thr"),
created_at: now,
updated_at: now,
model,
@@ -8178,7 +8185,7 @@ impl RuntimeThreadManager {
codewhale_config::notifications::NotificationEvent::parse(kind).is_some(),
"notice kind must be a NotificationEvent name: {kind}"
);
let id = format!("notice_{}", &Uuid::new_v4().to_string()[..8]);
let id = runtime_record_id("notice");
let mut notices = self.notices.lock();
let list = notices.entry(thread_id.to_string()).or_default();
list.push(ActiveNotice {
@@ -9126,7 +9133,7 @@ impl RuntimeThreadManager {
let source = self.get_thread(id).await?;
let mut forked = source.clone();
let now = Utc::now();
forked.id = format!("thr_{}", &Uuid::new_v4().to_string()[..8]);
forked.id = runtime_record_id("thr");
forked.created_at = now;
forked.updated_at = now;
forked.latest_turn_id = None;
@@ -9168,7 +9175,7 @@ impl RuntimeThreadManager {
let mut cloned_records = Vec::with_capacity(source_turns.len());
for source_turn in source_turns {
let mut cloned_turn = source_turn.clone();
cloned_turn.id = format!("turn_{}", &Uuid::new_v4().to_string()[..8]);
cloned_turn.id = runtime_record_id("turn");
cloned_turn.thread_id = forked.id.clone();
if let Some(checkpoint) = forked.saved_session_checkpoint.as_mut()
&& checkpoint.covered_turn_id.as_deref() == Some(source_turn.id.as_str())
@@ -9181,7 +9188,7 @@ impl RuntimeThreadManager {
let mut cloned_items = Vec::with_capacity(items.len());
for item in items {
let mut cloned_item = item.clone();
cloned_item.id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
cloned_item.id = runtime_record_id("item");
cloned_item.turn_id = cloned_turn.id.clone();
cloned_turn.item_ids.push(cloned_item.id.clone());
cloned_items.push(cloned_item);
@@ -9495,7 +9502,7 @@ impl RuntimeThreadManager {
// between the two paths.
let mut forked = source.clone();
let now = Utc::now();
forked.id = format!("thr_{}", &Uuid::new_v4().to_string()[..8]);
forked.id = runtime_record_id("thr");
forked.created_at = now;
forked.updated_at = now;
forked.latest_turn_id = None;
@@ -9609,7 +9616,7 @@ impl RuntimeThreadManager {
let mut cloned_records = Vec::with_capacity(cutoff_turn_idx);
for source_turn in source_turns.iter().take(cutoff_turn_idx) {
let mut cloned_turn = source_turn.clone();
cloned_turn.id = format!("turn_{}", &Uuid::new_v4().to_string()[..8]);
cloned_turn.id = runtime_record_id("turn");
cloned_turn.thread_id = forked.id.clone();
if let Some(checkpoint) = forked.saved_session_checkpoint.as_mut()
&& checkpoint.covered_turn_id.as_deref() == Some(source_turn.id.as_str())
@@ -9622,7 +9629,7 @@ impl RuntimeThreadManager {
let mut cloned_items = Vec::with_capacity(items.len());
for item in items {
let mut cloned_item = item.clone();
cloned_item.id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
cloned_item.id = runtime_record_id("item");
cloned_item.turn_id = cloned_turn.id.clone();
cloned_turn.item_ids.push(cloned_item.id.clone());
cloned_items.push(cloned_item);
@@ -9948,14 +9955,14 @@ impl RuntimeThreadManager {
for turn_seed in turns {
let turn_at = next_seed_stamp();
let turn_id = format!("turn_{}", &Uuid::new_v4().to_string()[..8]);
let turn_id = runtime_record_id("turn");
let summary =
crate::utils::truncate_with_ellipsis(&turn_seed.user_text, SUMMARY_LIMIT, "...");
let mut item_ids = Vec::new();
// Save user message item.
if !turn_seed.user_text.is_empty() || !turn_seed.image_content.is_empty() {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
let item_at = next_seed_stamp();
let mut item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
@@ -9980,7 +9987,7 @@ impl RuntimeThreadManager {
// Save assistant content items in order.
for seed_item in &turn_seed.items {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
let item_at = next_seed_stamp();
match seed_item {
SeedItem::Text(text) => {
@@ -10179,7 +10186,7 @@ impl RuntimeThreadManager {
.transpose()?;
let turn_id = match requested_turn_id.as_deref() {
Some(turn_id) => turn_id.to_string(),
None => format!("turn_{}", &Uuid::new_v4().to_string()[..8]),
None => runtime_record_id("turn"),
};
Ok(Some(PreparedRuntimeTurnOperation {
binding: RuntimeTurnOperationBinding {
@@ -11246,7 +11253,7 @@ impl RuntimeThreadManager {
let turn_id = operation
.as_ref()
.map(|operation| operation.binding.turn_id.clone())
.unwrap_or_else(|| format!("turn_{}", &Uuid::new_v4().to_string()[..8]));
.unwrap_or_else(|| runtime_record_id("turn"));
compaction.runtime_cost_owner = Some(turn_id.clone());
let input_summary = req
.input_summary
@@ -11295,7 +11302,7 @@ impl RuntimeThreadManager {
// including this classifier batch. Pre-persisting the count here and
// adding TurnComplete at settlement would count the same gap twice.
let user_item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let user_item_id = runtime_record_id("item");
let mut user_item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: user_item_id.clone(),
@@ -11599,7 +11606,7 @@ impl RuntimeThreadManager {
let queued_turn;
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.to_string(),
kind: TurnItemKind::UserMessage,
// Queued, not Completed: the text is in the engine's mailbox, not
@@ -11730,8 +11737,8 @@ impl RuntimeThreadManager {
);
let now = Utc::now();
let turn_id = format!("turn_{}", &Uuid::new_v4().to_string()[..8]);
let compaction_id = format!("compact_{}", &Uuid::new_v4().to_string()[..8]);
let turn_id = runtime_record_id("turn");
let compaction_id = runtime_record_id("compact");
compaction.runtime_cost_owner = Some(turn_id.clone());
// The same projection the turn record receipts, computed once: the
// compaction runs under the thread's persisted policy.
@@ -12765,7 +12772,7 @@ impl RuntimeThreadManager {
}
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.to_string(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -12965,7 +12972,7 @@ impl RuntimeThreadManager {
}
}
EngineEvent::MessageStarted { .. } => {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: item_id.clone(),
@@ -13038,7 +13045,7 @@ impl RuntimeThreadManager {
}
}
EngineEvent::ThinkingStarted { .. } => {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: item_id.clone(),
@@ -13107,7 +13114,7 @@ impl RuntimeThreadManager {
}
}
EngineEvent::ToolCallStarted { id, name, input } => {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
tool_items.insert(id.clone(), item_id.clone());
let kind = tool_kind_for_name(&name);
let summary = summarize_text(&format!("{name} started"), SUMMARY_LIMIT);
@@ -13334,7 +13341,7 @@ impl RuntimeThreadManager {
}
}
EngineEvent::CompactionStarted { id, auto, message } => {
let item_id = format!("item_{}", &Uuid::new_v4().to_string()[..8]);
let item_id = runtime_record_id("item");
compaction_items.insert(id.clone(), item_id.clone());
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
@@ -13471,7 +13478,7 @@ impl RuntimeThreadManager {
);
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -13506,7 +13513,7 @@ impl RuntimeThreadManager {
let message = format!("Sub-agent {id}: {status}");
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -13552,7 +13559,7 @@ impl RuntimeThreadManager {
);
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -13621,7 +13628,7 @@ impl RuntimeThreadManager {
);
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -14140,7 +14147,7 @@ impl RuntimeThreadManager {
);
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Status,
status: TurnItemLifecycleStatus::Completed,
@@ -14173,7 +14180,7 @@ impl RuntimeThreadManager {
let message = envelope.message.clone();
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Error,
status: TurnItemLifecycleStatus::Failed,
@@ -14395,7 +14402,7 @@ impl RuntimeThreadManager {
turn_error = Some(EMPTY_TURN_REASON.to_string());
let item = TurnItemRecord {
schema_version: CURRENT_RUNTIME_SCHEMA_VERSION,
id: format!("item_{}", &Uuid::new_v4().to_string()[..8]),
id: runtime_record_id("item"),
turn_id: turn_id.clone(),
kind: TurnItemKind::Error,
status: TurnItemLifecycleStatus::Failed,
@@ -14856,7 +14863,7 @@ impl crate::tools::spec::DynamicToolExecutor for RuntimeThreadManager {
"runtime dynamic tool '{name}' has no active turn"
))
})?;
let call_id = format!("call_{}", &Uuid::new_v4().to_string()[..8]);
let call_id = runtime_record_id("call");
let params = DynamicToolCallParams {
thread_id: thread_id.clone(),
turn_id: turn_id.clone(),
crates/tui/src/runtime_threads/tests.rs
+15
View File
@@ -1492,6 +1492,21 @@ const EVENT_PROCESS_HELPER: &str = "runtime_threads::tests::runtime_event_proces
// a test watchdog, not an expected runtime latency or a customer-facing SLO.
const TURN_SETTLEMENT_DEADLOCK_TIMEOUT: Duration = Duration::from_secs(10);
/// Durable record ids carry a full UUID: records live in flat per-kind
/// directories and a save replaces by id, so a short suffix would let one
/// thread's record overwrite another's.
#[test]
fn runtime_record_ids_carry_a_full_uuid() {
let id = runtime_record_id("thr");
let suffix = id.strip_prefix("thr_").expect("prefix");
assert_eq!(suffix.len(), 32, "{id}");
assert!(suffix.bytes().all(|byte| byte.is_ascii_hexdigit()), "{id}");
assert_eq!(validated_record_id(&id, "thread id").expect("valid"), id);
let ids: std::collections::HashSet<String> =
(0..10_000).map(|_| runtime_record_id("item")).collect();
assert_eq!(ids.len(), 10_000);
}
#[test]
#[ignore = "spawned by real cross-process Runtime event tests"]
fn runtime_event_process_child_helper() {
crates/tui/src/skills/install.rs
+87 -25
View File
@@ -76,6 +76,8 @@ pub const DEFAULT_REGISTRY_URL: &str =
/// gzip bomb can't blow up RAM.
pub const DEFAULT_MAX_SIZE_BYTES: u64 = 5 * 1024 * 1024;
const SYNC_REGISTRY_CONCURRENCY: usize = 8;
/// Upper bound on a registry index document.
const MAX_REGISTRY_BYTES: usize = 4 * 1024 * 1024;
/// File written under each installed skill so [`update`] / [`uninstall`] can
/// recover the original [`InstallSource`] without re-parsing user input.
@@ -554,17 +556,17 @@ pub async fn fetch_registry(
Decision::Deny => return Ok(RegistryFetchResult::Denied(host)),
Decision::Prompt => return Ok(RegistryFetchResult::NeedsApproval(host)),
}
let body = reqwest_client()
let response = reqwest_client()
.get(registry_url)
.send()
.await
.with_context(|| format!("failed to fetch registry {registry_url}"))?
.error_for_status()
.with_context(|| format!("registry {registry_url} returned an error status"))?
.text()
.with_context(|| format!("registry {registry_url} returned an error status"))?;
let body = crate::utils::read_response_body_capped(response, MAX_REGISTRY_BYTES)
.await
.with_context(|| format!("failed to read registry body from {registry_url}"))?;
let parsed: RegistryDocument = serde_json::from_str(&body)
let parsed: RegistryDocument = serde_json::from_slice(&body)
.with_context(|| format!("failed to parse registry json from {registry_url}"))?;
Ok(RegistryFetchResult::Loaded(parsed))
}
@@ -764,24 +766,18 @@ async fn sync_one_skill(
.and_then(|v| v.to_str().ok())
.map(|s| s.to_string());
let compressed_cap = max_size.saturating_mul(4);
let bytes = match resp.bytes().await {
let compressed_cap = compressed_download_cap(max_size);
let bytes = match crate::utils::read_response_body_capped(resp, compressed_cap).await {
Ok(b) => b,
Err(err) => {
return SkillSyncOutcome::Failed {
name: name.to_string(),
reason: format!("failed to read body from {url}: {err:#}"),
reason: format!(
"failed to read body from {url} (compressed size cap {compressed_cap} bytes): {err:#}"
),
};
}
};
if bytes.len() as u64 > compressed_cap {
return SkillSyncOutcome::Failed {
name: name.to_string(),
reason: format!(
"download from {url} exceeds compressed size cap ({compressed_cap} bytes)"
),
};
}
// Compute SHA-256 of the downloaded bytes.
let sha256 = sha256_hex(&bytes);
@@ -1158,17 +1154,19 @@ async fn download_with_cap(url: &str, max_size: u64) -> Result<DownloadAttempt>
}
bail!("download {url} returned {status}");
}
// Soft cap on the *compressed* download — well above max_size to allow
// for highly compressible payloads but still bounded.
let compressed_cap = max_size.saturating_mul(4);
let bytes = resp
.bytes()
let compressed_cap = compressed_download_cap(max_size);
let bytes = crate::utils::read_response_body_capped(resp, compressed_cap)
.await
.with_context(|| format!("failed to read body of {url}"))?;
if (bytes.len() as u64) > compressed_cap {
bail!("download {url} exceeds compressed size cap of {compressed_cap} bytes");
}
Ok(DownloadAttempt::Bytes(bytes.to_vec()))
.with_context(|| {
format!("failed to read body of {url} (compressed size cap {compressed_cap} bytes)")
})?;
Ok(DownloadAttempt::Bytes(bytes))
}
/// Soft cap on the *compressed* download — well above `max_size` to allow for
/// highly compressible payloads but still bounded.
fn compressed_download_cap(max_size: u64) -> usize {
usize::try_from(max_size.saturating_mul(4)).unwrap_or(usize::MAX)
}
struct StagedSkill {
@@ -1668,6 +1666,70 @@ fn hex_bytes(bytes: impl AsRef<[u8]>) -> String {
mod tests {
use super::*;
/// Serve one response per connection: an endless chunked body, or a
/// small body that declares an oversized `Content-Length`.
async fn oversized_body_server() -> std::net::SocketAddr {
use tokio::io::{AsyncReadExt, AsyncWriteExt};
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
tokio::spawn(async move {
while let Ok((mut socket, _)) = listener.accept().await {
tokio::spawn(async move {
let mut request = Vec::new();
let mut buf = [0u8; 1024];
while let Ok(n) = socket.read(&mut buf).await {
if n == 0 {
return;
}
request.extend_from_slice(&buf[..n]);
if request.windows(4).any(|window| window == b"\r\n\r\n") {
break;
}
}
if request.starts_with(b"GET /declared ") {
let _ = socket
.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 999999999\r\n\r\nxx")
.await;
return;
}
let _ = socket
.write_all(b"HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n")
.await;
let chunk = [b'x'; 4096];
// Never terminates: only a streaming cap ends this read.
loop {
if socket.write_all(b"1000\r\n").await.is_err()
|| socket.write_all(&chunk).await.is_err()
|| socket.write_all(b"\r\n").await.is_err()
{
return;
}
}
});
}
});
addr
}
#[tokio::test]
async fn download_with_cap_stops_reading_at_the_cap() {
let addr = oversized_body_server().await;
for path in ["endless", "declared"] {
let url = format!("http://{addr}/{path}");
let result = tokio::time::timeout(
std::time::Duration::from_secs(20),
download_with_cap(&url, 16 * 1024),
)
.await
.unwrap_or_else(|_| panic!("{path}: the size cap must end the read"));
let err = match result {
Ok(_) => panic!("{path}: an oversized body must be refused"),
Err(err) => format!("{err:#}"),
};
assert!(err.contains("exceeds"), "{path}: {err}");
}
}
#[test]
fn parse_github_source() {
let s = InstallSource::parse("github:Hmbown/test-skill").unwrap();
crates/tui/src/tools/git.rs
+159 -11
View File
@@ -209,9 +209,9 @@ impl ToolSpec for GitDiffTool {
"core.quotepath=false".to_string(),
"diff".to_string(),
"--no-color".to_string(),
"--no-ext-diff".to_string(),
format!("--unified={unified}"),
];
args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from));
args.push(format!("--unified={unified}"));
if cached {
args.push("--cached".to_string());
}
@@ -221,7 +221,7 @@ impl ToolSpec for GitDiffTool {
}
let command_str = format_command(&git_ctx.working_dir, &args);
let output = run_git_command(&git_ctx.working_dir, &args)?;
let output = run_git_review_command(&git_ctx.working_dir, &args)?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
@@ -318,15 +318,16 @@ impl ToolSpec for GitCommitPlanTool {
"diff".to_string(),
"HEAD".to_string(),
"--no-color".to_string(),
"--no-ext-diff".to_string(),
"-U3".to_string(),
];
diff_args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from));
if let Some(pathspec) = &git_ctx.pathspec {
diff_args.push("--".to_string());
diff_args.push(pathspec.display().to_string());
}
let command = format_command(working_dir, &diff_args);
let mut files = match git_stdout(working_dir, &diff_args)? {
let diff_output = run_git_review_command(working_dir, &diff_args)?;
let mut files = match stdout_or_failure(working_dir, &diff_args, diff_output) {
Ok(stdout) => parse_diff(&String::from_utf8_lossy(&stdout)),
Err(failure) => return Ok(failure),
};
@@ -374,13 +375,16 @@ impl ToolSpec for GitCommitPlanTool {
);
}
let staged_args = vec![
let mut staged_args = vec![
"diff".to_string(),
"--cached".to_string(),
"--quiet".to_string(),
];
let index_has_staged_changes =
run_git_command(working_dir, &staged_args)?.status.code() == Some(1);
staged_args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from));
let index_has_staged_changes = run_git_review_command(working_dir, &staged_args)?
.status
.code()
== Some(1);
let commits = match plan_commits(files) {
Ok(commits) => commits,
@@ -512,6 +516,24 @@ fn run_git_command(working_dir: &Path, args: &[String]) -> Result<std::process::
})
}
/// Run a content-reading `diff` under [`crate::dependencies::Git::review_command`],
/// so repository-configured filters, fsmonitor and hooks do not execute.
pub(super) fn run_git_review_command(
working_dir: &Path,
args: &[String],
) -> Result<std::process::Output, ToolError> {
if crate::dependencies::Git::command().is_none() {
return Err(ToolError::not_available(
"git is not installed or not in PATH",
));
}
let mut cmd = crate::dependencies::Git::review_command(working_dir)
.map_err(|e| ToolError::execution_failed(format!("Failed to prepare git: {e:#}")))?;
cmd.args(args);
cmd.output()
.map_err(|e| ToolError::execution_failed(format!("Failed to run git: {e}")))
}
fn format_command(working_dir: &Path, args: &[String]) -> String {
// `[String]::join` produces the same string as collecting `&str` first, so
// join the slice directly and skip the intermediate `Vec<&str>` allocation.
@@ -643,15 +665,23 @@ fn git_stdout(
args: &[String],
) -> Result<Result<Vec<u8>, ToolResult>, ToolError> {
let output = run_git_command(working_dir, args)?;
Ok(stdout_or_failure(working_dir, args, output))
}
fn stdout_or_failure(
working_dir: &Path,
args: &[String],
output: std::process::Output,
) -> Result<Vec<u8>, ToolResult> {
if output.status.success() {
return Ok(Ok(output.stdout));
return Ok(output.stdout);
}
let stderr = String::from_utf8_lossy(&output.stderr);
Ok(Err(ToolResult::error(format!(
Err(ToolResult::error(format!(
"{} failed: {}",
format_command(working_dir, args),
stderr.trim()
))))
)))
}
/// Parse `git diff` output into per-file hunks. Every `diff --git` section
@@ -1722,4 +1752,122 @@ Binary files a/image.png and b/image.png differ
result.content
);
}
/// Diff and show reads must not run commands a repository configures:
/// a superproject textconv driver, or — through a child git spawned in a
/// submodule for its dirty check or `diff.submodule=diff` — a submodule's
/// clean filter or external diff driver.
#[cfg(unix)]
#[tokio::test]
async fn diff_and_show_reads_run_no_repository_configured_commands() {
use std::os::unix::fs::PermissionsExt;
if !git_available() {
return;
}
let tmp = tempdir().expect("tempdir");
let marker = tmp.path().join("marker");
let script = |name: &str, body: &str| {
let path = tmp.path().join(name);
fs::write(
&path,
format!("#!/bin/sh\necho {name} >> '{}'\n{body}", marker.display()),
)
.expect("write script");
fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).expect("chmod");
path.display().to_string()
};
let clean = script("clean.sh", "cat\n");
let external = script("external.sh", "");
let textconv = script("textconv.sh", "cat \"$1\"\n");
let git = |dir: &Path, args: &[&str]| {
let status = crate::dependencies::Git::status(args, dir).expect("git should spawn");
assert!(status.success(), "git {args:?} failed");
};
let identity = ["-c", "[email protected]", "-c", "user.name=Test"];
let upstream = tmp.path().join("upstream");
fs::create_dir_all(&upstream).expect("mkdir upstream");
init_git_repo(&upstream);
fs::write(upstream.join("f.txt"), "a\n").expect("write");
commit_all(&upstream, "init");
let repo = tmp.path().join("repo");
fs::create_dir_all(&repo).expect("mkdir repo");
init_git_repo(&repo);
fs::write(repo.join(".gitattributes"), "a.md diff=conv\n").expect("attrs");
fs::write(repo.join("a.md"), "one\n").expect("write");
let upstream_arg = upstream.display().to_string();
git(
&repo,
&[
"-c",
"protocol.file.allow=always",
"submodule",
"add",
"-q",
&upstream_arg,
"sub",
],
);
commit_all(&repo, "init");
let sub = repo.join("sub");
fs::write(sub.join(".gitattributes"), "* filter=evil\n").expect("sub attrs");
git(&sub, &[&identity[..], &["add", "."]].concat());
git(
&sub,
&[&identity[..], &["commit", "-q", "-m", "attrs"]].concat(),
);
git(&sub, &["config", "filter.evil.clean", &clean]);
git(&sub, &["config", "diff.external", &external]);
let edited = sub.join("f.txt");
fs::write(&edited, "c\n").expect("same-size edit");
fs::File::options()
.write(true)
.open(&edited)
.and_then(|file| file.set_modified(std::time::UNIX_EPOCH))
.expect("age the edit so git re-reads it");
git(&repo, &["config", "diff.submodule", "diff"]);
git(&repo, &["config", "submodule.sub.ignore", "none"]);
git(&repo, &["config", "diff.conv.textconv", &textconv]);
fs::write(repo.join("a.md"), "two\n").expect("modify");
let no_marker = |step: &str| {
assert!(
!marker.exists(),
"{step} ran a repository-configured command: {}",
fs::read_to_string(&marker).unwrap_or_default()
);
};
let ctx = ToolContext::new(&repo);
let diff = GitDiffTool
.execute(json!({}), &ctx)
.await
.expect("git_diff");
no_marker("git_diff");
assert!(diff.success, "{}", diff.content);
assert!(
diff.content.contains("Subproject commit"),
"{}",
diff.content
);
assert!(diff.content.contains("+two"), "{}", diff.content);
git(&repo, &["add", "sub"]);
git(
&repo,
&[&identity[..], &["commit", "-q", "-m", "bump", "--", "sub"]].concat(),
);
let show = super::super::git_history::GitShowTool
.execute(json!({"rev": "HEAD"}), &ctx)
.await
.expect("git_show");
no_marker("git_show");
assert!(show.success, "{}", show.content);
assert!(
show.content.contains("Subproject commit"),
"{}",
show.content
);
}
}
crates/tui/src/tools/git_history.rs
+40 -8
View File
@@ -218,11 +218,8 @@ impl ToolSpec for GitShowTool {
let stat = optional_bool(&input, "stat", true)?;
let unified = optional_u64(&input, "unified", DEFAULT_UNIFIED)?.min(MAX_UNIFIED);
let mut args = vec![
"show".to_string(),
"--no-color".to_string(),
"--no-ext-diff".to_string(),
];
let mut args = vec!["show".to_string(), "--no-color".to_string()];
args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from));
if patch {
args.push(format!("--unified={unified}"));
} else {
@@ -238,7 +235,12 @@ impl ToolSpec for GitShowTool {
}
let command_str = format_command(&git_ctx.working_dir, &args);
let output = run_git_command_async(git_ctx.working_dir.clone(), args).await?;
let working_dir = git_ctx.working_dir.clone();
let output = tokio::task::spawn_blocking(move || {
super::git::run_git_review_command(&working_dir, &args)
})
.await
.map_err(|e| ToolError::execution_failed(format!("git task panicked: {e}")))??;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Ok(ToolResult::error(format!(
@@ -452,7 +454,7 @@ impl ToolSpec for GitFetchTool {
"refspecs": {
"type": "array",
"items": { "type": "string" },
"description": "Optional refspecs to fetch (e.g. pull/123/head). Empty fetches the remote's defaults."
"description": "Optional refspecs to fetch (e.g. pull/123/head); a `src:dst` destination must be under refs/remotes/. Empty fetches the remote's defaults."
},
"path": {
"type": "string",
@@ -790,9 +792,19 @@ fn validate_git_refspec(refspec: &str) -> Result<(), ToolError> {
"git refspec '{refspec}' must have at most one ':'"
)));
}
for side in parts {
for side in &parts {
validate_git_refspec_side(refspec, side)?;
}
// The tool updates remote-tracking refs only: a destination under
// `refs/heads/` or `refs/tags/` (or a bare name git would resolve there)
// could rewrite local branches and tags.
if let Some(dst) = parts.get(1)
&& (!dst.starts_with("refs/remotes/") || dst.split('/').any(|part| part == ".."))
{
return Err(ToolError::invalid_input(format!(
"git refspec '{refspec}' must write under refs/remotes/; omit the destination to fetch into FETCH_HEAD"
)));
}
Ok(())
}
@@ -1228,6 +1240,26 @@ mod tests {
"{refspec}: {err}"
);
}
for refspec in [
"+main:refs/heads/main",
"v1:refs/tags/v1",
"main:other",
"main:refs/remotes/../heads/main",
] {
let err = validate_git_refspec(refspec)
.expect_err("a destination outside refs/remotes/ must be refused");
assert!(
err.to_string().contains("refs/remotes/"),
"{refspec}: {err}"
);
}
for refspec in [
"pull/123/head",
"refs/heads/x:refs/remotes/origin/x",
"+refs/heads/*:refs/remotes/origin/*",
] {
validate_git_refspec(refspec).expect(refspec);
}
let err = GitFetchTool
.execute(json!({ "refspecs": "pull/1/head" }), &ctx)
.await
crates/tui/src/tools/git_tool.rs
+1 -1
View File
@@ -144,7 +144,7 @@ impl ToolSpec for GitTool {
"refspecs": {
"type": "array",
"items": { "type": "string" },
"description": "Optional refspecs to fetch, e.g. pull/123/head (action=fetch). Empty fetches the remote's defaults."
"description": "Optional refspecs to fetch, e.g. pull/123/head (action=fetch); a `src:dst` destination must be under refs/remotes/. Empty fetches the remote's defaults."
},
"ours": {
"type": "string",
crates/tui/src/tools/review.rs
+2 -5
View File
@@ -1730,11 +1730,8 @@ async fn resolve_diff_target(
None
};
let mut args = vec![
"diff".to_string(),
"--no-ext-diff".to_string(),
"--no-textconv".to_string(),
];
let mut args = vec!["diff".to_string()];
args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from));
if staged {
args.push("--cached".to_string());
if let Some(base_commit) = base_commit {
crates/tui/src/tools/review_pr.rs
+20 -19
View File
@@ -286,25 +286,26 @@ fn diff_with(
if !commit_id(base) {
bail!("The pinned PR commits do not have one available merge base");
}
let diff = run(
Program::Git,
&[
"diff".into(),
"--no-ext-diff".into(),
"--no-textconv".into(),
"--no-color".into(),
"--no-relative".into(),
"--full-index".into(),
"--find-renames=50%".into(),
"--src-prefix=a/".into(),
"--dst-prefix=b/".into(),
"--ignore-submodules=none".into(),
"--submodule=short".into(),
base.into(),
view.head_sha.clone(),
"--".into(),
],
)?;
let mut diff_args = vec!["diff".to_string()];
diff_args.extend(Git::REVIEW_DIFF_ARGS.map(String::from));
diff_args.extend(
[
"--no-color",
"--no-relative",
"--full-index",
"--find-renames=50%",
"--src-prefix=a/",
"--dst-prefix=b/",
// Commit-to-commit: no worktree is read, so every
// submodule pointer change belongs in the file set.
"--ignore-submodules=none",
base,
view.head_sha.as_str(),
"--",
]
.map(String::from),
);
let diff = run(Program::Git, &diff_args)?;
complete_file_set(&diff, view)?;
Ok(diff)
})();
crates/tui/src/tools/shell.rs
+54 -6
View File
@@ -4187,18 +4187,17 @@ fn hardened_readonly_argv(command: &str) -> Result<(String, Vec<String>)> {
let at = subcommand_index + 1;
argv.splice(
at..at,
["--no-ext-diff".to_string(), "--no-textconv".to_string()],
crate::dependencies::Git::REVIEW_DIFF_ARGS.map(String::from),
);
}
"log" | "show" => {
let at = subcommand_index + 1;
argv.splice(
at..at,
[
"--no-ext-diff".to_string(),
"--no-textconv".to_string(),
"--no-show-signature".to_string(),
],
crate::dependencies::Git::REVIEW_DIFF_ARGS
.into_iter()
.chain(["--no-show-signature"])
.map(String::from),
);
}
"status" | "ls-files" | "blame" | "grep" => {}
@@ -6566,6 +6565,49 @@ fn shell_delta_with_accumulated_output(
}
}
/// Refuse a notes target this auto-approved tool must not append to.
///
/// The file itself must not be a symlink (a committed `notes.md -> ~/.zshrc`
/// would redirect the append), and a target placed inside the workspace must
/// resolve inside it after symlinked parent directories are followed.
async fn ensure_notes_target_is_safe(
notes_path: &std::path::Path,
workspace: &std::path::Path,
) -> Result<(), ToolError> {
if let Ok(meta) = tokio::fs::symlink_metadata(notes_path).await
&& (meta.file_type().is_symlink() || !meta.is_file())
{
return Err(ToolError::permission_denied(format!(
"Refusing to append a note to {}: the notes path is a symlink or not a regular file.",
notes_path.display()
)));
}
if notes_path.starts_with(workspace)
&& let (Some(parent), Ok(root)) = (
notes_path.parent(),
tokio::fs::canonicalize(workspace).await,
)
{
// Walk up to the nearest existing ancestor: the rest is created
// below as real directories, so only existing links can redirect.
let mut existing = parent.to_path_buf();
while !tokio::fs::try_exists(&existing).await.unwrap_or(false) {
if !existing.pop() {
break;
}
}
if let Ok(resolved) = tokio::fs::canonicalize(&existing).await
&& !resolved.starts_with(&root)
{
return Err(ToolError::permission_denied(format!(
"Refusing to append a note to {}: its directory resolves outside the workspace.",
notes_path.display()
)));
}
}
Ok(())
}
/// Tool for appending notes to a notes file.
pub struct NoteTool;
@@ -6606,6 +6648,7 @@ impl ToolSpec for NoteTool {
context: &ToolContext,
) -> Result<ToolResult, ToolError> {
let note_content = required_str(&input, "content")?;
ensure_notes_target_is_safe(&context.notes_path, &context.workspace).await?;
// Ensure parent directory exists. Tool handlers run on the Tokio
// runtime, so filesystem calls use tokio::fs (blocking-call
@@ -6628,6 +6671,11 @@ impl ToolSpec for NoteTool {
file.write_all(format!("\n---\n{note_content}\n").as_bytes())
.await
.map_err(|e| ToolError::execution_failed(format!("Failed to write note: {e}")))?;
// A tokio file finishes writes in the background; flush so the note
// is on disk when the tool reports success.
file.flush()
.await
.map_err(|e| ToolError::execution_failed(format!("Failed to write note: {e}")))?;
Ok(ToolResult::success(format!(
"Note appended to {}",
crates/tui/src/tools/shell/tests.rs
+53 -1
View File
@@ -945,11 +945,13 @@ fn readonly_argv_is_shell_free_and_disables_git_helpers() {
let (program, args) = hardened_readonly_argv("git show HEAD").expect("argv");
assert_eq!(program, "git");
assert_eq!(
&args[..4],
&args[..6],
[
"show",
"--no-ext-diff",
"--no-textconv",
"--submodule=short",
"--ignore-submodules=dirty",
"--no-show-signature"
]
);
@@ -4952,3 +4954,53 @@ fn pty_raw_stdin_roundtrips_nul_and_non_utf8_bytes() {
std::thread::sleep(Duration::from_millis(20));
}
}
/// The auto-approved `note` tool appends to the configured notes file. A
/// committed symlink (`notes.md -> ~/.zshrc`) or a symlinked notes directory
/// must not redirect that append outside the workspace.
#[cfg(unix)]
#[tokio::test]
async fn note_tool_refuses_symlinked_targets_that_leave_the_workspace() {
let workspace = tempdir().expect("workspace");
let outside = tempdir().expect("outside");
let rc = outside.path().join(".zshrc");
std::fs::write(&rc, "# rc\n").expect("write rc");
let linked_file = workspace.path().join("notes.md");
std::os::unix::fs::symlink(&rc, &linked_file).expect("symlink notes file");
let context = ToolContext::with_options(workspace.path(), false, &linked_file, "mcp.json");
let err = NoteTool
.execute(json!({"content": "appended text"}), &context)
.await
.expect_err("a symlinked notes file must be refused");
assert!(err.to_string().contains("symlink"), "{err}");
let linked_dir = workspace.path().join("notes");
std::os::unix::fs::symlink(outside.path(), &linked_dir).expect("symlink notes dir");
let context = ToolContext::with_options(
workspace.path(),
false,
linked_dir.join("sub/notes.md"),
"mcp.json",
);
let err = NoteTool
.execute(json!({"content": "appended text"}), &context)
.await
.expect_err("a notes dir resolving outside the workspace must be refused");
assert!(err.to_string().contains("outside the workspace"), "{err}");
assert_eq!(std::fs::read_to_string(&rc).expect("read rc"), "# rc\n");
assert!(!outside.path().join("sub").exists());
let plain = workspace.path().join("docs/notes.md");
let context = ToolContext::with_options(workspace.path(), false, &plain, "mcp.json");
NoteTool
.execute(json!({"content": "kept"}), &context)
.await
.expect("an ordinary in-workspace notes file is writable");
assert!(
std::fs::read_to_string(&plain)
.expect("read notes")
.contains("kept")
);
}
crates/tui/src/tools/subagent/delivery.rs
+4 -13
View File
@@ -208,19 +208,10 @@ impl DeliveryEvidence {
let mut candidates = status_paths(&baseline.root)?;
candidates.extend(baseline.dirty.keys().cloned());
if let Some(head) = baseline.head.as_deref() {
let output = git(
&baseline.root,
&[
"diff",
"--no-ext-diff",
"--no-textconv",
"--name-only",
"-z",
head,
"HEAD",
"--",
],
)?;
let mut args = vec!["diff"];
args.extend(crate::dependencies::Git::REVIEW_DIFF_ARGS);
args.extend(["--name-only", "-z", head, "HEAD", "--"]);
let output = git(&baseline.root, &args)?;
for path in output
.split(|byte| *byte == 0)
.filter(|path| !path.is_empty())
crates/tui/src/tools/verify.rs
+2 -1
View File
@@ -681,7 +681,8 @@ async fn run_git_diff(workspace: &Path, args: &[String]) -> Result<Option<String
// git not installed: degrade gracefully rather than failing the tool.
return Ok(None);
};
cmd.args(["diff", "--no-ext-diff", "--no-textconv"]);
cmd.arg("diff")
.args(crate::dependencies::Git::REVIEW_DIFF_ARGS);
for arg in args {
cmd.arg(arg);
}
crates/tui/src/utils.rs
+46 -5
View File
@@ -687,12 +687,40 @@ pub fn flush_and_sync(writer: &mut std::io::BufWriter<std::fs::File>) -> std::io
writer.get_ref().sync_all()
}
/// Read a whole response body, failing as soon as it would exceed `max_bytes`.
///
/// A declared `Content-Length` over the cap is refused before anything is
/// read; chunked or length-less bodies are bounded while streaming, so a
/// server cannot make the caller buffer an unbounded body before a size check.
pub async fn read_response_body_capped(
response: reqwest::Response,
max_bytes: usize,
) -> Result<Vec<u8>> {
use futures_util::StreamExt;
if let Some(len) = response.content_length()
&& len > max_bytes as u64
{
anyhow::bail!("response body of {len} bytes exceeds {max_bytes} bytes — aborting");
}
let mut stream = response.bytes_stream();
let mut buf: Vec<u8> = Vec::new();
while let Some(chunk) = stream.next().await {
let chunk = chunk.map_err(|e| anyhow::anyhow!("failed to read response body: {e}"))?;
if buf.len().saturating_add(chunk.len()) > max_bytes {
anyhow::bail!("response body exceeds {max_bytes} bytes — aborting");
}
buf.extend_from_slice(&chunk);
}
Ok(buf)
}
/// Open a URL in the system's default browser.
///
/// Dispatches to the platform-appropriate opener:
/// - macOS: `open`
/// - Linux / BSD: `xdg-open`
/// - Windows: `cmd /C start ""`
/// - Windows: `rundll32 url.dll,FileProtocolHandler`
/// - Other: returns an error.
///
/// This is the single entry point for URL opening — every call site in
@@ -733,10 +761,12 @@ fn browser_open_command(url: &str) -> Result<Command> {
Ok(command)
}
// Not `cmd /C start`: cmd.exe would parse `&`, `|`, `^` and `%` inside
// the URL. The protocol handler receives it as data.
#[cfg(target_os = "windows")]
{
let mut cmd = Command::new("cmd");
cmd.args(["/C", "start", "", url]);
let mut cmd = Command::new("rundll32");
cmd.args(["url.dll,FileProtocolHandler", url]);
Ok(cmd)
}
@@ -1908,13 +1938,24 @@ mod project_mapping_tests {
#[cfg(target_os = "windows")]
{
assert_eq!(command.get_program(), "cmd");
assert_eq!(command.get_program(), "rundll32");
assert_eq!(
command
.get_args()
.map(|arg| arg.to_string_lossy().into_owned())
.collect::<Vec<_>>(),
vec!["/C", "start", "", "https://example.com"]
vec!["url.dll,FileProtocolHandler", "https://example.com"]
);
// Shell metacharacters stay inside the single URL argument.
let url = "https://example.com/?a=1&b=2|x^y%PATH%";
let command = super::browser_open_command(url).expect("command");
assert_eq!(command.get_program(), "rundll32");
assert_eq!(
command
.get_args()
.last()
.map(|arg| arg.to_string_lossy().into_owned()),
Some(url.to_string())
);
}
}
docs/CONFIGURATION.md
+1 -2
View File
@@ -318,7 +318,6 @@ Supported keys in the project overlay (top-level fields only):
| `reasoning_effort` | force `"high"` / `"max"` for a complex repo |
| `approval_policy` | only values that tighten the user's current permission posture |
| `sandbox_mode` | only values that tighten the user's current sandbox posture |
| `notes_path` | keep notes in-repo |
| `max_subagents` | clamp sub-agent concurrency for a constrained repo (clamped to 1..=128) |
| `allow_shell` | `false` can disable shell access; `true` is ignored |
@@ -327,7 +326,7 @@ maintainer is most likely to want to standardize across contributors.
Credential, endpoint, provider-selection, MCP config, hooks, skills,
retry, hotbar bindings, and `instructions = [...]` settings stay user-global.
If a repo-local config declares `api_key`, `base_url`, `providers`, `provider`,
`mcp_config_path`, `hotbar`, `allow_shell = true`, or `instructions`,
`mcp_config_path`, `notes_path`, `hotbar`, `allow_shell = true`, or `instructions`,
Codewhale ignores that key and keeps the user's global setting.
The consolidated `codewhale` runtime uses one config file for DeepSeek auth