fix(safeoutputs): rewrite upload-pipeline-artifact flow to fix HTTP 405

[jamesadevine]

Why

Stage 3 was failing with HTTP 405 Method Not Allowed on every upload-pipeline-artifact:

upload-pipeline-artifact failed: Failed to create artifact container (HTTP 405 Method Not Allowed):
{"count":1,"value":{"Message":"The requested resource does not support http method 'POST'."}}

The prior implementation tried to create a per-artifact file container via POST /_apis/resources/containers, which is not a real Azure DevOps endpoint — only GET (list containers) is registered for the unscoped /_apis/resources/Containers/{containerId}/{*itemPath} route. The earlier #421 attempt to add a scopeIdentifier= query parameter did not help because no POST handler exists at the unscoped path on the server side.

What changed

New 2-step flow that all official Microsoft tools (PublishBuildArtifacts@1, the agent's artifact.upload log command, every SDK's FileContainerApi) actually use:

1. PUT bytes to /_apis/resources/Containers/{BUILD_CONTAINERID}?itemPath={folder}/{file}&scope={projectId} — into the agent's own pre-existing build container (Azure DevOps creates one container per build at job init and exposes its ID via BUILD_CONTAINERID).
2. POST an artifact record to /{project}/_apis/build/builds/{effective_build_id}/artifacts with resource.type = "Container" and resource.data = "#/{containerId}/{folder}".

Cross-build publishing now works as a side effect. The artifact record on a target build is just a pointer; ADO does not require the container to belong to the target build (this is exactly how DownloadBuildArtifacts@1 buildType=specific already works in the wild). The bytes physically live in the agent's own build container, so cross-published artifacts share the agent build's retention — documented in docs/safe-outputs.md.

Avoid silent byte-overwrites when multiple calls in one run share an artifact_name (e.g. publishing the same TriageSummary to many failing builds at once): the executor inserts a 6-character hash discriminator into the internal container folder name ({artifact_name}__{6 hex}). The hash is invisible in standard download paths (web UI zip wrapper, DownloadBuildArtifacts@1, DownloadPipelineArtifact@2 — all strip the prefix); the user-visible artifact_name your downstream consumers query is unaffected.

New require-unique-names: true config opt-out — uses a clean folder name and rejects in-run reuse of (effective_build_id, artifact_name) early with a clear error. Use this when you guarantee one artifact per name per run and want the shortest possible internal addressing.

Other fixes:

• Switched upload query param scopeIdentifier= → scope= (canonical name; all SDKs use scope; scopeIdentifier is a response model field name, not a query key).
• Fixed resource.type casing: Container (was lowercase container).
• Better associate-POST error messages on 401/403/404/409 with actionable hints (likely missing scope, cross-project not supported, name already used on the target build).

How to test

Compiles, all 1269 existing tests + 4 new tests pass, no new clippy warnings.

cargo build
cargo test
cargo clippy --all-targets --all-features

Manual smoke test (recommended before relying on this in production)

1. Cross-build publish to a completed target build. Microsoft does not document a build-state restriction on POST /builds/{B}/artifacts, but it is the riskiest unverified piece of the flow. The triage scenario specifically depends on this working.
2. Confirm the container folder hash suffix does not surface prominently in the inline 'Browse' view of the ADO Artifacts tab. The web UI zip download definitely strips the prefix; the inline browser drill-down behavior is uncertain from public documentation.

If either fails, the follow-ups are:

• For the completed-build rejection: switch to PUT .../builds/{B}/attachments/... (build attachments). This loses Artifacts-tab visibility but is otherwise equivalent.
• For an ugly browse view: set require-unique-names: true per-pipeline to remove the suffix entirely.

Closes

Supersedes #421 (which addressed the wrong root cause).

[github-actions]

🔍 Rust PR Review

Summary: The core fix is correct and well-motivated. Two design issues worth addressing before merge — one is a genuine bug in the require-unique-names deduplication path.

Findings

🐛 Bugs / Logic Issues

src/safeoutputs/upload_pipeline_artifact.rs lines 449–463 — Dedupe key inserted before HTTP calls

if config.require_unique_names {
    let mut seen = ctx.uploaded_pipeline_artifact_keys.lock()...;
    if seen.contains(&dedupe_key) { return Ok(ExecutionResult::failure(...)); }
    seen.insert(dedupe_key);   // ← inserted here, before any HTTP call
}
// ... then Step 1 PUT + Step 2 POST ...

If Step 1 (PUT bytes) or Step 2 (POST artifact record) fails with a transient/network error, the key stays in the set permanently for that Stage 3 run. A second entry in the same NDJSON batch that retries the same (effective_build_id, artifact_name) pair would receive the misleading "already used / require-unique-names is configured to reject reuse" error instead of the real HTTP error. Move the seen.insert(dedupe_key) to after a fully successful execute (immediately before the Ok(ExecutionResult::success(...)) at the bottom).

---

⚠️ Suggestions

src/safeoutputs/upload_pipeline_artifact.rs line 477 — Hash discriminator is derived from staged file name, not content

let disc = &crate::hash::sha256_hex(self.staged_file.as_bytes())[..6];

The module doc comment says the suffix prevents "silently overwriting each other's bytes", but if two calls share the same staged_file path (e.g. publishing an identical triage summary file to N failing builds in one run), they produce the same container_folder. The N subsequent PUT calls then overwrite each other — idempotently for identical content, but silently if the content differs. For the described triage use case this is fine (same file, N target builds), but the comment overstates the guarantee. Recommend either:

• Clarifying the comment to "differentiates uploads from distinct staged files in one run", or
• Deriving the disc from the content hash that's already computed (self.file_hash) to make the guarantee actually hold regardless of filename.

Using self.file_hash (already read in the executor) as the source of the discriminator would be strictly more correct with no extra I/O cost.

src/safeoutputs/result.rs line 454 — expect() on mutex lock

.expect("uploaded_pipeline_artifact_keys mutex poisoned");

Consistent with anyhow usage elsewhere this could be .map_err(|e| anyhow::anyhow!("mutex poisoned: {}", e))?. Minor, since mutex poisoning is a programming error, but the rest of the executor propagates errors via ?.

---

✅ What Looks Good

• The root cause diagnosis is correct: POST /_apis/resources/containers (unscoped) is not a real ADO endpoint. Switching to PUT into BUILD_CONTAINERID matches what all official Microsoft tooling uses.
• scopeIdentifier → scope fix and "container" → "Container" casing fix are both real bugs in the prior implementation.
• The 401/403/404/409 hint messages on the associate POST are high-signal and actionable.
• BUILD_CONTAINERID env-var parsing (numeric, non-numeric, unset) is fully covered by three focused unit tests.
• Cross-build retention caveat (agent build purge stops artifact from being downloadable) is clearly documented in both the code and docs/safe-outputs.md.
• URL construction uses utf8_percent_encode consistently for all user-supplied path segments — no injection risk.

Generated by Rust PR Reviewer for issue #425 · ● 655.4K · ◷

[jamesadevine]

@copilot address review bugs and suggestions

[Copilot]

@copilot address review bugs and suggestions

Done in bfb4535. Three fixes applied to src/safeoutputs/upload_pipeline_artifact.rs:

1. Bug (dedupe key inserted before HTTP calls): Moved seen.insert(dedupe_key) to immediately before the success return, so a transient network error on Step 1 or Step 2 doesn't permanently block retries of the same artifact in the same run.

2. Discriminator now uses content hash: Changed crate::hash::sha256_hex(self.staged_file.as_bytes()) → &live_hash[..6] (the SHA-256 of the actual file bytes already computed for integrity checking). Distinct content always maps to a distinct container folder; identical content maps to the same folder (idempotent PUT — safe). Updated the test to mirror this logic.

3. Replaced .expect() on mutex lock with ?: Both lock sites now use .map_err(|e| anyhow::anyhow!("... poisoned: {}", e))?, consistent with the rest of the executor's error propagation style.