Skip to content

Commit 211f498

Browse files
jamesadevineCopilot
andcommitted
fix(compile): correct threat-prompt doc claim and gate required_hosts on actual download
Two findings from the latest PR review: 1. docs/template-markers.md falsely claimed that the `{{ threat_analysis_prompt }}` marker is emitted as a `{{#runtime-import ...}}` when `inlined-imports: false`. The threat-analysis prompt is tooling-shipped (compiled into the `ado-aw` binary via `include_str!`) and unconditionally inlined at step 11 of `compile_shared`. The marker is for the agent body, not the threat prompt. Rewrote the paragraph to reflect this and to cross-reference the rationale in `src/compile/common.rs`. 2. `AdoScriptExtension::required_hosts()` always requested `github.com`, even when `inlined-imports: true` AND no filters were configured (so neither `setup_steps()` nor `prepare_steps()` emits the NodeTool@0 + curl pair, and github.com is therefore unreachable from the pipeline). For a security-sensitive project, the allowlist should match the actual network reach of the compiled pipeline. Now returns `vec![]` unless `has_gate()` or `runtime_imports_active()`. Added three unit tests covering all three branches (no-consumer, gate-active, imports-active). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a0c4504 commit 211f498

2 files changed

Lines changed: 44 additions & 6 deletions

File tree

docs/template-markers.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ Tool names are validated at compile time:
489489

490490
Should be replaced with the embedded threat detection analysis prompt from `src/data/threat-analysis.md`. This prompt template includes markers for `{{ source_path }}`, `{{ agent_name }}`, `{{ agent_description }}`, and `{{ working_directory }}` which are replaced during compilation.
491491

492-
When `inlined-imports: false`, the compiler emits a top-level `{{#runtime-import ...}}` marker pointing at the bundled threat-analysis prompt that ships in `ado-script.zip`; when `inlined-imports: true`, the expanded prompt body is embedded directly into the YAML.
492+
When `inlined-imports: false`, the compiler emits a top-level `{{#runtime-import ...}}` marker pointing at the agent's source `.md` file so the agent body is reloaded from the trigger-repo checkout at pipeline runtime. The threat-analysis prompt itself is **always** inlined at compile time via `include_str!` regardless of `inlined-imports`, because it is tooling-shipped (compiled into the `ado-aw` binary) rather than authored alongside agents. See the comment block at step 11 of `compile_shared` in `src/compile/common.rs` for the rationale; this mirrors gh-aw's model.
493493

494494
The threat analysis prompt instructs the security analysis agent to check for:
495495
- Prompt injection attempts

src/compile/extensions/ado_script.rs

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -195,11 +195,18 @@ impl CompilerExtension for AdoScriptExtension {
195195
}
196196

197197
fn required_hosts(&self) -> Vec<String> {
198-
// Either consumer (gate or import resolver) needs github.com to
199-
// pull the release artifact at runtime. Conservatively always
200-
// requested; the host list is allowlist-additive across
201-
// extensions, so a always-on contribution is benign.
202-
vec!["github.com".to_string()]
198+
// Only request github.com when the bundle is actually downloaded.
199+
// When `inlined-imports: true` AND no filters are configured,
200+
// neither `setup_steps()` nor `prepare_steps()` emits the
201+
// NodeTool@0 + curl pair, so the github.com release-asset host
202+
// is never reached and shouldn't be on the allowlist. The host
203+
// list is allowlist-additive across extensions, so this stays
204+
// safe even when other extensions independently need github.com.
205+
if self.has_gate() || self.runtime_imports_active() {
206+
vec!["github.com".to_string()]
207+
} else {
208+
vec![]
209+
}
203210
}
204211
}
205212

@@ -390,6 +397,37 @@ mod tests {
390397
assert!(ext.validate(&ctx).is_err());
391398
}
392399

400+
#[test]
401+
fn required_hosts_empty_when_no_consumer_active() {
402+
// inlined-imports: true AND no filters ⇒ no NodeTool / no
403+
// download / no gate / no resolver step. The github.com host
404+
// (used to fetch the release asset) is therefore unreachable
405+
// and must NOT be requested.
406+
let ext = ext_with(None, None, true);
407+
assert!(ext.required_hosts().is_empty());
408+
}
409+
410+
#[test]
411+
fn required_hosts_requests_github_when_gate_active() {
412+
let filters = PrFilters {
413+
labels: Some(LabelFilter {
414+
any_of: vec!["run-agent".into()],
415+
..Default::default()
416+
}),
417+
..Default::default()
418+
};
419+
let ext = ext_with(Some(filters), None, true);
420+
assert_eq!(ext.required_hosts(), vec!["github.com".to_string()]);
421+
}
422+
423+
#[test]
424+
fn required_hosts_requests_github_when_runtime_imports_active() {
425+
// inlined-imports: false (default) ⇒ resolver step runs ⇒
426+
// github.com is needed for the bundle download.
427+
let ext = ext_with(None, None, false);
428+
assert_eq!(ext.required_hosts(), vec!["github.com".to_string()]);
429+
}
430+
393431
// ── resolve_imports_inline ─────────────────────────────────────────
394432

395433
static NEXT_ID: AtomicUsize = AtomicUsize::new(0);

0 commit comments

Comments
 (0)