fix(compile): emit prepare-pr-base in the SafeOutputs job (#1453)

[jamesadevine]

Summary

Closes #1453.

#1425 (v0.43.0) fixed the shallow-clone diff-base problem by adding a host-side "Prepare create-pull-request base ref (fetch/deepen)" step (prepare-pr-base.js). But that step was emitted only in the Agent job, while the create-pull-request executor (ado-aw execute) runs in the SafeOutputs job — a separate ADO job with its own fresh, shallow checkout: self. Each ADO job gets an isolated checkout, so the Agent-job fetch is invisible there and the executor's git worktree add origin/<target> fails:

Worktree creation with origin/ prefix failed, trying without: fatal: invalid reference: origin/develop
Failed to create worktree: fatal: invalid reference: develop

This PR emits the existing, tested prepare-pr-base bundle step in the SafeOutputs job too, immediately before ado-aw execute (issue option a). It reuses all existing machinery, so the ref is landed in the same checkout that builds the worktree.

Key changes

• src/compile/agentic_pipeline.rs — extracted create_pr_prepare_repos (pure per-repo (dir, target) pairs) and warn_create_pr_target_inference (advisory emitted once, Agent-job only). build_safeoutputs_job now stages the ado-script bundle (install_and_download_steps_typed) and emits prepare_pr_base_step_typed(...) before the executor. build_safeoutputs_job is shared via build_canonical_jobs, so this covers standalone, 1es, job, and stage targets.
• Split-approval scoping — the prepare step + bundle download is gated on a per-variant runs_create_pull_request flag, so in a split-approval config (create-pull-request with require-approval alongside a non-gated tool) only the variant that actually executes create-pull-request pays for the Node install + fetch/deepen; the other variant, which filters the tool out, no longer does wasted work.
• tests/safe-outputs/create-pull-request.lock.yml — regenerated golden lock (SafeOutputs job now has Node install + ado-script staging + prepare step).
• tests/compiler_tests.rs — new tests asserting the prepare step (self + per-repo targets) appears in the SafeOutputs job before the executor, plus split-approval tests proving the step lands only in the running variant (both gated and sibling-gated cases).
• scripts/ado-script/test/smoke.test.ts — hermetic real-git smoke test that builds a shallow single-branch clone missing origin/<target>, asserts git worktree add origin/<target> fails (reproducing [aw]: Follow-up to #1413 / #1425: create-pull-request worktree fails — the prepare-pr-base step runs in the Agent job, but the executor runs in the SafeOutputs job (standalone target) #1453), runs the real prepare-pr-base.js, then asserts the same worktree add succeeds. No ADO/network. package.json test:smoke now builds the prepare-pr-base bundle.
• Docs — docs/safe-outputs.md, docs/ado-script.md, AGENTS.md, and the two site/**.mdx mirrors updated to note the step runs in both jobs.

Follow-up

The prepare step also runs in the Agent job for the host-side SafeOutputs MCP diff base; an external comment on #1453 reports that path can still fail on shallow/detached-HEAD checkouts. Tracked separately in #1457 (out of scope here).

Test plan

• cargo build — clean.
• cargo test — full suite, 0 failures (incl. the 10 create_pull_request compiler tests and the regenerated golden lock).
• cargo clippy --all-targets --all-features — clean.
• cargo test --test bash_lint_tests — shellcheck-clean (SafeOutputs job gained bash bundle steps).
• npm run test:smoke (in scripts/ado-script/) — 7/7, including the new prepare-pr-base.js [aw]: Follow-up to #1413 / #1425: create-pull-request worktree fails — the prepare-pr-base step runs in the Agent job, but the executor runs in the SafeOutputs job (standalone target) #1453 smoke test.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-tested fix with one minor inefficiency worth noting.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — redundant prepare step in split-approval scenarios

  build_safeoutputs_job checks front_matter.create_pr_config().is_some() to decide whether to emit the bundle-staging + prepare-step, but this function is called for both the SafeOutputs (automatic) and SafeOutputs_Reviewed (gated) variants in a split-approval config. When create-pull-request carries require-approval, only SafeOutputs_Reviewed will ever execute it; the SafeOutputs (auto) job gets the full Node install + curl/NuGet bundle download + prepare-pr-base step unnecessarily — and vice-versa when it's in the auto set.

  This isn't a correctness bug (ado-aw execute is still filtered by --only/--exclude, so no spurious PR gets opened), but it wastes wall-clock time in the job that won't use the ref. A tighter guard — e.g. checking whether create-pull-request is included in the current variant's filter set — would be cleaner. Low priority, since split-approval + create-pull-request is an uncommon combo.

✅ What Looks Good

• Refactoring quality: Extracting create_pr_prepare_repos (pure, no side effects) and warn_create_pr_target_inference (advisory, Agent-job only) into separate helpers is the right call. The "print exactly once" invariant is clear and correct.
• Tests: The two new compiler tests are precise — the ordering assertion (prepare_at < execute_at) is particularly valuable since order matters here.
• Smoke test: The hermetic real-git smoke test that asserts failure before and success after the bundle run is the ideal way to regression-test this class of bug. withSmokeScratchDir + spawnSync keeps it dependency-free.
• Comment fidelity: The inline comment in build_safeoutputs_job explaining why the fetch must be re-done per-job (isolated ADO checkout) is exactly the right level of detail for future readers.
• Bundle auth: Correctly uses System.AccessToken (the identity the checkout persists credentials for) for the git fetch — no write token needed or used.

Generated by Rust PR Reviewer for #1458 · 52.3 AIC · ⌖ 5.71 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the fix is correct, well-scoped, and fully tested.

Findings

✅ What Looks Good

• Correct root cause fix: Each ADO job has an isolated checkout, so re-fetching origin/<target> in the SafeOutputs job before ado-aw execute is exactly the right solution.
• Clean refactor: Extracting create_pr_prepare_repos (pure, no side effects) and warn_create_pr_target_inference (called once, Agent-job only to avoid duplicate warnings) is the right split.
• Split-approval scoping is correct: The create_pr_reviewed flag correctly routes runs_create_pull_request — auto variant gets it when the tool is not review-gated, reviewed variant gets it when it is. The default_single degenerate case (all-auto or all-reviewed collapsing to one job) uses create_pr_configured directly, which is correct since the single job runs every tool.
• No duplicate bundle staging: install_and_download_steps_typed is only added to the SafeOutputs job under the runs_create_pull_request guard; the ado-script extension doesn't normally add these steps to SafeOutputs.
• Smoke test: The hermetic prepare-pr-base.js smoke test is well-constructed — it reproduces the exact failure mode (step 3: worktree add on origin/develop fails before the fix) then verifies both the ref and the worktree add succeed after running the bundle. Good use of (redacted) URL to force a transport that honours --depth`.
• Test coverage: Four new compiler tests cover the basic case, multi-repo targets, PR-tool-gated split, and the mirror sibling-gated split.

⚠️ Suggestions

• CREATE_PULL_REQUEST_TOOL constant coupling (src/compile/agentic_pipeline.rs): The constant "create-pull-request" must stay in sync with the key format returned by partition_safe_outputs_by_approval. The doc comment calls this out, but a test-time assertion or a shared source-of-truth (e.g. a const from types.rs used in both places) would make silent rename drift impossible. Minor — not a current bug.

• Double checkout_repo_refs() call (src/compile/agentic_pipeline.rs): create_pr_prepare_repos and warn_create_pr_target_inference each call front_matter.checkout_repo_refs() independently, and both are invoked from build_agent_job in the same path. If the method is non-trivial (map construction), computing once and threading through would be cleaner. Negligible in practice.

Neither finding is a blocker. The fix is correct and the PR is ready to merge.

Generated by Rust PR Reviewer for #1458 · 42.8 AIC · ⌖ 5.78 AIC · ⊞ 6.2K · ◷