feat(safe-outputs): add upload-build-artifact safe output

[Copilot]

Summary

Adds upload-build-artifact, a safe output that lets an agent attach a workspace file to an arbitrary (typically already-finished) Azure DevOps build via the build attachments REST API. Fills the gap left by upload-artifact (#363), which can only target the current run via ##vso[artifact.upload].

The split mirrors create-pull-request and upload-artifact:

• Stage 1 (MCP, in sandbox) — validates build_id / artifact_name / file_path, canonicalises the file inside the agent workspace (rejecting symlink escapes and directories), and copies it into output_directory under a generated unique name. The sandbox workspace is gone by Stage 3, so the file has to be staged.
• Stage 3 (executor) — re-canonicalises the staged file inside working_directory, applies operator policy, then PUTs the bytes to …/_apis/build/builds/{buildId}/attachments/{type}/{name}?api-version=7.1-preview.1.

Files

• src/safeoutputs/upload_build_artifact.rs — new tool: UploadBuildArtifactParams, UploadBuildArtifactResult (with build_id, artifact_name, file_path, staged_file, file_size and an explicit ::new() constructor), internal UploadBuildArtifactResultFields to satisfy the tool_result! macro, validation reusing is_valid_version / is_safe_path_segment from src/validate.rs, and the Stage 3 REST executor (+23 unit/executor tests).
• src/safeoutputs/mod.rs — module, re-export, WRITE_REQUIRING_SAFE_OUTPUTS, ALL_KNOWN_SAFE_OUTPUTS, requires-write test.
• src/execute.rs — dispatcher entry and per-run budget registration.
• src/mcp.rs — upload-build-artifact MCP tool: Stage 1 validation, workspace-bounded canonicalisation, directory rejection, file copy into output_directory, NDJSON write.
• docs/safe-outputs.md — reference section.

Front-matter shape

safe-outputs:
  upload-build-artifact:
    max-file-size: 52428800              # default: 50 MB
    allowed-extensions: [".png", ".pdf", ".log"]
    allowed-artifact-names: ["agent-*"]  # `*` suffix → prefix match
    allowed-build-ids: [12345, 67890]    # empty = any build the token can reach
    name-prefix: "agent-"
    attachment-type: "agent-artifact"    # used as {type} in the URL; default "agent-artifact"
    max: 3

Notes / future work

• Single-file only; directory uploads can be layered on later.
• attachment-type is operator-controlled and re-validated against the artifact-name charset before being interpolated into the URL.
• The default attachment-type is agent-artifact so the executor's contributions are visually distinguishable from the build's own pipeline-artifact list.

Test plan

• cargo build ✅
• cargo test ✅ (all green; 23 new tests covering name/path/build-id validation, config defaults + YAML round-trip, serialization, and Stage 3 executor behaviour against a staged file in a temporary working directory — including allow-list, oversize, and missing-file rejection paths)
• cargo clippy --all-targets --all-features ✅ (no new warnings on the new file)
• Code-review feedback addressed: log a warning when the ADO build-attachment response JSON can't be parsed instead of silently swallowing the error.

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Well-implemented — the two-phase path canonicalization and defense-in-depth are solid. Two minor issues worth addressing before merge.

---

Findings

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs — extension check operates on NDJSON file_path, not the staged file's actual extension

  In execute_impl, allowed-extensions are tested against self.file_path (the agent-supplied value stored in NDJSON):

  let has_valid_ext = config.allowed_extensions.iter().any(|ext| {
      self.file_path.to_lowercase().ends_with(&ext.to_lowercase())
  });

  The NDJSON field is agent-writable (Stage 1), so a malicious agent could write file_path: "report.pdf" while staging a .exe. Stage 2 detection is the safety net here, but checking the staged filename's extension (which Stage 1 derives from file_path and embeds in the staged name) would be strictly tamper-resistant:

  let has_valid_ext = config.allowed_extensions.iter().any(|ext| {
      self.staged_file.to_lowercase().ends_with(&ext.to_lowercase())
  });

  This matches how Stage 1 constructs staged_filename — the extension is already embedded there.

• src/safeoutputs/upload_build_artifact.rs — name_prefix not charset-validated at config parse time

  attachment_type gets an explicit is_valid_version check before URL interpolation (good). name_prefix does not — it's only sanitized by SanitizeConfig (strips control chars), and the combined final_name is validated at runtime instead. This means an operator who configures name-prefix: "my prefix " (with a space) gets an opaque runtime error saying "Resolved artifact name 'my prefix agent-report' is not valid" rather than a clear config-time error. Symmetry with attachment_type suggests validating name_prefix's charset at config load time (or in Validate).

✅ What Looks Good

• Two-phase path safety — Stage 1 canonicalize + starts_with(bounding_directory) and Stage 3 canonicalize + starts_with(working_directory) are both correct and catch symlink escapes. The ordering (canonicalize first, then prefix-check) is right.
• staged_file NDJSON value is canonicalize-checked at Stage 3 — even if the NDJSON is tampered with a path traversal value, the starts_with(canonical_base) guard catches it.
• attachment_type re-validated before URL interpolation — the is_valid_version guard on attachment_type before it's embedded in the URL is solid defense-in-depth.
• final_name re-validated at Stage 3 — is_valid_version(&final_name) after prefix concatenation prevents operator-prefix-induced charset violations from reaching the URL.
• HTTP error handling — response.json().await.unwrap_or_else(|e| { warn!(...) }) correctly degrades gracefully rather than panicking on a malformed success response.
• unwrap() uses are all test-only — production code paths consistently use ?/.context()/anyhow::bail!.

Generated by Rust PR Reviewer for issue #373 · ● 729.2K · ◷