mirror of
https://github.com/instructkr/claw-code.git
synced 2026-10-23 00:00:22 +08:00
Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 gaebal-gajae
|
09731036ac |
No files matched your search
@@ -1 +0,0 @@
|
||||
{"schema_version":1,"session_id":"5e2dd1e8-ea4b-4af1-b56b-89ab7cca4299","state":"running","ready_for_input":false,"updated_at":"2026-10-12T22:09:30.659Z","current_turn_id":null,"last_turn_id":null,"live":true,"reason":null,"source":"agent_session_event","event":"turn_start","cwd":"/home/bellman/Data/Workspace/gc-task-cc-issue-3257-lane","workdir":"/home/bellman/Data/Workspace/gc-task-cc-issue-3257-lane","branch":null,"session_file":"/home/bellman/.gajaeway/broker/agent/sessions/v2-6fji37qf7ld523ivvoiohlcpmz3nu45m2apxyrkusn5nxczjmouq/2026-10-12T22-04-33-362Z_5e2dd1e8-ea4b-4af1-b56b-89ab7cca4299.jsonl","activity":{"seq":115,"last_activity_at":"2026-10-12T22:09:34.144Z","tool":"bash","phase":"started","outcome":null,"elapsed_ms":null,"active_tool_count":1,"active_tools":[{"tool":"bash","started_at":"2026-10-12T22:09:34.144Z"}],"in_flight":[{"digest":"e603a70efb03b6b2b78c94b704197d26fc8a35113543291a21f68b3b08ca11a9","tool":"bash","started_at":"2026-10-12T22:09:34.144Z"}]}}
|
||||
@@ -1 +0,0 @@
|
||||
{"pid":1,"start_time":"unknown","token":"1a2e8a33-76f2-4175-ac23-1521458f12a2","owner_host_id":"7af2fb0e8e37bd543e3d0bda6f205b56f30077b5a9b77e83208e94ae441315ac","released":true}
|
||||
-1
@@ -1 +0,0 @@
|
||||
{"pid":1,"start_time":"unknown","token":"a1342032-af80-4a49-a783-291bf73bf027","owner_host_id":"7af2fb0e8e37bd543e3d0bda6f205b56f30077b5a9b77e83208e94ae441315ac","released":true}
|
||||
Whitespace-only changes.
@@ -1,12 +0,0 @@
|
||||
{
|
||||
"version": 1,
|
||||
"sessionId": "5e2dd1e8-ea4b-4af1-b56b-89ab7cca4299",
|
||||
"pid": 487360,
|
||||
"host": "127.0.0.1",
|
||||
"port": 37541,
|
||||
"url": "ws://127.0.0.1:37541",
|
||||
"token": "LlIfu_aHH57jfys2Ii0ORMrnu56NLAa1",
|
||||
"startedAt": 1791842673967,
|
||||
"updatedAt": 1791842673967,
|
||||
"stale": false
|
||||
}
|
||||
@@ -1 +0,0 @@
|
||||
{"effectMarker":"f441857a-f591-474b-b34a-6d89b2bf9feb","incarnation":"linux:159099652","pid":487360}
|
||||
@@ -1 +0,0 @@
|
||||
{"effectMarker":"f441857a-f591-474b-b34a-6d89b2bf9feb","incarnation":"linux:159099652","pid":487360}
|
||||
+1
-1
@@ -221,7 +221,7 @@ rust/
|
||||
## Stats
|
||||
|
||||
- **~20K lines** of Rust
|
||||
- **11 crates** in workspace
|
||||
- **9 crates** in workspace
|
||||
- **Binary name:** `claw`
|
||||
- **Default model:** `claude-opus-4-7`
|
||||
- **Default permissions:** `workspace-write`
|
||||
|
||||
@@ -40,32 +40,17 @@ fn is_binary_file(path: &Path) -> io::Result<bool> {
|
||||
/// the workspace boundary (e.g. via `../` traversal or symlink).
|
||||
#[allow(dead_code)]
|
||||
fn validate_workspace_boundary(resolved: &Path, workspace_root: &Path) -> io::Result<()> {
|
||||
let workspace_root = normalize_for_comparison(workspace_root);
|
||||
let comparable = normalize_for_comparison(resolved);
|
||||
if comparable.starts_with(&workspace_root) || resolves_within(resolved, &workspace_root) {
|
||||
return Ok(());
|
||||
if !resolved.starts_with(workspace_root) {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::PermissionDenied,
|
||||
format!(
|
||||
"path {} escapes workspace boundary {}",
|
||||
resolved.display(),
|
||||
workspace_root.display()
|
||||
),
|
||||
));
|
||||
}
|
||||
Err(io::Error::new(
|
||||
io::ErrorKind::PermissionDenied,
|
||||
format!(
|
||||
"path {} escapes workspace boundary {}",
|
||||
comparable.display(),
|
||||
workspace_root.display()
|
||||
),
|
||||
))
|
||||
}
|
||||
|
||||
/// Whether `resolved` names a location inside `workspace_root` once the
|
||||
/// filesystem resolves it. Windows accepts several spellings of one location
|
||||
/// that [`Path::starts_with`] treats as different: letter case (a working
|
||||
/// directory typed as `c:\users\...` for `C:\Users\...`), the `\\.\` device
|
||||
/// namespace, and 8.3 short names. Only consulted after the lexical comparison
|
||||
/// fails, so it can accept such a spelling but never widens what already
|
||||
/// compared inside, and a path whose canonical form is outside stays rejected.
|
||||
fn resolves_within(resolved: &Path, workspace_root: &Path) -> bool {
|
||||
resolved
|
||||
.canonicalize()
|
||||
.is_ok_and(|canonical| normalize_for_comparison(&canonical).starts_with(workspace_root))
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Text payload returned by file-reading operations.
|
||||
@@ -542,29 +527,9 @@ fn build_grep_content_output(
|
||||
}
|
||||
|
||||
fn canonicalize_workspace_root(workspace_root: &Path) -> PathBuf {
|
||||
let canonical = workspace_root
|
||||
workspace_root
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| workspace_root.to_path_buf());
|
||||
normalize_for_comparison(&canonical)
|
||||
}
|
||||
|
||||
/// Normalize Windows' extended-length path prefix before comparing paths.
|
||||
///
|
||||
/// `std::fs::canonicalize` returns paths with a `\\?\` prefix on Windows,
|
||||
/// while a workspace root may still be represented as a regular path when it
|
||||
/// cannot be canonicalized (for example, while a workspace is bootstrapping).
|
||||
/// These paths refer to the same location but do not compare equal with
|
||||
/// [`Path::starts_with`]. Keep the comparison representation stable without
|
||||
/// changing the paths returned to callers.
|
||||
fn normalize_for_comparison(path: &Path) -> PathBuf {
|
||||
let raw = path.to_string_lossy();
|
||||
if let Some(unc_path) = raw.strip_prefix(r"\\?\UNC\") {
|
||||
return PathBuf::from(format!(r"\\{unc_path}"));
|
||||
}
|
||||
if let Some(dos_path) = raw.strip_prefix(r"\\?\") {
|
||||
return PathBuf::from(dos_path);
|
||||
}
|
||||
path.to_path_buf()
|
||||
.unwrap_or_else(|_| workspace_root.to_path_buf())
|
||||
}
|
||||
|
||||
fn should_skip_glob_dir(entry: &DirEntry) -> bool {
|
||||
@@ -812,9 +777,8 @@ mod tests {
|
||||
|
||||
use super::{
|
||||
component_contains_glob, derive_glob_walk_root, edit_file, expand_braces, glob_search,
|
||||
grep_search, is_symlink_escape, normalize_for_comparison, read_file,
|
||||
read_file_in_workspace, write_file, write_file_in_workspace, GrepSearchInput,
|
||||
MAX_WRITE_SIZE,
|
||||
grep_search, is_symlink_escape, read_file, read_file_in_workspace, write_file,
|
||||
write_file_in_workspace, GrepSearchInput, MAX_WRITE_SIZE,
|
||||
};
|
||||
|
||||
fn temp_path(name: &str) -> std::path::PathBuf {
|
||||
@@ -942,86 +906,6 @@ mod tests {
|
||||
let _ = std::fs::remove_dir_all(&outside);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalizes_windows_extended_length_prefixes_for_boundary_checks() {
|
||||
let dos_path = PathBuf::from(r"C:\workspace\file.txt");
|
||||
let extended_dos_path = PathBuf::from(r"\\?\C:\workspace\file.txt");
|
||||
assert_eq!(
|
||||
normalize_for_comparison(&extended_dos_path),
|
||||
dos_path,
|
||||
"extended DOS paths should compare like their regular form"
|
||||
);
|
||||
|
||||
let unc_path = PathBuf::from(r"\\server\share\workspace\file.txt");
|
||||
let extended_unc_path = PathBuf::from(r"\\?\UNC\server\share\workspace\file.txt");
|
||||
assert_eq!(
|
||||
normalize_for_comparison(&extended_unc_path),
|
||||
unc_path,
|
||||
"extended UNC paths should compare like their regular form"
|
||||
);
|
||||
}
|
||||
|
||||
// Backslash-separated paths only parse into path components on Windows.
|
||||
#[test]
|
||||
#[cfg(windows)]
|
||||
fn accepts_equivalent_extended_path_at_workspace_boundary() {
|
||||
let root = PathBuf::from(r"C:\workspace");
|
||||
let resolved = PathBuf::from(r"\\?\C:\workspace\src\main.rs");
|
||||
super::validate_workspace_boundary(&resolved, &root)
|
||||
.expect("equivalent Windows path representations should be accepted");
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[cfg(unix)]
|
||||
fn accepts_a_path_that_resolves_inside_the_workspace() {
|
||||
let workspace = temp_path("boundary-alias-workspace");
|
||||
let aliases = temp_path("boundary-alias-links");
|
||||
std::fs::create_dir_all(workspace.join("src")).expect("workspace dir should be created");
|
||||
std::fs::create_dir_all(&aliases).expect("alias dir should be created");
|
||||
let alias = aliases.join("src");
|
||||
std::os::unix::fs::symlink(workspace.join("src"), &alias).expect("symlink should create");
|
||||
let root = workspace
|
||||
.canonicalize()
|
||||
.expect("workspace should canonicalize");
|
||||
|
||||
super::validate_workspace_boundary(&alias, &root)
|
||||
.expect("a spelling that resolves inside the workspace should be accepted");
|
||||
super::validate_workspace_boundary(&aliases, &root)
|
||||
.expect_err("a path that resolves outside the workspace must stay rejected");
|
||||
|
||||
let _ = std::fs::remove_dir_all(&workspace);
|
||||
let _ = std::fs::remove_dir_all(&aliases);
|
||||
}
|
||||
|
||||
// Windows resolves one location from spellings that compare unequal as
|
||||
// paths, letter case and the `\\.\` device namespace among them.
|
||||
#[test]
|
||||
#[cfg(windows)]
|
||||
fn accepts_other_windows_spellings_of_a_workspace_path() {
|
||||
let workspace = temp_path("Boundary-Case-Workspace");
|
||||
std::fs::create_dir_all(workspace.join("src")).expect("workspace dir should be created");
|
||||
let root = workspace
|
||||
.canonicalize()
|
||||
.expect("workspace should canonicalize");
|
||||
|
||||
let lower = PathBuf::from(workspace.join("src").to_string_lossy().to_lowercase());
|
||||
super::validate_workspace_boundary(&lower, &root)
|
||||
.expect("a lower-cased spelling of a workspace path should be accepted");
|
||||
|
||||
let device = PathBuf::from(format!(r"\\.\{}", workspace.join("src").display()));
|
||||
super::validate_workspace_boundary(&device, &root)
|
||||
.expect("a device-namespace spelling of a workspace path should be accepted");
|
||||
|
||||
let parent = workspace
|
||||
.parent()
|
||||
.expect("workspace has a parent")
|
||||
.to_path_buf();
|
||||
super::validate_workspace_boundary(&parent, &root)
|
||||
.expect_err("the workspace's parent must stay rejected");
|
||||
|
||||
let _ = std::fs::remove_dir_all(&workspace);
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[cfg(unix)]
|
||||
fn workspace_write_rejects_parent_symlink_escape_regression_3007_class() {
|
||||
|
||||
@@ -81,10 +81,8 @@ impl PermissionEnforcer {
|
||||
|
||||
let active_mode = self.policy.active_mode();
|
||||
|
||||
// Check if active mode meets the dynamically determined required mode.
|
||||
// Ranked explicitly: `PermissionMode` no longer derives `Ord`, so the
|
||||
// ladder is a named function rather than declaration order.
|
||||
if active_mode.privilege_rank() >= required_mode.privilege_rank() {
|
||||
// Check if active mode meets the dynamically determined required mode
|
||||
if active_mode >= required_mode {
|
||||
return EnforcementResult::Allowed;
|
||||
}
|
||||
|
||||
|
||||
@@ -5,10 +5,7 @@ use serde_json::Value;
|
||||
use crate::config::RuntimePermissionRuleConfig;
|
||||
|
||||
/// Permission level assigned to a tool invocation or runtime session.
|
||||
///
|
||||
/// Deliberately **not** `PartialOrd`/`Ord`: see [`PermissionMode::privilege_rank`]
|
||||
/// for why ordering is a named function here rather than declaration order.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
|
||||
pub enum PermissionMode {
|
||||
ReadOnly,
|
||||
WorkspaceWrite,
|
||||
@@ -28,53 +25,6 @@ impl PermissionMode {
|
||||
Self::Allow => "allow",
|
||||
}
|
||||
}
|
||||
|
||||
/// Position on the privilege ladder used by [`PermissionPolicy`].
|
||||
///
|
||||
/// This is the only ranking, and `Ord` is intentionally not derived, so a
|
||||
/// stray `active_mode >= required_mode` no longer compiles. Ranking through
|
||||
/// declaration order meant a variant inserted in the middle of the enum
|
||||
/// silently re-ranked the ladder, and the difference between "this tool may
|
||||
/// run" and "this tool may run after a prompt" was therefore a property of
|
||||
/// where an `enum` line happened to sit. Every variant is named here, so an
|
||||
/// insertion is a build failure instead of a silent re-ranking; the two
|
||||
/// comparisons outside this module that do read a rank — the
|
||||
/// `PermissionEnforcer`'s `check_with_required_mode` and the `claw doctor`
|
||||
/// permission check — now say so at the call site.
|
||||
///
|
||||
/// `Prompt` and `Allow` are *modes of operation*, not privilege tiers. They
|
||||
/// rank above every tier so those two callers keep the answers they had, but
|
||||
/// they are never ranked *against* a tier: see [`Self::covers`].
|
||||
#[must_use]
|
||||
pub const fn privilege_rank(self) -> u8 {
|
||||
match self {
|
||||
Self::ReadOnly => 0,
|
||||
Self::WorkspaceWrite => 1,
|
||||
Self::DangerFullAccess => 2,
|
||||
Self::Prompt => 3,
|
||||
Self::Allow => 4,
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether a session running in `self` may use a tool that requires
|
||||
/// `required` without asking.
|
||||
///
|
||||
/// Only tiers answer this by rank. `Allow` never asks and `Prompt` always
|
||||
/// asks — it is the exact opposite of a silent allow — so neither is a
|
||||
/// statement about how much of the workspace the session may touch, and
|
||||
/// comparing either to `required` is comparing a mode to a tier. That
|
||||
/// comparison is what let `Prompt`, which outranks `DangerFullAccess`, make
|
||||
/// the ladder return `Allow` for every tool and skip the prompter entirely.
|
||||
#[must_use]
|
||||
pub const fn covers(self, required: Self) -> bool {
|
||||
match self {
|
||||
Self::Allow => true,
|
||||
Self::Prompt => false,
|
||||
Self::ReadOnly | Self::WorkspaceWrite | Self::DangerFullAccess => {
|
||||
self.privilege_rank() >= required.privilege_rank()
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Hook-provided override applied before standard permission evaluation.
|
||||
@@ -286,12 +236,29 @@ impl PermissionPolicy {
|
||||
prompter,
|
||||
);
|
||||
}
|
||||
// A hook `allow` is an override, not a bypass: the ask-rule check
|
||||
// and the privilege ladder below still run for it. Both used to be
|
||||
// spelled out a second time inside this arm, which is why the ladder
|
||||
// existed in two copies that had to be corrected in step — and the
|
||||
// second copy is exactly what a fix for one of them is easy to miss.
|
||||
Some(PermissionOverride::Allow) | None => {}
|
||||
Some(PermissionOverride::Allow) => {
|
||||
if let Some(rule) = ask_rule {
|
||||
let reason = format!(
|
||||
"tool '{tool_name}' requires approval due to ask rule '{}'",
|
||||
rule.raw
|
||||
);
|
||||
return Self::prompt_or_deny(
|
||||
tool_name,
|
||||
input,
|
||||
current_mode,
|
||||
required_mode,
|
||||
Some(reason),
|
||||
prompter,
|
||||
);
|
||||
}
|
||||
if allow_rule.is_some()
|
||||
|| current_mode == PermissionMode::Allow
|
||||
|| current_mode >= required_mode
|
||||
{
|
||||
return PermissionOutcome::Allow;
|
||||
}
|
||||
}
|
||||
None => {}
|
||||
}
|
||||
|
||||
if let Some(rule) = ask_rule {
|
||||
@@ -309,10 +276,10 @@ impl PermissionPolicy {
|
||||
);
|
||||
}
|
||||
|
||||
// The ladder, in one place. An `allow` rule is an explicit grant for
|
||||
// this tool and outranks the mode; otherwise the mode decides through
|
||||
// [`PermissionMode::covers`].
|
||||
if allow_rule.is_some() || current_mode.covers(required_mode) {
|
||||
if allow_rule.is_some()
|
||||
|| current_mode == PermissionMode::Allow
|
||||
|| current_mode >= required_mode
|
||||
{
|
||||
return PermissionOutcome::Allow;
|
||||
}
|
||||
|
||||
@@ -620,132 +587,6 @@ mod tests {
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_mode_routes_to_prompter_instead_of_auto_allowing() {
|
||||
// Regression: `Prompt` sorts above `DangerFullAccess` in the
|
||||
// `PermissionMode` `Ord` derivation, so the `current_mode >= required_mode`
|
||||
// ladder check used to auto-allow every tool in `Prompt` mode and never
|
||||
// invoke the prompter — defeating the entire purpose of the mode.
|
||||
let policy = PermissionPolicy::new(PermissionMode::Prompt)
|
||||
.with_tool_requirement("read_file", PermissionMode::ReadOnly);
|
||||
let mut prompter = RecordingPrompter {
|
||||
seen: Vec::new(),
|
||||
allow: true,
|
||||
};
|
||||
|
||||
let outcome = policy.authorize("read_file", "{}", Some(&mut prompter));
|
||||
|
||||
assert_eq!(outcome, PermissionOutcome::Allow);
|
||||
assert_eq!(prompter.seen.len(), 1);
|
||||
assert_eq!(prompter.seen[0].tool_name, "read_file");
|
||||
assert_eq!(prompter.seen[0].current_mode, PermissionMode::Prompt);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_mode_denies_when_no_prompter_is_available() {
|
||||
let policy = PermissionPolicy::new(PermissionMode::Prompt)
|
||||
.with_tool_requirement("read_file", PermissionMode::ReadOnly);
|
||||
|
||||
assert!(matches!(
|
||||
policy.authorize("read_file", "{}", None),
|
||||
PermissionOutcome::Deny { .. }
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_mode_prompts_for_every_privilege_tier() {
|
||||
// The ladder used to exist twice — once on the plain path and once inside
|
||||
// the `Some(PermissionOverride::Allow)` arm — so a hook emitting
|
||||
// `permissionDecision: "allow"` (see `hooks.rs`) had its own route to the
|
||||
// auto-allow. Sweeping the tiers against both routes keeps them honest now
|
||||
// that they are one predicate.
|
||||
for required in [
|
||||
PermissionMode::ReadOnly,
|
||||
PermissionMode::WorkspaceWrite,
|
||||
PermissionMode::DangerFullAccess,
|
||||
] {
|
||||
for override_decision in [None, Some(PermissionOverride::Allow)] {
|
||||
let policy = PermissionPolicy::new(PermissionMode::Prompt)
|
||||
.with_tool_requirement("read_file", required);
|
||||
let context = PermissionContext::new(
|
||||
override_decision,
|
||||
override_decision.map(|_| "hook approved".to_string()),
|
||||
);
|
||||
let mut prompter = RecordingPrompter {
|
||||
seen: Vec::new(),
|
||||
allow: true,
|
||||
};
|
||||
|
||||
let outcome =
|
||||
policy.authorize_with_context("read_file", "{}", &context, Some(&mut prompter));
|
||||
|
||||
assert_eq!(
|
||||
outcome,
|
||||
PermissionOutcome::Allow,
|
||||
"an approving prompter must still pass a {required:?} tool"
|
||||
);
|
||||
assert_eq!(
|
||||
prompter.seen.len(),
|
||||
1,
|
||||
"prompt mode skipped the prompter for a {required:?} tool \
|
||||
(override = {override_decision:?})"
|
||||
);
|
||||
assert_eq!(prompter.seen[0].tool_name, "read_file");
|
||||
assert_eq!(prompter.seen[0].current_mode, PermissionMode::Prompt);
|
||||
assert_eq!(prompter.seen[0].required_mode, required);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn privilege_rank_orders_the_privilege_tiers() {
|
||||
assert!(
|
||||
PermissionMode::ReadOnly.privilege_rank()
|
||||
< PermissionMode::WorkspaceWrite.privilege_rank()
|
||||
);
|
||||
assert!(
|
||||
PermissionMode::WorkspaceWrite.privilege_rank()
|
||||
< PermissionMode::DangerFullAccess.privilege_rank()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn privilege_rank_places_the_operating_modes_above_every_tier() {
|
||||
// `Prompt` and `Allow` are modes of operation rather than tiers, so
|
||||
// `covers` answers for them by name and never consults this number. They
|
||||
// still have to rank above every tier, because two callers outside this
|
||||
// module — the `PermissionEnforcer`'s `check_with_required_mode` and the
|
||||
// `claw doctor` permission check — compare ranks, and a `Prompt`/`Allow`
|
||||
// session must keep the result it had under the old derived `Ord`.
|
||||
for mode in [PermissionMode::Prompt, PermissionMode::Allow] {
|
||||
assert!(
|
||||
mode.privilege_rank() > PermissionMode::DangerFullAccess.privilege_rank(),
|
||||
"{mode:?} must outrank danger-full-access"
|
||||
);
|
||||
assert!(
|
||||
mode.privilege_rank() > PermissionMode::WorkspaceWrite.privilege_rank(),
|
||||
"{mode:?} must outrank workspace-write"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn covers_never_compares_an_operating_mode_against_a_tier() {
|
||||
for required in [
|
||||
PermissionMode::ReadOnly,
|
||||
PermissionMode::WorkspaceWrite,
|
||||
PermissionMode::DangerFullAccess,
|
||||
] {
|
||||
assert!(
|
||||
!PermissionMode::Prompt.covers(required),
|
||||
"prompt mode must ask before running a {required:?} tool"
|
||||
);
|
||||
}
|
||||
assert!(PermissionMode::Allow.covers(PermissionMode::DangerFullAccess));
|
||||
assert!(PermissionMode::DangerFullAccess.covers(PermissionMode::WorkspaceWrite));
|
||||
assert!(!PermissionMode::WorkspaceWrite.covers(PermissionMode::DangerFullAccess));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn applies_rule_based_denials_and_allows() {
|
||||
let rules = RuntimePermissionRuleConfig::new(
|
||||
|
||||
@@ -4222,18 +4222,15 @@ fn check_permission_health(permission_mode: PermissionModeProvenance) -> Diagnos
|
||||
|| source.to_string(),
|
||||
|env_var| format!("{source}:{env_var}"),
|
||||
);
|
||||
// Ranks, not declaration order: `PermissionMode` deliberately does not
|
||||
// derive `Ord`, so the ladder is named at every comparison site.
|
||||
let specs = mvp_tool_specs();
|
||||
let active_rank = permission_mode.mode.privilege_rank();
|
||||
let tools_satisfied = specs
|
||||
.iter()
|
||||
.filter(|spec| active_rank >= spec.required_permission.privilege_rank())
|
||||
.filter(|spec| permission_mode.mode >= spec.required_permission)
|
||||
.map(|spec| spec.name)
|
||||
.collect::<Vec<_>>();
|
||||
let tools_gated = specs
|
||||
.iter()
|
||||
.filter(|spec| active_rank < spec.required_permission.privilege_rank())
|
||||
.filter(|spec| permission_mode.mode < spec.required_permission)
|
||||
.map(|spec| spec.name)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
@@ -7766,20 +7763,15 @@ impl LiveCli {
|
||||
match result {
|
||||
Ok(summary) => {
|
||||
self.replace_runtime(runtime)?;
|
||||
// The assistant response was already streamed to the terminal live
|
||||
// during the turn (emit_output = true), so reprinting the full text
|
||||
// here would duplicate the entire message. When text was streamed,
|
||||
// emit a newline first so `spinner.finish`'s line-clear lands on a
|
||||
// fresh line instead of erasing the last streamed line.
|
||||
let final_text = final_assistant_text(&summary);
|
||||
if !final_text.is_empty() {
|
||||
println!();
|
||||
}
|
||||
spinner.finish(
|
||||
"✨ Done",
|
||||
TerminalRenderer::new().color_theme(),
|
||||
&mut stdout,
|
||||
)?;
|
||||
let final_text = final_assistant_text(&summary);
|
||||
if !final_text.is_empty() {
|
||||
println!("{final_text}");
|
||||
}
|
||||
println!();
|
||||
if let Some(event) = summary.auto_compaction {
|
||||
println!(
|
||||
@@ -7911,9 +7903,6 @@ impl LiveCli {
|
||||
match new_runtime.run_turn(input, Some(&mut rp)) {
|
||||
Ok(summary) => {
|
||||
self.replace_runtime(new_runtime)?;
|
||||
if !final_assistant_text(&summary).is_empty() {
|
||||
println!();
|
||||
}
|
||||
spinner.finish(
|
||||
if round == 0 {
|
||||
"✨ Done (after auto-compact)"
|
||||
|
||||
@@ -249,16 +249,6 @@ impl TerminalRenderer {
|
||||
|
||||
#[must_use]
|
||||
pub fn render_markdown(&self, markdown: &str) -> String {
|
||||
self.render_markdown_raw(markdown).trim_end().to_string()
|
||||
}
|
||||
|
||||
/// Render markdown to ANSI without trimming trailing whitespace.
|
||||
///
|
||||
/// Block-level events (paragraphs, headings, lists, code blocks) emit their
|
||||
/// trailing newline separators here. `render_markdown` trims them for
|
||||
/// one-shot output, while streaming keeps them so that successive chunks
|
||||
/// stay on their own lines instead of running together.
|
||||
fn render_markdown_raw(&self, markdown: &str) -> String {
|
||||
let normalized = normalize_nested_fences(markdown);
|
||||
let mut output = String::new();
|
||||
let mut state = RenderState::default();
|
||||
@@ -277,7 +267,7 @@ impl TerminalRenderer {
|
||||
);
|
||||
}
|
||||
|
||||
output
|
||||
output.trim_end().to_string()
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
@@ -285,18 +275,6 @@ impl TerminalRenderer {
|
||||
self.render_markdown(markdown)
|
||||
}
|
||||
|
||||
/// Stream-friendly variant of `markdown_to_ansi` that preserves trailing
|
||||
/// block separators (such as a paragraph's closing newlines).
|
||||
///
|
||||
/// `MarkdownStreamState::push` writes each rendered chunk directly to the
|
||||
/// terminal with no separator of its own, so the trailing newlines must
|
||||
/// survive here to keep a paragraph from sticking to the block that follows
|
||||
/// it. The final `flush` still uses `markdown_to_ansi` to trim the tail.
|
||||
#[must_use]
|
||||
pub fn markdown_to_ansi_stream(&self, markdown: &str) -> String {
|
||||
self.render_markdown_raw(markdown)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_lines)]
|
||||
fn render_event(
|
||||
&self,
|
||||
@@ -631,7 +609,7 @@ impl MarkdownStreamState {
|
||||
let split = find_stream_safe_boundary(&self.pending)?;
|
||||
let ready = self.pending[..split].to_string();
|
||||
self.pending.drain(..split);
|
||||
Some(renderer.markdown_to_ansi_stream(&ready))
|
||||
Some(renderer.markdown_to_ansi(&ready))
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
@@ -1074,34 +1052,6 @@ mod tests {
|
||||
assert!(plain_text.contains("fn nested()"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stream_rendering_preserves_block_separators() {
|
||||
let renderer = TerminalRenderer::new();
|
||||
|
||||
// The stream variant keeps the paragraph-ending newlines that the
|
||||
// one-shot variant trims, so consecutive streamed chunks do not run
|
||||
// into each other when written back-to-back to the terminal.
|
||||
let streamed = renderer.markdown_to_ansi_stream("Intro paragraph.\n\n");
|
||||
assert!(streamed.ends_with('\n'));
|
||||
assert!(!renderer
|
||||
.markdown_to_ansi("Intro paragraph.\n\n")
|
||||
.ends_with('\n'));
|
||||
|
||||
let mut state = MarkdownStreamState::default();
|
||||
let first = state
|
||||
.push(&renderer, "Intro paragraph.\n\n")
|
||||
.expect("blank line closes the paragraph block");
|
||||
let second = state
|
||||
.push(&renderer, "1. first item\n\n")
|
||||
.expect("blank line closes the list block");
|
||||
|
||||
let combined = strip_ansi(&format!("{first}{second}"));
|
||||
assert!(combined.contains("Intro paragraph."));
|
||||
assert!(combined.contains("1. first item"));
|
||||
// The two blocks stay on separate lines instead of sticking together.
|
||||
assert!(!combined.contains("Intro paragraph.1."));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spinner_advances_frames() {
|
||||
let terminal_renderer = TerminalRenderer::new();
|
||||
|
||||
@@ -130,7 +130,7 @@ fn compact_flag_streaming_text_only_emits_final_message_text() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn text_prompt_mode_streams_assistant_text_once_before_spinner_completion() {
|
||||
fn text_prompt_mode_prints_final_assistant_text_after_spinner() {
|
||||
// given a workspace pointed at the mock Anthropic service running the
|
||||
// streaming_text scenario which only emits a single assistant text block
|
||||
let runtime = tokio::runtime::Runtime::new().expect("tokio runtime should build");
|
||||
@@ -162,7 +162,7 @@ fn text_prompt_mode_streams_assistant_text_once_before_spinner_completion() {
|
||||
],
|
||||
);
|
||||
|
||||
// then the streamed answer should appear once before the spinner completion
|
||||
// then stdout should contain the final assistant text, not just spinner output
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"text prompt run should succeed\nstdout:\n{}\n\nstderr:\n{}",
|
||||
@@ -171,19 +171,19 @@ fn text_prompt_mode_streams_assistant_text_once_before_spinner_completion() {
|
||||
);
|
||||
let stdout = String::from_utf8(output.stdout).expect("stdout should be utf8");
|
||||
let plain_stdout = strip_ansi_codes(&stdout);
|
||||
let assistant_text = "Mock streaming says hello from the parity harness.";
|
||||
assert_eq!(
|
||||
plain_stdout.matches(assistant_text).count(),
|
||||
1,
|
||||
"text prompt stdout should contain the streamed assistant text exactly once ({stdout:?})"
|
||||
assert!(
|
||||
plain_stdout.contains("Mock streaming says hello from the parity harness."),
|
||||
"text prompt stdout should include the assistant text ({stdout:?})"
|
||||
);
|
||||
assert!(
|
||||
plain_stdout.contains("✔ ✨ Done"),
|
||||
"text prompt stdout should still include spinner completion ({stdout:?})"
|
||||
);
|
||||
assert!(
|
||||
plain_stdout.find(assistant_text) < plain_stdout.find("✔ ✨ Done"),
|
||||
"text prompt stdout should stream the assistant text before spinner completion ({stdout:?})"
|
||||
plain_stdout
|
||||
.lines()
|
||||
.any(|line| line == "Mock streaming says hello from the parity harness."),
|
||||
"text prompt stdout should print the assistant text as its own line ({stdout:?})"
|
||||
);
|
||||
|
||||
fs::remove_dir_all(&workspace).expect("workspace cleanup should succeed");
|
||||
|
||||
+1
-17
@@ -1,7 +1,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import re
|
||||
from dataclasses import asdict, dataclass
|
||||
from pathlib import Path
|
||||
|
||||
@@ -16,31 +15,16 @@ class StoredSession:
|
||||
|
||||
DEFAULT_SESSION_DIR = Path('.port_sessions')
|
||||
|
||||
# Session ids become filenames inside the session directory, so they must not be
|
||||
# able to escape it via path separators or traversal. Restrict to the charset
|
||||
# produced by legitimate ids (uuid4 hex, `session-<digits>-<n>`).
|
||||
_SESSION_ID_PATTERN = re.compile(r'\A[A-Za-z0-9_-]+\Z')
|
||||
|
||||
|
||||
def _validate_session_id(session_id: str) -> str:
|
||||
if not _SESSION_ID_PATTERN.match(session_id):
|
||||
raise ValueError(
|
||||
f'invalid session_id {session_id!r}: only [A-Za-z0-9_-] characters are allowed'
|
||||
)
|
||||
return session_id
|
||||
|
||||
|
||||
def save_session(session: StoredSession, directory: Path | None = None) -> Path:
|
||||
session_id = _validate_session_id(session.session_id)
|
||||
target_dir = directory or DEFAULT_SESSION_DIR
|
||||
target_dir.mkdir(parents=True, exist_ok=True)
|
||||
path = target_dir / f'{session_id}.json'
|
||||
path = target_dir / f'{session.session_id}.json'
|
||||
path.write_text(json.dumps(asdict(session), indent=2))
|
||||
return path
|
||||
|
||||
|
||||
def load_session(session_id: str, directory: Path | None = None) -> StoredSession:
|
||||
session_id = _validate_session_id(session_id)
|
||||
target_dir = directory or DEFAULT_SESSION_DIR
|
||||
data = json.loads((target_dir / f'{session_id}.json').read_text())
|
||||
return StoredSession(
|
||||
|
||||
@@ -9,66 +9,9 @@ from src.models import PermissionDenial
|
||||
from src.path_scope import WorkspacePathScope, extract_path_candidates
|
||||
from src.permissions import ToolPermissionContext
|
||||
from src.query_engine import QueryEnginePort
|
||||
from src.session_store import StoredSession, load_session, save_session
|
||||
from src.tools import execute_tool
|
||||
|
||||
|
||||
class SessionStorePathTraversalTests(unittest.TestCase):
|
||||
def test_traversal_session_id_is_rejected_on_load(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
store = Path(tmp) / 'store'
|
||||
store.mkdir()
|
||||
# A secret living outside the session store that traversal would reach.
|
||||
(Path(tmp) / 'secret.json').write_text('{}')
|
||||
|
||||
with self.assertRaises(ValueError):
|
||||
load_session('../secret', directory=store)
|
||||
|
||||
def test_absolute_and_separator_session_ids_are_rejected(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
store = Path(tmp) / 'store'
|
||||
store.mkdir()
|
||||
for bad_id in (
|
||||
'/etc/passwd',
|
||||
'nested/child',
|
||||
r'nested\child',
|
||||
r'C:\Windows\win.ini',
|
||||
'..',
|
||||
'a/../../b',
|
||||
):
|
||||
with self.assertRaises(ValueError):
|
||||
load_session(bad_id, directory=store)
|
||||
|
||||
def test_traversal_session_id_is_rejected_on_save(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
store = Path(tmp) / 'store'
|
||||
store.mkdir()
|
||||
session = StoredSession(
|
||||
session_id='../escape',
|
||||
messages=('hello',),
|
||||
input_tokens=1,
|
||||
output_tokens=2,
|
||||
)
|
||||
with self.assertRaises(ValueError):
|
||||
save_session(session, directory=store)
|
||||
self.assertFalse((Path(tmp) / 'escape.json').exists())
|
||||
|
||||
def test_legitimate_session_id_round_trips(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
store = Path(tmp) / 'store'
|
||||
session = StoredSession(
|
||||
session_id='session-1775386832313-0',
|
||||
messages=('hi', 'there'),
|
||||
input_tokens=3,
|
||||
output_tokens=4,
|
||||
)
|
||||
path = save_session(session, directory=store)
|
||||
|
||||
self.assertEqual(store, path.parent)
|
||||
loaded = load_session('session-1775386832313-0', directory=store)
|
||||
self.assertEqual(session, loaded)
|
||||
|
||||
|
||||
class WorkspacePathScopeTests(unittest.TestCase):
|
||||
def test_direct_parent_escape_is_denied(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
|
||||
Reference in new issue
Block a user