fix(safe-outputs): create build attachments via DistributedTask timeline attachment API

[jamesadevine]

Summary

Fixes upload-build-attachment, which failed against the live ADO server in the
deterministic executor-e2e pipeline (build 619362):

Failed to attach artifact to build #619362 (HTTP 404 Not Found):
The controller for path '/AgentPlayground/_apis/build/builds/619362/attachments/…'
was not found or does not implement IController.

Root cause: the executor PUTs to
/_apis/build/builds/{id}/attachments/{type}/{name}, but ADO's Build ▸
Attachments REST API is read-only (Get/List only) — that create route
never existed, so every live upload 404'd as an unmatched route.

Fix

Build attachments are created via the DistributedTask timeline attachment
API — the same mechanism as the ##vso[task.addattachment] logging command. The
resulting object is a build attachment (same {type}/{name}, read back via
the Build ▸ Attachments Get/List API); only the write endpoint changes. Because a
timeline attachment targets the current job's record, it is current-run only.

Verified working endpoint (empirically confirmed):

PUT {org}/{SYSTEM_TEAMPROJECTID}/_apis/distributedtask/hubs/build
    /plans/{SYSTEM_PLANID}/timelines/{SYSTEM_TIMELINEID}/records/{SYSTEM_JOBID}
    /attachments/{type}/{name}?api-version=7.1

• Scope identifier is the project GUID (SYSTEM_TEAMPROJECTID) — the project
  name routes but is rejected HTTP 400; a collection-scoped URL 404s.
• The attachment URL is read from _links.self.href (there is no top-level
  url field).

Changes

• result.rs: add plan_id / timeline_id / job_id to ExecutionContext
  from SYSTEM_PLANID / SYSTEM_TIMELINEID / SYSTEM_JOBID (auto-injected
  predefined vars — no compiler change needed).
• upload_build_attachment.rs: switch to the timeline-attachment endpoint;
  reject a build_id that differs from the current run with a clear message;
  remove the now-meaningless allowed-build-ids config; parse
  _links.self.href; rewrite the module doc.
• Codemod 0005_drop_build_attachment_allowed_build_ids: auto-strips
  safe-outputs.upload-build-attachment.allowed-build-ids (with a compile
  warning). Leaves upload-pipeline-artifact.allowed-build-ids untouched.
• mcp.rs tool description + docs/safe-outputs.md / docs/codemods.md.

Semantic note

There is no user-facing difference in the resulting attachment — a timeline
attachment is a build attachment, and it is read back through the same Build ▸
Attachments API. The only behavioural change is that attaching to an arbitrary
other build is no longer offered (it was never actually possible).

Testing

• cargo test full suite (2511 unit + all integration), cargo clippy --all-targets --all-features — clean. (No cargo fmt: local rustfmt 1.9.0
  reformats unrelated committed code and CI has no fmt gate.)
• Live validation: the deterministic executor-e2e pipeline (def 2541) run on
  this branch (build 619391) passes: 20 total, 19 passed, 0 failed, 1
  skipped, with [PASS] upload-build-attachment.

Fixes the failure tracked in jamesadevine/ado-aw-issues#8.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — correct fix for a real API bug, well-documented, follows project conventions throughout.

Findings

⚠️ Suggestions

• src/safe_outputs/upload_build_attachment.rs (test_executor_rejects_sha256_mismatch) — The comment says // Record the hash of different content — same size but wrong hash. but the sizes actually differ: "real file content" = 17 bytes, "wrong file content" = 18 bytes. The test is functionally correct (the file size check passes because self.file_size = content.len() records the actual on-disk size, so SHA-256 fires), but the comment is misleading. Suggest either making the strings genuinely the same length or correcting the comment to "different content, wrong hash".

• src/execute.rs (test) — The test was relaxed from assert!(result.is_err()) to if let Ok(r) = result { assert!(!r.1.success, ...) }. This is reasonable given the executor now returns Ok(ExecutionResult::failure(...)) for missing env vars rather than hard errors, but it means the test passes vacuously if result is Ok(_, success=false) for a completely different reason. The comment explaining the BUILD_BUILDID ambiguity is helpful; no blocker, just noting the test is weaker now.

• src/safe_outputs/upload_build_attachment.rs (execute_impl) — The new .context(...)? paths for project_id, plan_id, timeline_id, and record_id return Err(...) (infra failure) rather than Ok(ExecutionResult::failure(...)) (user-visible failure). These paths have no direct unit tests — the SHA-256 test covers them only accidentally (SHA-256 fires first). Not a blocker given these are operator-misconfigured-env failures, but a single test verifying at least one of these .context() paths returns Err would add confidence.

✅ What Looks Good

• Root cause fix is correct: The PR description clearly explains why the old PUT /_apis/build/builds/{id}/attachments/... never worked (read-only endpoint), and the switch to the DistributedTask timeline attachment API is the right call — empirically confirmed in build 619391.
• Security model is solid: SHA-256 integrity check prevents file substitution between stages; canonicalize() + starts_with(canonical_base) prevents path traversal; all user/operator-supplied URL segments go through utf8_percent_encode with PATH_SEGMENT; attachment_type has defensive charset validation before interpolation.
• Codemod 0005 is well-scoped: Correctly strips only upload-build-attachment.allowed-build-ids while leaving upload-pipeline-artifact.allowed-build-ids untouched; 5 test cases cover removal, sibling preservation, no-op, absent key, and idempotency.
• build_id matching logic is exhaustive: The four-arm match on (self.build_id, current_build_id) covers all cases with clear error messages; the _current discard in (Some(requested), Some(_current)) => requested is correct (the preceding guard already proved equality).
• project_id (GUID, not name) is used in the URL: The comment correctly notes the project name routes but returns HTTP 400; using SYSTEM_TEAMPROJECTID for the scope identifier is the right fix as documented empirically.

Generated by Rust PR Reviewer for #1433 · 66.4 AIC · ⌖ 6.62 AIC · ⊞ 6.2K · ◷

[jamesadevine]

Thanks for the review. Addressed in 129377e:

• sha256 test comment: switched to two genuinely 17-byte strings (real file content / fake file content), so the "same size, wrong hash" claim now holds and the test truly exercises the SHA-256 check independent of the size check.
• .context()? env paths: there was already a direct test for the SYSTEM_PLANID path (test_executor_fails_when_plan_id_missing); added test_executor_fails_when_project_id_missing to also cover the SYSTEM_TEAMPROJECTID scope-identifier path, so these no longer rely on the SHA-256 test firing first.
• execute.rs relaxed assertion: left as-is by design — the executor now returns Ok(ExecutionResult::failure(...)) for an undeterminable current run rather than a hard error, and the outcome legitimately depends on whether the host has BUILD_BUILDID set. The comment documents the ambiguity.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — correct fix for a real production bug, well-tested, with minor notes below.

Findings

✅ What Looks Good

• Correct API endpoint. The switch from PUT /_apis/build/builds/{id}/attachments/... (never existed — Build Attachments API is read-only) to the DistributedTask timeline attachment endpoint is the right fix, empirically validated. Project GUID (SYSTEM_TEAMPROJECTID) is used as the URL scope identifier as required (project name causes HTTP 400).
• URL encoding. Every user-controlled path segment (project_id, plan_id, timeline_id, record_id, attachment_type, final_name) is wrapped in utf8_percent_encode(..., PATH_SEGMENT).
• build_id validation exhaustiveness. All four (self.build_id, current_build_id) match arms are covered: (Some, Some) match, (Some, Some) mismatch, (Some, None), (None, None). Each returns a clear ExecutionResult::failure rather than a panic or silent success.
• Graceful unknown-field handling. Removing allowed_build_ids from the struct without #[serde(deny_unknown_fields)] means stray allowed-build-ids keys in pre-migration files are silently ignored — confirmed by test_config_ignores_stray_allowed_build_ids.
• Response URL extraction. _links.self.href is the correct field for the timeline-attachment response, with a defensive url fallback.
• Codemod + tests. Codemod is idempotent, preserves sibling config, and correctly leaves upload-pipeline-artifact.allowed-build-ids untouched.

⚠️ Suggestions

• (Some(requested), None) arm is untested. test_executor_fails_when_build_id_omitted_and_not_in_env covers (None, None), but there is no test for the case where the agent supplies a build_id while BUILD_BUILDID is unset — i.e. ctx.build_id = None, self.build_id = Some(x). The branch is small and the message is correct, but a dedicated test would pin the behavior.

• execute.rs assertion relaxation (assert!(result.is_err()) → if let Ok(r) = result { assert!(!r.1.success) }). The rationale is sound — the error path moved from anyhow::Err to Ok(ExecutionResult::failure(...)) — but the relaxed form silently passes if result is Err(...) for an unexpected reason. A slightly tighter form would be:

  match result {
      Ok((_entry, r)) => assert!(!r.success, "expected non-success, got: {:?}", r),
      Err(_) => {} // hard error also counts as non-success
  }

  Minor, but worth noting.

• Early info! log uses pre-prefix name. src/safe_outputs/upload_build_attachment.rs logs self.artifact_name before final_name (the post-prefix name) is computed. The later success log correctly uses final_name. Cosmetic only, but can confuse log readers when a name-prefix is configured.

Generated by Rust PR Reviewer for #1433 · 130.1 AIC · ⌖ 6.42 AIC · ⊞ 6.2K · ◷