@sigitcodereview / sigit / commits / adec36b

fix(acp): decode resource_link file URIs before reading them (#170)

* fix(acp): decode resource_link file URIs before reading them A resource_link was read by stripping file:// and using what was left as the path, so percent escapes stayed literal (a space arrived as %20), non-ASCII names failed, and a Windows URI kept its leading slash. A # in a file name was also taken for the line-range fragment. Parse the URI with the url crate and take the path from to_file_path(), and the line range from the parsed fragment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use path components for cross-platform file link test assertions Replace direct PathBuf equality with path component comparison so the tests pass on Windows, where to_file_path() converts forward slashes to backslashes. Co-Authored-By: siGit Code v1.6.1-acp <noreply@sigit.si> * fix: split file-link tests by platform for Windows drive-letter paths url::Url::to_file_path() on Windows requires a drive letter in the file URI (e.g. file:///C:/Users/...). The tests used Unix-style paths (file:///Users/..., file:///tmp/...) which cause to_file_path() to return Err on Windows, making parse_file_link return None and the .unwrap() panic. Split each test into #[cfg(unix)] and #[cfg(windows)] variants with platform-appropriate URIs and expected paths. Co-Authored-By: siGit Code v1.6.1-acp <noreply@sigit.si> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: siGit Code v1.6.1-acp <noreply@sigit.si>

Seto Elkahfi committed Oct 6, 2026 at 09:20 UTC adec36bc59029d30ae986c4053e43c5a47ab2393

3 files changed +105 -21

Cargo.lock

@@ -5801,6 +5801,7 @@ dependencies = [

5801 "tokio-util",

5802 "toml 0.8.23",

5803 "tracing-subscriber",

5804 + "url",

5805 "uuid 1.26.0",

5806 ]

5807

Cargo.toml

@@ -60,6 +60,11 @@ base64 = "0.22"

60 rand = "0.9"

61 sha2 = "0.10"

63 +# ACP `resource_link` blocks carry file URIs. Parsing them with `url` decodes

64 +# percent escapes and maps Windows drive paths, which hand-stripping `file://`

65 +# did not. Already in the graph through reqwest.

66 +url = "2"

67 +

68 # Release binaries ship to five registries, and the NuGet package bundles all

69 # six targets in a single archive, so binary size is a distribution constraint

70 # rather than a nicety. Stripping symbols takes the macOS arm64 binary from

src/main.rs

+99 -21

@@ -2402,24 +2402,8 @@ impl SiGitAgent {

2402 // reference without content; read the file ourselves

2403 let label = link.name.clone();

2404

2405 - if let Some(raw_path) = link.uri.strip_prefix("file://") {

2406 - let (file_path, line_range) = if let Some(hash_pos) = raw_path.rfind('#') {

2407 - let fragment = &raw_path[hash_pos + 1..];

2408 - let path = &raw_path[..hash_pos];

2409 - // Parse "L207:219" or "L207-219" → (207, 219)

2410 - let range = fragment.strip_prefix('L').and_then(|rest| {

2411 - let sep = if rest.contains(':') { ':' } else { '-' };

2412 - let mut parts = rest.splitn(2, sep);

2413 - let start = parts.next()?.parse::<usize>().ok()?;

2414 - let end = parts.next()?.parse::<usize>().ok()?;

2415 - Some((start, end))

2416 - });

2417 - (path, range)

2418 - } else {

2419 - (raw_path, None)

2420 - };

2421 -

2422 - match std::fs::read_to_string(file_path) {

2405 + if let Some((file_path, line_range)) = parse_file_link(&link.uri) {

2406 + match std::fs::read_to_string(&file_path) {

2407 Ok(contents) => {

2408 let extracted = if let Some((start, end)) = line_range {

2409 let selected: Vec<&str> = contents

@@ -2432,19 +2416,24 @@ impl SiGitAgent {

2416 .map(|(_, line)| line)

2417 .collect();

2418 format!(

2435 - "\n--- {label} ({file_path} lines {start}-{end}) ---\n{}\n--- end {label} ---",

2419 + "\n--- {label} ({} lines {start}-{end}) ---\n{}\n--- end {label} ---",

2420 + file_path.display(),

2421 selected.join("\n")

2422 )

2423 } else {

2424 format!(

2440 - "\n--- {label} ({file_path}) ---\n{contents}\n--- end {label} ---"

2425 + "\n--- {label} ({}) ---\n{contents}\n--- end {label} ---",

2426 + file_path.display()

2427 )

2428 };

2429 parts.push(extracted);

2430 }

2431 Err(err) => {

2432 log::warn!("could not read ResourceLink {}: {err}", link.uri);

2447 - parts.push(format!("[referenced file: {label} ({file_path})]"));

2433 + parts.push(format!(

2434 + "[referenced file: {label} ({})]",

2435 + file_path.display()

2436 + ));

2437 }

2438 }

2439 } else {

@@ -4016,6 +4005,31 @@ fn parse_slash(input: &str) -> Option<SlashCommand> {

4005 })

4006 }

4007

4008 +/// Resolves an ACP `resource_link` URI to a local path and an optional line

4009 +/// range. Parsing with `url` decodes percent escapes (`%20`, non-ASCII names),

4010 +/// maps Windows drive URIs, and reads `#` only as the fragment separator, so a

4011 +/// `#` inside a file name stays part of the path. `None` for anything that is

4012 +/// not a local `file:` URI.

4013 +fn parse_file_link(uri: &str) -> Option<(PathBuf, Option<(usize, usize)>)> {

4014 + let url = url::Url::parse(uri).ok()?;

4015 + if url.scheme() != "file" {

4016 + return None;

4017 + }

4018 + let path = url.to_file_path().ok()?;

4019 + let range = url.fragment().and_then(parse_line_range);

4020 + Some((path, range))

4021 +}

4022 +

4023 +/// Parses a `L207:219` or `L207-219` fragment into `(207, 219)`.

4024 +fn parse_line_range(fragment: &str) -> Option<(usize, usize)> {

4025 + let rest = fragment.strip_prefix('L')?;

4026 + let sep = if rest.contains(':') { ':' } else { '-' };

4027 + let mut parts = rest.splitn(2, sep);

4028 + let start = parts.next()?.parse::<usize>().ok()?;

4029 + let end = parts.next()?.parse::<usize>().ok()?;

4030 + Some((start, end))

4031 +}

4032 +

4033 /// ACP clients may prepend context as separate text blocks before the user's

4034 /// input. Search from the end so a standalone slash command in the final user

4035 /// block is still dispatched locally instead of being buried in joined context.

@@ -5186,6 +5200,70 @@ async fn main() -> anyhow::Result<()> {

5200 mod tests {

5201 use super::*;

5202

5203 + /// Compares path components so the tests pass on both Unix (forward slashes)

5204 + /// and Windows (`to_file_path` yields backslashes). On Windows the expected

5205 + /// string uses forward slashes too; `Path::components` normalizes both.

5206 + fn assert_path_components(actual: &std::path::Path, expected: &str) {

5207 + let expected: Vec<_> = std::path::Path::new(expected).components().collect();

5208 + let actual: Vec<_> = actual.components().collect();

5209 + assert_eq!(actual, expected);

5210 + }

5211 +

5212 + /// On Windows `to_file_path` requires a drive letter, so the test URIs use

5213 + /// `C:` and the expected paths mirror that. On Unix the URIs use plain

5214 + /// `/tmp`/`/Users` roots.

5215 + #[cfg(unix)]

5216 + #[test]

5217 + fn file_links_decode_percent_escapes_and_non_ascii_names() {

5218 + let (path, range) = parse_file_link("file:///Users/me/My%20Project/a.rs").unwrap();

5219 + assert_path_components(&path, "/Users/me/My Project/a.rs");

5220 + assert_eq!(range, None);

5221 +

5222 + let (path, _) = parse_file_link("file:///Users/me/%C3%A9t%C3%A9/notes.md").unwrap();

5223 + assert_path_components(&path, "/Users/me/été/notes.md");

5224 + }

5225 +

5226 + #[cfg(windows)]

5227 + #[test]

5228 + fn file_links_decode_percent_escapes_and_non_ascii_names() {

5229 + let (path, range) = parse_file_link("file:///C:/Users/me/My%20Project/a.rs").unwrap();

5230 + assert_path_components(&path, "C:/Users/me/My Project/a.rs");

5231 + assert_eq!(range, None);

5232 +

5233 + let (path, _) = parse_file_link("file:///C:/Users/me/%C3%A9t%C3%A9/notes.md").unwrap();

5234 + assert_path_components(&path, "C:/Users/me/été/notes.md");

5235 + }

5236 +

5237 + #[cfg(unix)]

5238 + #[test]

5239 + fn file_links_keep_a_hash_in_the_file_name_and_read_the_line_fragment() {

5240 + let (path, range) = parse_file_link("file:///tmp/c%23/a%23b.rs#L207:219").unwrap();

5241 + assert_path_components(&path, "/tmp/c#/a#b.rs");

5242 + assert_eq!(range, Some((207, 219)));

5243 +

5244 + let (path, range) = parse_file_link("file:///tmp/a.rs#L3-5").unwrap();

5245 + assert_path_components(&path, "/tmp/a.rs");

5246 + assert_eq!(range, Some((3, 5)));

5247 + }

5248 +

5249 + #[cfg(windows)]

5250 + #[test]

5251 + fn file_links_keep_a_hash_in_the_file_name_and_read_the_line_fragment() {

5252 + let (path, range) = parse_file_link("file:///C:/tmp/c%23/a%23b.rs#L207:219").unwrap();

5253 + assert_path_components(&path, "C:/tmp/c#/a#b.rs");

5254 + assert_eq!(range, Some((207, 219)));

5255 +

5256 + let (path, range) = parse_file_link("file:///C:/tmp/a.rs#L3-5").unwrap();

5257 + assert_path_components(&path, "C:/tmp/a.rs");

5258 + assert_eq!(range, Some((3, 5)));

5259 + }

5260 +

5261 + #[test]

5262 + fn file_links_reject_non_file_uris() {

5263 + assert!(parse_file_link("https://example.com/a.rs").is_none());

5264 + assert!(parse_file_link("not a uri").is_none());

5265 + }

5266 +

5267 #[test]

5268 fn stop_reasons_follow_the_round_cap_then_the_finish_reason() {

5269 use backend::FinishReason;