Skip to content

Commit 193c6a7

Browse files
jamesadevineCopilot
andcommitted
fix(compile): guard runtime-import marker paths against whitespace and ##vso injection
Addresses three findings from the 2026-05-19 Rust PR review on #625: 1. Reject whitespace in agent source paths at compile time when inlined-imports is false. The runtime resolver (scripts/ado-script/src/import/index.ts) matches marker bodies with [^\s}]+, so a space silently truncated the path and produced either a misleading runtime `file not found` or, for optional markers, an unexpanded marker visible to the LLM. Mirrors the existing resolve_imports_inline guard. 2. Aggregate all import errors instead of reporting only the first. Previously hadError ??= captured one failure and silently swallowed subsequent ones. Now every missing/unreadable required marker emits its own ##vso[task.logissue type=error] line before exit(1). 3. Sanitize rawPath in ##vso error output. Strips `]`, CR, and LF from path strings embedded in diagnostic lines so an unusual path can no longer break the ##vso command framing. Tests: * tests/compiler_tests.rs::test_runtime_imports_default_rejects_source_path_with_whitespace * scripts/ado-script/src/import/__tests__/error-reporting.test.ts (2 cases) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 003b6f3 commit 193c6a7

4 files changed

Lines changed: 156 additions & 9 deletions

File tree

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import { describe, expect, it } from "vitest";
2+
import { readText, runImportSource, withScratchDir, writeFixture } from "./helpers.js";
3+
4+
describe("runtime-import error reporting", () => {
5+
it("reports every missing required marker, not just the first", () => {
6+
withScratchDir("multiple-missing", (dir) => {
7+
const original = [
8+
"head",
9+
"{{#runtime-import ./missing-one.md}}",
10+
"{{#runtime-import ./missing-two.md}}",
11+
"tail",
12+
"",
13+
].join("\n");
14+
const target = writeFixture(dir, "prompt.md", original);
15+
16+
const result = runImportSource(target);
17+
18+
expect(result.status).toBe(1);
19+
expect(result.stderr).toBe("");
20+
expect(result.stdout).toBe(
21+
[
22+
"##vso[task.logissue type=error]runtime-import: file not found: ./missing-one.md",
23+
"##vso[task.logissue type=error]runtime-import: file not found: ./missing-two.md",
24+
"",
25+
].join("\n"),
26+
);
27+
// Target file must NOT be overwritten on error.
28+
expect(readText(target)).toBe(original);
29+
});
30+
});
31+
32+
it("strips characters that would break the ##vso command framing", () => {
33+
withScratchDir("vso-injection", (dir) => {
34+
// The path contains `]` which would close the ##vso bracket
35+
// prematurely, and the marker is the only one in the file so the
36+
// diagnostic line is deterministic.
37+
const target = writeFixture(
38+
dir,
39+
"prompt.md",
40+
"{{#runtime-import ./bad]type=warning]injected.md}}\n",
41+
);
42+
43+
const result = runImportSource(target);
44+
45+
expect(result.status).toBe(1);
46+
expect(result.stderr).toBe("");
47+
// `]` characters are stripped from the path in the error message.
48+
expect(result.stdout).toBe(
49+
"##vso[task.logissue type=error]runtime-import: file not found: ./badtype=warninginjected.md\n",
50+
);
51+
});
52+
});
53+
});

scripts/ado-script/src/import/index.ts

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,37 @@ import { dirname, isAbsolute, resolve } from "node:path";
33

44
const MARKER = /\{\{#runtime-import(\?)?\s+([^\s}]+)\s*\}\}/g;
55

6-
function fail(msg: string): never {
7-
process.stdout.write(`##vso[task.logissue type=error]runtime-import: ${msg}\n`);
6+
// Strip characters that would let an attacker-controlled `rawPath` break
7+
// out of the `##vso[task.logissue type=error]…` framing:
8+
// * `]` — closes the VSO command bracket prematurely.
9+
// * `\r`, `\n` — split the diagnostic line so subsequent text would be
10+
// parsed as a new ADO logging command.
11+
// Marker paths normally come from a compile-time-generated location, but
12+
// `import.js` is also invoked against arbitrary author-written markers in
13+
// the agent body, so this is a defence-in-depth guard.
14+
function sanitizeForVsoMessage(value: string): string {
15+
return value.replace(/[\]\r\n]/g, "");
16+
}
17+
18+
function fail(messages: string[]): never {
19+
for (const msg of messages) {
20+
process.stdout.write(`##vso[task.logissue type=error]runtime-import: ${msg}\n`);
21+
}
822
process.exit(1);
923
}
1024

1125
function main(): void {
1226
const target = process.argv[2];
1327
if (!target) {
14-
fail("missing target file argument");
28+
fail(["missing target file argument"]);
1529
}
1630
if (!existsSync(target)) {
17-
fail(`target file not found: ${target}`);
31+
fail([`target file not found: ${sanitizeForVsoMessage(target)}`]);
1832
}
1933

2034
const base = process.env.ADO_AW_IMPORT_BASE ?? dirname(target);
2135
const original = readFileSync(target, "utf8");
22-
let hadError: string | null = null;
36+
const errors: string[] = [];
2337

2438
// Single-pass by design: imported snippets are inserted verbatim and any
2539
// nested runtime-import markers inside them are not expanded. This matches
@@ -31,20 +45,22 @@ function main(): void {
3145
if (optional === "?") {
3246
return "";
3347
}
34-
hadError ??= `file not found: ${rawPath}`;
48+
errors.push(`file not found: ${sanitizeForVsoMessage(rawPath)}`);
3549
return "";
3650
}
3751

3852
try {
3953
return readFileSync(absPath, "utf8");
4054
} catch (error) {
41-
hadError ??= `failed to read ${rawPath}: ${(error as Error).message}`;
55+
errors.push(
56+
`failed to read ${sanitizeForVsoMessage(rawPath)}: ${sanitizeForVsoMessage((error as Error).message)}`,
57+
);
4258
return "";
4359
}
4460
});
4561

46-
if (hadError) {
47-
fail(hadError);
62+
if (errors.length > 0) {
63+
fail(errors);
4864
}
4965
writeFileSync(target, expanded, "utf8");
5066
}

src/compile/common.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3218,6 +3218,21 @@ pub async fn compile_shared(
32183218
} else {
32193219
let agent_marker_path =
32203220
source_path.replace("{{ trigger_repo_directory }}", &trigger_repo_directory);
3221+
// The runtime resolver (`scripts/ado-script/src/import/index.ts`)
3222+
// matches marker bodies with `[^\s}]+`, which truncates at the
3223+
// first whitespace character. If the agent's source path contains
3224+
// a space (e.g. `my agents/pipeline.md`), the resolver would
3225+
// silently parse a truncated path, fail the existence check, and
3226+
// surface a misleading error — or, worse, leave the marker
3227+
// unexpanded if optional. Reject at compile time so the failure
3228+
// mode is a clear compile error, not a confusing runtime one.
3229+
// This mirrors the same guard in `resolve_imports_inline` for the
3230+
// inlined-imports path.
3231+
anyhow::ensure!(
3232+
!agent_marker_path.chars().any(char::is_whitespace),
3233+
"runtime-import: agent source path '{}' contains whitespace, which is not supported by the runtime resolver (rename the path to remove spaces, or set `inlined-imports: true`)",
3234+
agent_marker_path
3235+
);
32213236
format!("{{{{#runtime-import {}}}}}", agent_marker_path)
32223237
};
32233238

tests/compiler_tests.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3829,6 +3829,69 @@ fn test_stage_inlined_imports_true_resolves_author_markers() {
38293829
assert_runtime_imports_author_marker_output("runtime_imports_author_marker_stage.md");
38303830
}
38313831

3832+
/// Compile a default-mode (inlined-imports: false) agent whose source path
3833+
/// contains a space. The runtime resolver matches marker bodies with
3834+
/// `[^\s}]+`, so a space would silently truncate the marker at runtime and
3835+
/// surface a confusing "file not found" error (or, for optional markers,
3836+
/// leave the marker unexpanded). Reject at compile time so the failure is
3837+
/// a clear, actionable compile error rather than a runtime data-integrity
3838+
/// bug.
3839+
#[test]
3840+
fn test_runtime_imports_default_rejects_source_path_with_whitespace() {
3841+
use std::sync::atomic::{AtomicU64, Ordering};
3842+
static COUNTER: AtomicU64 = AtomicU64::new(0);
3843+
let unique_id = COUNTER.fetch_add(1, Ordering::Relaxed);
3844+
3845+
// Use a top-level temp dir (NOT under the repo) so the compiler can't
3846+
// discover a git root and rebase the path on it.
3847+
let temp_dir = std::env::temp_dir().join(format!(
3848+
"agentic-pipeline-spaced-path-{}-{}",
3849+
std::process::id(),
3850+
unique_id,
3851+
));
3852+
let spaced_dir = temp_dir.join("my agents");
3853+
fs::create_dir_all(&spaced_dir).expect("Failed to create spaced temp dir");
3854+
// generate_source_path falls back to the filename only when it can't
3855+
// locate a git root above the input path — which would hide the space
3856+
// from the marker. Create an empty `.git` marker so the spaced dir is
3857+
// resolved relative to a discoverable repo root and the space ends up
3858+
// in the runtime-import marker (i.e. exercises the new guard).
3859+
fs::create_dir_all(temp_dir.join(".git")).expect("Failed to create .git marker");
3860+
3861+
let input = "---\nname: \"Spaced Path Agent\"\ndescription: \"Agent whose source path contains a space\"\n---\n\n## Body\n\nhello\n";
3862+
let input_path = spaced_dir.join("pipeline.md");
3863+
let output_path = spaced_dir.join("pipeline.yml");
3864+
fs::write(&input_path, input).unwrap();
3865+
3866+
let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw"));
3867+
let output = std::process::Command::new(&binary_path)
3868+
.args([
3869+
"compile",
3870+
input_path.to_str().unwrap(),
3871+
"-o",
3872+
output_path.to_str().unwrap(),
3873+
])
3874+
.output()
3875+
.expect("Failed to run compiler");
3876+
3877+
assert!(
3878+
!output.status.success(),
3879+
"Compiler should fail when source path contains whitespace and inlined-imports is false"
3880+
);
3881+
3882+
let stderr = String::from_utf8_lossy(&output.stderr);
3883+
assert!(
3884+
stderr.contains("contains whitespace"),
3885+
"Error message should mention whitespace: {stderr}"
3886+
);
3887+
assert!(
3888+
stderr.contains("inlined-imports: true"),
3889+
"Error message should suggest inlined-imports as an escape hatch: {stderr}"
3890+
);
3891+
3892+
let _ = fs::remove_dir_all(&temp_dir);
3893+
}
3894+
38323895
/// Test that the 1ES fixture produces valid YAML with correct structure
38333896
#[test]
38343897
fn test_1es_compiled_output_is_valid_yaml() {

0 commit comments

Comments
 (0)