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

[jamesadevine]

Summary

Unifies PRs #363 (upload-artifact) and #373 (upload-build-artifact) into a single upload-build-artifact safe output that handles both use cases — attaching a file to the current pipeline run or to an arbitrary build.

Design

• build_id is Option<i64> — omit to target the current pipeline run (resolved from BUILD_BUILDID in Stage 3). When provided, the file is attached to that specific build.
• REST API for all cases — uses PUT .../builds/{buildId}/attachments/{type}/{name} uniformly, replacing PR feat(safe-outputs): add upload-artifact safe output #363's ##vso[artifact.upload] logging command approach.
• allowed-build-ids bypassed for current build — when the agent omits build_id, the allow-list check is skipped since the current build is implicitly trusted.
• Follows the add-build-tag precedent for resolving ctx.build_id from BUILD_BUILDID.

Stage 1 → Stage 3 file staging contract

Preserves the full staging contract refined in #363:

1. Stage 1 (MCP): validate params → canonicalize + bounds-check source → reject directories → generate staged filename with extension preservation → copy to output_directory → record file size → write NDJSON
2. Stage 3 (Executor): resolve build_id → load operator config → apply name-prefix → check allow-lists (artifact names, extensions, build IDs) → re-canonicalize staged file → re-verify bounds → re-check live file size → read bytes → PUT to ADO REST API

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; skipped for current build
    name-prefix: "agent-"
    attachment-type: "agent-artifact"    # {type} in URL; default "agent-artifact"
    max: 3

Files changed

File	Change
src/safeoutputs/upload_build_artifact.rs	New — params, result, config, executor, 27 tests
src/safeoutputs/mod.rs	Wire module, re-exports, add to safe-output lists
src/execute.rs	Dispatcher entry + budget registration
src/mcp.rs	MCP tool handler with full staging logic
docs/safe-outputs.md	Reference documentation

Test plan

• cargo build ✅
• cargo test ✅ (1088 tests, 0 failures — 27 new tests covering: explicit/omitted build_id, current build resolution, missing BUILD_BUILDID error, allowed-build-ids bypass for current build, oversize file rejection, config defaults, YAML round-trip, serialization)
• cargo clippy --all-targets --all-features ✅

Supersedes #363 and #373.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid defense-in-depth, good test coverage. Two actionable issues found.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_build_artifact.rs ~line 327 — std::fs::read is called before the if ctx.dry_run guard. In dry-run mode, up to 50 MB is read into memory and then discarded without being used. Move the file_bytes read (and the subsequent if ctx.dry_run { return ... }) so the read only happens on the live path:

  // correct ordering
  if ctx.dry_run {
      return Ok(ExecutionResult::success(format!(...)));
  }
  let file_bytes = std::fs::read(&canonical).context("Failed to read file contents")?;

• src/safeoutputs/upload_build_artifact.rs extension allowlist check — the check uses self.file_path.to_lowercase().ends_with(&ext.to_lowercase()) against the full path string. This works correctly only when every entry in allowed-extensions starts with .. If an operator accidentally writes allowed-extensions: ["log"] instead of [".log"], the suffix "log" would match filenames like catalog or syslog (where the last 3 chars are log). Either validate at config-load time that each entry starts with ., or use Path::extension() for the comparison:

  let file_ext = std::path::Path::new(&self.file_path)
      .extension()
      .and_then(|e| e.to_str())
      .unwrap_or("");
  let has_valid_ext = config.allowed_extensions.iter().any(|ext| {
      ext.trim_start_matches('.').eq_ignore_ascii_case(file_ext)
  });

✅ What Looks Good

• Double canonicalize + prefix check in both Stage 1 (MCP) and Stage 3 (executor) — solid defense against symlink escapes and path traversal at both trust boundaries.
• i64::try_from(u64) with .context(...) for the BUILD_BUILDID conversion — correctly handles the overflow edge case.
• allowed-build-ids bypass for the current build is logically sound and clearly documented.
• The UploadBuildArtifactResultFields / tool_result! split cleanly separates the staged-file construction from the NDJSON deserialization path.
• 27 tests cover all the interesting branches (current build, missing env var, oversized file, allow-lists, dry-run).

Generated by Rust PR Reviewer for issue #380 · ● 386.9K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the staging contract is well-implemented and the test coverage is solid. One documentation bug and one minor observation.

Findings

🐛 Bugs / Logic Issues

• docs/safe-outputs.md line ~458 — The ### cache-memory (moved to \tools:`)section heading was removed by this PR, but its body text was left in place. It now appears as a stray paragraph at the end of the### upload-build-artifactsection (between the Notes bullet list and### create-wiki-page), making it look like a note about cache-memory is part of upload-build-artifact's documentation. The fix is to re-add the ### cache-memory (moved to `tools:`)` heading before that line.

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs ~line 590 — std::fs::read(&canonical) loads up to 50 MB synchronously into a Vec<u8> before the reqwest PUT. This works fine in practice (the async executor runs this on a Tokio thread), but it's worth noting as a potential memory spike if many upload-build-artifact outputs are executed in parallel. tokio::fs::File + reqwest::Body::wrap_stream would be the streaming alternative. Not a blocker, and consistent with upload-workitem-attachment.

✅ What Looks Good

• Defense-in-depth path handling — double canonicalization (Stage 1 bounding_directory, Stage 3 working_directory) correctly catches symlink escapes and absolute-path injection in staged_file, with the Stage 3 check using the live file size from disk rather than the NDJSON-recorded value.
• allowed-build-ids bypass for current build — the self.build_id.is_some() guard on the allow-list check is exactly right; no way for the agent to force a cross-build write by omitting build_id.
• is_valid_version reuse — covers [A-Za-z0-9._-] which correctly prevents ##vso[ injection and shell metacharacters in both artifact_name and attachment_type before URL percent-encoding.
• 27 tests covering all the key paths: explicit/omitted build_id, current-build resolution, missing BUILD_BUILDID, allow-list bypass, oversize rejection, YAML round-trip.

Generated by Rust PR Reviewer for issue #380 · ● 580.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean implementation following established patterns. A few items worth addressing.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_build_artifact.rs:435 — std::fs::read() is called synchronously inside an async fn execute_impl, which blocks the tokio runtime for up to 50 MB. The existing upload_workitem_attachment.rs has the same antipattern, but the larger default limit (50 MB vs 5 MB) makes it more impactful here. Should be tokio::fs::read(&canonical).await.

• src/safeoutputs/upload_build_artifact.rs (execute_impl) — self.file_size (recorded in Stage 1 and stored in NDJSON) is never compared against the live file_size re-read in Stage 3. A discrepancy between the two would go undetected — the max_file_size cap still applies to the actual bytes, so this isn't exploitable, but it's a missed integrity signal. Adding if file_size != self.file_size { warn!(...) } (or a hard failure) would catch staged-file tampering between stages.

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs:46 — is_valid_version() is reused to validate artifact_name and attachment_type. The charset ([A-Za-z0-9._-]) happens to be correct for ADO artifact names, but the function is semantically scoped to version strings. If is_valid_version were ever tightened (e.g., "must start with a digit"), artifact name validation would silently break. Consider a dedicated is_valid_artifact_name() validator or a doc comment on each call site noting the intentional reuse.

• src/mcp.rs (upload_build_artifact handler) — the staged filename embeds params.0.artifact_name directly (e.g., upload-build-artifact-{artifact_name}-{id}.ext). The charset is validated before this point so it's safe, but the artifact name can be up to 100 characters — worth a brief comment explaining the max-filename-length reasoning is already considered.

✅ What Looks Good

• Double canonicalization (both in MCP Stage 1 and in the executor Stage 3) is thorough and correctly closes symlink-escape paths at both boundaries.
• The allowed-build-ids bypass for current-build (self.build_id.is_none()) is clean and well-tested — the test test_executor_skips_build_id_allowlist_for_current_build is particularly good.
• Percent-encoding all three user-influenced URL segments (project, attachment_type, final_name) via utf8_percent_encode(..., PATH_SEGMENT) is correct.
• i64::try_from(u64) conversion from ctx.build_id with a proper .context() message is handled without a panic path.
• 27 tests with good coverage of the edge cases (zero/negative build ID, path traversal, oversize, allowlist bypass).

Generated by Rust PR Reviewer for issue #380 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good security posture — two gaps worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_build_artifact.rs — The attachment_type leading-dot guard is absent. The executor validates with is_valid_version(attachment_type) (which allows ".hidden-type"), but the starts_with('.') check that protects artifact_name is missing for attachment_type. Since this is operator-controlled config rather than agent input it's low-severity, but it's an inconsistency worth fixing defensively — the check that exists for artifact_name should also apply here.

⚠️ Suggestions

• src/mcp.rs (MCP handler, Stage 1) — The MCP handler copies files of any size into the safe-outputs staging directory without applying a size guard. The operator's max-file-size is only enforced at Stage 3. Since this is the first tool that physically stages a file at Stage 1, an agent (even a misbehaving one within quota) can fill the staging disk with up to max_file_size × max bytes before Stage 3 gets a chance to reject. The operator config isn't available to the MCP server, but a hardcoded safety cap (e.g. DEFAULT_MAX_FILE_SIZE) at copy time would be a consistent defense-in-depth measure. This also brings the behavior in line with the spirit of the staging contract described in the PR.

• src/safeoutputs/upload_build_artifact.rs (executor tests) — Three config-driven executor paths — allowed_extensions, allowed_artifact_names, and name_prefix — have no unit tests at Stage 3. The PR covers the build-id allowlist and size checks well; these three code paths should have parallel test_executor_rejects_disallowed_extension, test_executor_rejects_disallowed_artifact_name, and test_executor_applies_name_prefix tests to match that level of coverage.

✅ What Looks Good

• Double canonicalization (both MCP Stage 1 and executor Stage 3) guards against symlink escapes correctly.
• Tamper detection (live file size vs. recorded size) is a nice integrity check that has no equivalent in upload_workitem_attachment — good addition to the staging contract.
• Extension matching (Path::extension() + eq_ignore_ascii_case) is strictly better than the suffix-match approach in the sibling upload_workitem_attachment tool; the comment explaining the improvement is appreciated.
• Dry-run guard before file read avoids loading up to 50 MB into memory unnecessarily.
• allowed-build-ids bypass for current build is well-tested and the rationale is clearly documented.

Generated by Rust PR Reviewer for issue #380 · ● 1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — solid design with defense-in-depth, a few minor suggestions worth considering.

Findings

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs — UploadBuildArtifactResultFields::validate() is a no-op
  The staged_file field loaded from NDJSON at Stage 3 has no format validation (the impl Validate for UploadBuildArtifactResultFields {} body is empty). Path traversal is blocked by the double canonicalize+starts_with check in execute_impl, so this is not a security hole — but validating staged_file with is_safe_path_segment per component would be consistent with the codebase's validate-at-entry pattern and add explicit defense-in-depth before the canonicalization logic runs.

• src/safeoutputs/upload_build_artifact.rs — integrity check is size-only
  The cross-stage tamper detection compares file_size != self.file_size. A same-size file swap by a compromised background process would pass. Given the OneBranch sandbox model this is a low-probability attack, but a SHA-256 hash recorded at staging time and verified at execution time would give stronger guarantees for a modest cost. See how upload_workitem_attachment handles this for comparison.

• src/safeoutputs/upload_build_artifact.rs — name_prefix length is unbounded in config
  name_prefix: Option<String> has no maximum length defined in UploadBuildArtifactConfig. A long prefix causes a graceful runtime failure via the final_name.len() > 100 check, but the config quietly accepts malformed values. Capping it (e.g., 50 chars) at deserialization/validation time would give operators a clearer error message.

• src/mcp.rs — minor comment typo
  "Generate a unique staged filenameand copy" — missing space between filename and and.

✅ What Looks Good

• Path traversal prevention is thorough: Stage 1 validates every component of file_path with is_safe_path_segment (correctly blocking /etc/passwd via the empty leading component, .. traversal, .git hidden segments, drive-letter colons, and null bytes). Stage 3 re-canonicalizes the staged path and checks starts_with(working_directory) independently — the double-check is the right pattern here.
• URL construction is safe: effective_build_id is an i64 (no injection), attachment_type and final_name are both URL-encoded via utf8_percent_encode with PATH_SEGMENT, and the project field is similarly encoded.
• allowed-build-ids bypass for the current build is correctly scoped: The guard if self.build_id.is_some() && !config.allowed_build_ids.is_empty() means the check only activates when the agent explicitly targets a different build and the operator has configured a list — the implicit current-build case is cleanly handled.
• i64::try_from(ctx.build_id: u64) overflow check is present with a good error message.
• allowed_extensions comparison correctly uses Path::extension() (no leading dot) and trim_start_matches('.') so operators can write ".pdf" or "pdf" interchangeably, case-insensitively.
• 27 tests cover all the meaningful paths (explicit/omitted build_id, missing BUILD_BUILDID, allow-list bypass for current build, oversized file, tamper detection, prefix application, disallowed extension/name/build-id).
• file_bytes read is gated behind the dry-run check — a 50 MB allocation only happens when a real upload is needed.

Generated by Rust PR Reviewer for issue #380 · ● 1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid defense-in-depth design, comprehensive tests, and well-structured execution flow. Three items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_build_artifact.rs (name-prefix slice panic) — In execute_impl, the oversized-prefix error path contains &prefix[..20]. Since prefix.len() is a byte count and operator-supplied front matter can contain arbitrary UTF-8, this will panic if byte index 20 falls inside a multi-byte character (e.g. a 4-byte emoji makes the boundary at offset 18 or 20 ambiguous). The panic would be in the Stage 3 executor.

  // Panics on multi-byte char boundary:
  &prefix[..20]
  // Safe alternative:
  &prefix.chars().take(20).collect::<String>()

• src/safeoutputs/create_pr.rs (backward-compat deserialization break) — patch_sha256: String is added to CreatePrResultFields as a non-optional, no-serde(default) field. Any create-pr NDJSON record written before this code is deployed (Stage 1 old, Stage 3 new) will fail to deserialize entirely rather than simply failing the hash check. In normal deployments Stage 1 and Stage 3 run in the same build, so the blast radius is small — but worth an #[serde(default)] or Option<String> + explicit check, which would give a clearer error ("no hash recorded — replay attack protection unavailable") rather than an opaque deserialization failure.

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs (utility function placement) — sha256_hex is pub(crate) inside upload_build_artifact.rs but is called from both create_pr.rs and mcp.rs via the full crate::safeoutputs::upload_build_artifact::sha256_hex path. This creates an unexpected coupling between unrelated modules. A small utility in src/sanitize.rs or a dedicated src/hash.rs would make the dependency direction clearer and avoid the module name implying "this SHA utility belongs to artifact uploads."

✅ What Looks Good

• The double-verification contract (file size + SHA-256) across stage boundary is well-designed and tested. The 27 tests cover the boundary conditions thoroughly including the allowed-build-ids bypass for current-build, tampered-file detection, and dry-run paths.
• Path traversal and symlink escape protection is applied correctly at both MCP (Stage 1) and executor (Stage 3) layers — canonicalize + starts_with is the right approach.
• URL construction correctly percent-encodes all operator/agent-supplied path segments (project, attachment_type, final_name) via utf8_percent_encode(..., PATH_SEGMENT) while leaving the base org URL and the integer build ID unencoded.
• The allowed-build-ids bypass for current-build targeting is consistent, documented, and tested — the security rationale is sound.

Generated by Rust PR Reviewer for issue #380 · ● 424.6K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid security model with good layered defenses; one gap in test coverage for the SHA-256 integrity check and a minor code-smell in function reuse.

Findings

🐛 Bugs / Logic Issues

• src/safeoutputs/upload_build_artifact.rs — SHA-256 mismatch is completely untested

  The SHA-256 check (the defense against same-size file swaps) is only executed when dry_run = false, because file_bytes is read after the dry-run early return:

  if ctx.dry_run {
      return Ok(ExecutionResult::success(...)); // ← returns here
  }
  let file_bytes = tokio::fs::read(&canonical).await...?;
  let live_hash = crate::hash::sha256_hex(&file_bytes);
  if live_hash != self.staged_sha256 { ... } // ← never reached in tests

  Every executor test uses dry_run = true (and DUMMY_HASH), so the SHA-256 branch has zero test coverage. Crucially, a non-dry_run test can cover it without ADO credentials — the failure path returns before the HTTP call:

  // In a new test: write 5 bytes, record the wrong hash, dry_run = false
  // → hash check fires and returns ExecutionResult::failure before any HTTP
  let mut ctx = make_ctx(dir.path().to_path_buf(), false /* non-dry-run */);
  ctx.build_id = Some(1);
  ctx.ado_org_url = Some("(dev.azure.com/redacted) // not reached
  // record hash of empty string — won't match 5-byte file
  let result = UploadBuildArtifactResult::new(Some(1), "r".to_string(), "f.pdf".to_string(),
      staged.to_string(), 5, crate::hash::sha256_hex(b""));
  let outcome = result.execute_impl(&ctx).await.unwrap();
  assert!(!outcome.success);
  assert!(outcome.message.contains("SHA-256 mismatch"));

  The analogous path in create_pr.rs has the same gap (no test for the hash-mismatch branch in execute_impl).

⚠️ Suggestions

• src/safeoutputs/upload_build_artifact.rs:77,212 — is_valid_version reused for artifact-name and attachment-type validation

  The comment acknowledges it ("its charset rule happens to match ADO's requirements exactly"), but calling is_valid_version to validate an artifact name is a fragile semantic coupling. If is_valid_version is ever tightened (e.g., to require a leading digit, or to reject multiple consecutive dots for semver reasons), artifact-name validation silently breaks. Consider extracting a is_valid_artifact_name predicate in validate.rs that documents the exact charset [A-Za-z0-9._-] rather than borrowing an unrelated function.

• src/mcp.rs — staged file hash computed from a re-read of the staged copy, not from the source

  tokio::fs::copy(&canonical, &staged_path).await...?;
  let staged_bytes = tokio::fs::read(&staged_path).await...?;  // reads staged copy
  let staged_sha256 = crate::hash::sha256_hex(&staged_bytes);

  There is a narrow TOCTOU window between copy and re-read where a race could cause the recorded hash to differ from what was actually staged. In practice this is unlikely in a pipeline sandbox, but hashing canonical (the source) before copying would eliminate the window and avoid the extra syscall.

✅ What Looks Good

• Double canonicalization at Stage 1 (MCP) and Stage 3 (executor) correctly prevents symlink escapes and path traversal at both trust boundaries.
• The allowed-build-ids bypass for the current build is clearly reasoned, tested (test_executor_skips_build_id_allowlist_for_current_build), and documented.
• Size check (file_size != self.file_size) fires before reading bytes — fast-fail avoids loading a tampered 50 MB file into memory only to reject it.
• SHA-256 addition to create_pr.rs is a nice defence-in-depth improvement for patch integrity and follows the same pattern cleanly.
• STAGE1_MAX_FILE_SIZE constant in MCP correctly mirrors DEFAULT_MAX_FILE_SIZE so the sandbox disk is bounded even before Stage 3 config is loaded.
• 27 new tests cover the major allow-list paths and resolution logic well.

Generated by Rust PR Reviewer for issue #380 · ● 434.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured, strong security posture, thorough test coverage. Three items worth discussing before merge.

---

Findings

⚠️ Suggestions

• src/safeoutputs/create_pr.rs:272 — Breaking deserialization for in-flight pipelines

  patch_sha256: String is added to CreatePrResultFields (the serde deserialization struct) without #[serde(default)]. If a pipeline had Stage 1 complete with an old binary (writing NDJSON without this field) and Stage 3 then runs with this new binary, the JSON deserialization will hard-fail rather than producing a clear error. Adding #[serde(default)] would let Stage 3 accept old records gracefully — it would just skip the hash check when the field is absent (e.g., treat empty string as "no recorded hash, skip check"). New records will always have the field, so the check stays effective going forward.

  // in CreatePrResultFields:
  #[serde(default)]
  patch_sha256: String,

  Then in execute_impl, guard the check:

  if !self.patch_sha256.is_empty() && live_hash != self.patch_sha256 {
      // fail
  }

  Same applies to staged_sha256 in UploadBuildArtifactResultFields (new field, no existing records, so less urgent there).

• src/mcp.rs — STAGE1_MAX_FILE_SIZE is a floating copy of DEFAULT_MAX_FILE_SIZE

  Both constants are defined independently at 50 * 1024 * 1024 with a comment saying "matches DEFAULT_MAX_FILE_SIZE". There's no compile-time linkage. A future change to DEFAULT_MAX_FILE_SIZE in upload_build_artifact.rs would silently leave the Stage 1 guard unchanged. Consider importing/re-exporting the constant:

  // in mcp.rs
  use crate::safeoutputs::UPLOAD_BUILD_ARTIFACT_MAX_FILE_SIZE;

  or at minimum a static assertion if cross-crate visibility is unwanted.

• src/safeoutputs/upload_build_artifact.rs:execute_impl — SHA-256 integrity check is skipped in dry-run mode

  The SHA-256 check is placed after the dry-run guard (by design, to avoid reading ≤50 MB into memory for dry runs). This is asymmetric with create-pr, where the hash check runs before the dry-run return. In dry-run mode for upload-build-artifact, a tampered staged file won't be detected. This is acceptable given it's an explicit trade-off documented in a test comment, but it's worth a note in the code so future reviewers don't unknowingly tighten the dry-run path and break the performance intent (or move the SHA check before the dry-run guard without realizing the cost).

---

✅ What Looks Good

• Double-canonicalization in both stages: MCP (Stage 1) and the executor (Stage 3) independently resolve the path and verify it stays inside its respective bounding directory. Symlink escape is covered at both ends.
• SHA-256 over file bytes, not the staged path re-read: Computing the hash from source_bytes before writing (MCP) and from file_bytes after reading (executor) eliminates any TOCTOU window between copy and re-hash.
• Build-ID allowlist bypass for current build: The design — skip allowed-build-ids when build_id is None (current build) — is sound and well-tested.
• Extension check uses Path::extension() rather than a suffix match on the full path, preventing catalog from matching log.
• 27 new tests with good coverage across: explicit/omitted build ID, missing BUILD_BUILDID, allowlist enforcement and bypass, oversize rejection, SHA-256 mismatch, and name-prefix application.

Generated by Rust PR Reviewer for issue #380 · ● 1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — the two-stage file-integrity model (size + SHA-256) is well-designed and the validation layering is correct. Two minor findings below.

---

Findings

⚠️ Suggestions

• src/safeoutputs/create_pr.rs:272 — patch_sha256: String in CreatePrResultFields (the NDJSON deserialization struct) has no #[serde(default)]. This makes the wire format non-backward-compatible: an older Stage 1 NDJSON piped to a newer Stage 3 executor will fail deserialization with a missing-field error. For the current same-run model (Stage 1 and Stage 3 always use the same binary in one pipeline execution) this is a non-issue. But if the format is ever versioned or a migration path is needed, this field will silently break older payloads. Worth adding #[serde(default)] defensively, matching how optional fields are handled elsewhere in this codebase.

• src/safeoutputs/upload_build_artifact.rs — validate() error message — The error string "(no empty, '..', or leading '.' allowed)" understates what is_safe_path_segment actually rejects. The function also rejects any component containing .. as a substring (e.g. foo..bar fails). The validation itself is correct and more conservative than the message implies, but a misleading error message could confuse operators debugging legitimate filenames like v1..2 or build..log.

✅ What Looks Good

• Defense-in-depth staging contract: canonicalize + bounds check in both Stage 1 (MCP) and Stage 3 (executor) — symlink escapes are caught at both ends.
• SHA-256 computed from source bytes before writing (not re-read after write), which eliminates the TOCTOU window between copy and re-hash.
• allowed-build-ids bypass for current build is correctly keyed on self.build_id.is_none() (the agent's intent) rather than comparing effective_build_id against the allow-list — avoids a confused-deputy scenario.
• Dry-run guard placement: size and integrity checks run before the dry-run short-circuit, so dry-run still validates the staged file's metadata; only the hash and network call are skipped. The test coverage (27 tests, including the dry_run=false SHA-256 mismatch test) confirms this path is exercised correctly.
• sha256_hex as a dedicated src/hash.rs module with known-answer tests is a clean extraction — avoids subtle reimplementation divergence if the same primitive is needed elsewhere.

Generated by Rust PR Reviewer for issue #380 · ● 883.7K · ◷