fix(safe-outputs): upload-pipeline-artifact uses $(System.AccessToken) (#471)

[jamesadevine]

Closes #471.

Root cause

Build 610504 failed at PUT /_apis/resources/Containers/{id} with HTTP 404 ContainerWriteAccessDeniedException.

The ADO File Container ACL is keyed on the build''s job-plan identity (the Project Build Service account). When permissions.write: is set, Stage 3 currently overrides SYSTEM_ACCESSTOKEN with $(SC_WRITE_TOKEN) — an ARM SPN bearer minted via AzureCLI@2. ARM SPNs are never in the File Container ACL regardless of project-level RBAC, so the PUT is rejected with 404 (ADO returns 404 instead of 403 for ACL denials).

Phase 1 — surgical fix

This PR unblocks #471 with a minimal change scoped to upload-pipeline-artifact:

• New SYSTEM_TOKEN_SAFE_OUTPUTS category in src/safeoutputs/mod.rs containing UploadPipelineArtifactResult. Handlers in this category are excluded from WRITE_REQUIRING_SAFE_OUTPUTS and from validate_write_permissions''s gating.
• generate_executor_ado_env now emits ADO_SYSTEM_ACCESS_TOKEN: $(System.AccessToken) alongside the existing SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN) whenever any SYSTEM_TOKEN_SAFE_OUTPUTS handler is configured.
• ExecutionContext gains a system_access_token field, populated from ADO_SYSTEM_ACCESS_TOKEN.
• upload_pipeline_artifact reads system_access_token first and falls back to access_token. Every other handler is bit-identical.

The change is deliberately narrow: 13 other write-requiring handlers continue to use the ARM SPN token. No schema change, no codemod, no deprecation window.

Orthogonal cleanup

• Removed build_id and allowed-build-ids from upload-pipeline-artifact. The File Container ACL constraint makes cross-build publishing impossible; the fields never worked and silently rejected requests at runtime. Documented in the rewritten module docstring.

Forward look — Phase 2

This PR is the first half of a larger gh-aw-aligned redesign. Phase 2 will introduce per-handler service-connection: declarations, a block-level safe-outputs.service-connection: default, and a codemod that auto-migrates the top-level permissions: schema. The SYSTEM_TOKEN_SAFE_OUTPUTS category introduced here becomes the mechanism for "use the Build Service identity" handlers in that redesign.

Validation

• cargo build ✅
• cargo test ✅ (1758 unit + integration, zero failures)
• cargo clippy --all-targets --all-features ✅ (zero warnings)
• tests/safe-outputs/upload-pipeline-artifact.lock.yml regenerated; the new env block contains both SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN) and ADO_SYSTEM_ACCESS_TOKEN: $(System.AccessToken).
• No other dogfood or example pipeline currently uses upload-pipeline-artifact, so no further lock-file diffs.

Docs updated

• docs/safe-outputs.md — ### upload-pipeline-artifact rewritten (current-build-only; uses $(System.AccessToken); no build_id).
• docs/template-markers.md — {{ executor_ado_env }} updated.
• docs/network.md — Security Model reframed with the upload-pipeline-artifact exception.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the fix is correct, well-scoped, and the test coverage is thorough.

Findings

⚠️ Suggestions

• src/safeoutputs/upload_pipeline_artifact.rs (token fallback comment): The fallback from system_access_token → access_token is documented as being for "test fixtures and older compiled lock files". However, the lock file fixture in this same PR already emits ADO_SYSTEM_ACCESS_TOKEN, so in practice the fallback is only ever hit in unit tests. Consider tightening the comment (or adding a note) so future maintainers know the fallback path has no production use and shouldn't be extended.

• validate_write_permissions filter order (src/compile/common.rs): The two .filter() calls on WRITE_REQUIRING_SAFE_OUTPUTS are ordered SYSTEM_TOKEN exclusion first, then contains_key. Because the disjointness invariant is enforced by the compile-time test, the SYSTEM_TOKEN filter is redundant (the entries can never appear in WRITE_REQUIRING_SAFE_OUTPUTS). It's harmless but adds an O(n) inner scan per outer iteration. A short comment noting it's a forward-compatibility safety net would prevent accidental removal.

• Missing end-to-end fixture test for needs_system_access_token (src/compile/common.rs): The unit tests in this PR validate generate_executor_ado_env directly, but there's no integration/fixture test asserting that a compiled pipeline with upload-pipeline-artifact in safe-outputs: produces ADO_SYSTEM_ACCESS_TOKEN: $(System.AccessToken) in the output YAML. The lock file update is evidence it works, but a test in tests/compiler_tests.rs would make the regression surface explicit.

✅ What Looks Good

• The SYSTEM_TOKEN_SAFE_OUTPUTS / WRITE_REQUIRING_SAFE_OUTPUTS split is clean; the compile-time disjointness tests will catch any accidental double-registration.
• Token selection (system_access_token.or(access_token)) is handled in exactly one place — easy to audit.
• Removing build_id / allowed-build-ids is the right call: the cross-build path never worked with the File Container ACL, and keeping dead config fields would have been a support burden.
• test_config_ignores_removed_keys correctly validates backward-compat for old lock files that still carry allowed-build-ids.
• All three new token-selection tests (prefers_system_access_token, falls_back_to_access_token, fails_when_no_token_available) cover exactly the right boundary conditions.

Generated by Rust PR Reviewer for issue #835 · sonnet46 1.8M · ◷