test: add deterministic Stage 3 executor safe-output E2E suite

[jamesadevine]

Summary

Adds a deterministic, no-LLM end-to-end test of the Stage 3 (ado-aw execute) safe-output executor.

The existing daily suite in tests/safe-outputs/ drives Stage 3 by having an LLM agent (Stage 1) emit each safe output, so it validates the full agentic flow but is inherently non-deterministic/flaky. This new suite removes the agent from the loop: for every ADO-write safe output it

1. sets up preconditions deterministically via the ADO REST API,
2. crafts the executor's safe_outputs.ndjson input directly (fixed literal values),
3. runs the real ado-aw execute binary (built from the checkout),
4. asserts the effect via the ADO REST API,
5. cleans up every object it created,

and, on any failure, files a deduped [executor-e2e-failure] GitHub issue on githubnext/ado-aw and fails the build.

What's included

• Harness — scripts/ado-script/src/executor-e2e/ (TypeScript): scenario contract, focused ADO REST client, ado-aw execute wrapper, runner, entry point, and direct GitHub issue filer.
• Scenarios — all deterministically-assertable ADO-write tools: work items (create/update/comment/link/attachment), wiki (create/update), PR (comment/reply/resolve-thread/review/update), git (create-branch/create-git-tag), build (add-build-tag/queue-build/upload-build-attachment/upload-pipeline-artifact), plus the flagship create-pull-request (clones agent-definitions, generates a real git diff patch + base_commit + patch_sha256, asserts the PR + pushed branch, then abandons + deletes).
• Graceful skips — scenarios needing optional infra (queue-build pipeline id, a wiki, a real current build) skip instead of failing.
• Pipeline — tests/executor-e2e/azure-pipelines.yml (daily + manual, builds HEAD, runs against msazuresphere/AgentPlayground) with a registration/handoff README.md.
• Packaging carve-out — the harness is test-only and excluded from the shipped ado-script.zip: executor-e2e added to NON_BUNDLE_DIRS, a build:executor-e2e script emitting to gitignored test-bin/, kept out of the main build chain.
• Docs — cross-references in docs/ado-script.md and tests/safe-outputs/README.md.

Test plan

• npm run typecheck — clean.
• npm test (vitest) — 477 pass, including 20 new offline unit tests (ndjson/source rendering, executed-record parsing, issue title/body/dedupe, runner skip/setup-failure handling, bundle-coverage carve-out).
• ado-aw execute --dry-run against a synthesized source + NDJSON — confirmed the source/config/NDJSON plumbing and executed-record output.
• npm run build:executor-e2e — bundle builds to test-bin/, git check-ignore confirms it's ignored, and no root ado-script/*.js is produced (so it never enters the release glob).
• Built bundle runs and fails cleanly on missing env; pipeline YAML parses.

Remaining (one-time manual ADO handoff, documented in tests/executor-e2e/README.md): register the pipeline in AgentPlayground, grant the build identity write access on agent-definitions, and set the EXECUTOR_E2E_GITHUB_TOKEN secret — after which the first live run exercises the scenarios end-to-end.

[github-actions]

🔍 Rust PR Review

Summary: Well-structured addition with good test hygiene — a few real issues worth fixing before merging.

Findings

🐛 Bugs / Logic Issues

• github-issue.ts:40-45 — SYSTEM_TEAMPROJECT not URL-encoded in buildUrl

  `${env.SYSTEM_COLLECTIONURI.replace(/\/+$/, "")}/${env.SYSTEM_TEAMPROJECT}/_build/results?buildId=...`

  Project names like "Agent Playground" produce a space-containing URL that's technically malformed. The buildUrl is only embedded in the GitHub issue body as display text, so it won't break issue filing, but the link will be dead. Fix: encodeURIComponent(env.SYSTEM_TEAMPROJECT).

• scenarios/create-pull-request.ts:135-143 — orphaned PR when assert fails before recording prId
state.prId is set inside assert. If assert throws before that assignment (e.g., the getPullRequest call fails after the executor already created the PR), cleanup skips abandonPullRequest. The PR stays open. Fix: include prId?: number in CreatePrState directly (not via cast), and populate it from record.result?.pull_request_id at the top of assert.

• execute-cli.ts:152-169 — spawnCollect has no timeout
  If the ado-aw execute binary hangs (deadlock, network wait), the test suite hangs indefinitely and only fails when the ADO pipeline's overall job timeout fires. A 5–10 minute AbortSignal-based timeout would give a meaningful error message and prevent runaway scenarios from blocking later ones.

🔒 Security Concerns

• scenarios/create-pull-request.ts:51-57 — auth token visible in process arguments

  const fullArgs = ["-c", `http.extraheader=Authorization: ${extraHeader}`, ...args];
  const child = spawn("git", fullArgs, { cwd });

  On Linux, process arguments are readable from /proc/<pid>/cmdline by any process running as the same user for the duration of the git command. In a single-tenant CI VM this is low risk, but it's a well-known leaky pattern. Prefer GIT_ASKPASS, a temporary .git/config written to a restricted-permission temp file (chmod 0600), or git's credential.helper mechanism.

⚠️ Suggestions

• ado-rest.ts:41-56 — readonly token declared after constructor
  TypeScript allows this, but every other field in the class is declared before the constructor. Moving readonly token: string up with the other declarations makes the class easier to read and makes it obvious the field is intentionally public.

• runner.ts:113 — Scenario<any>[] in runAll
  Minor: Scenario<unknown>[] would be slightly safer than Scenario<any>[] while still accepting heterogeneous scenario collections.

✅ What Looks Good

• log() function percent-encodes a leading # to block ##vso command smuggling — exactly the right defence.
• Issue deduplication uses an exact title match (i.title === title), not a prefix check — prevents over-merging unrelated failures.
• finally block in runScenario guarantees cleanup even when assert throws, including after setup-only scenarios.
• Bundle carve-out (NON_BUNDLE_DIRS + test-bin/ in .gitignore + separate build:executor-e2e script) cleanly proves the harness never enters the release glob.
• 20 offline unit tests with good coverage of skip handling, issue deduplication, body rendering, and the parse/render plumbing.

Generated by Rust PR Reviewer for #1353 · 66.3 AIC · ⌖ 6.86 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-engineered test harness with a clean scenario contract, correct cleanup semantics, and solid security hygiene. A few minor issues worth noting.

Findings

⚠️ Suggestions

• [create-pull-request.ts:57] Unused ctx parameter in git()

  async function git(ctx: ScenarioContext, args: string[], cwd: string, extraHeader: string): Promise<string>

  ctx is accepted but never used inside the function body — no ctx.log(...) or other context access. If the intent was to log git commands, the ctx.log call is missing. If not, the parameter should be dropped to avoid misleading readers.

• [runner.ts, ado-rest.ts] No timeout on the assert phase or ADO REST calls
  runExecute correctly guards the child process with EXECUTOR_E2E_EXECUTE_TIMEOUT_MS, but scenario.assert() (and all ADO REST calls inside it) has no timeout. A single hung ADO endpoint — e.g., a stalled wiki or work-item GET — would block the entire suite until the ADO job-level deadline (typically 60–120 min). Consider wrapping scenario.assert() in a timeout or adding an AbortSignal with a deadline to the fetch() calls in AdoRest.request().

• [ado-rest.ts:~322] getWikiPage bypasses the centralized request() helper
  The method manually constructs its auth header and calls fetch() directly to capture the ETag. This is understandable, but any future change to auth logic in request() (e.g., token refresh, header rotation) would silently miss this code path. A request() overload that returns the raw Response would keep auth centralized.

• [execute-cli.ts:~80] renderSourceMarkdown embeds tool name as an unquoted YAML key

  lines.push(`  ${opts.tool}: ${JSON.stringify(opts.config)}`);

  All current tool names are safe kebab-case strings, so this isn't a current bug. But the function silently emits broken YAML for any tool name containing : or a leading {. Worth adding a guard or quoting the key since the function is exported.

✅ What Looks Good

• ##vso injection prevention in index.ts:log() is correct — msg.replace(/^#/gm, "%23") covers multi-line messages.
• Token not exposed in argv — create-pull-request.ts correctly passes the ADO PAT via GIT_CONFIG_VALUE_0 / GIT_CONFIG_KEY_0 env vars rather than -c http.extraHeader=... on the git command line, avoiding /proc/<pid>/cmdline exposure.
• SkipError pattern is elegant — distinguishes missing-precondition (skip) from real failures cleanly at the runner level.
• Cleanup semantics — the setupDone guard in runner.ts correctly skips cleanup when setup throws (state not reliable), and the finally block ensures cleanup always runs on any post-setup path.
• GitHub issue dedup — findOpenIssueByTitle with exact title match keyed on the sorted failing-tool set is a sound strategy for a daily pipeline.
• Bundle carve-out — executor-e2e in NON_BUNDLE_DIRS, gitignored test-bin/, and a dedicated build:executor-e2e script kept out of the main build chain is the right approach to prevent test code from shipping in ado-script.zip.

Generated by Rust PR Reviewer for #1353 · 75.4 AIC · ⌖ 6.49 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Overall solid work — the harness design is clean and well-structured. A few issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• runner.ts:73 — Cleanup silently skipped when setup() returns undefined

  if (setupDone && state !== undefined) {
    await scenario.cleanup(ctx, state);
  }

  The state !== undefined guard means any future scenario where setup() returns void/undefined will silently skip cleanup even when setup succeeded. All current scenarios return a non-empty object so no immediate breakage, but this is a footgun for future authors. Consider: if (setupDone) (removing the undefined check) or documenting that State = void is unsupported.

• ado-rest.ts:103–105 — Non-JSON response silently cast to T

  if (ct.includes("application/json")) return JSON.parse(text) as T;
  return text as unknown as T;  // ← silent cast

  If ADO returns XML or an HTML error page (non-JSON content-type), text is returned as T. Callers that access .pullRequestId etc. will get undefined with no diagnostic. Should probably throw new Error(\unexpected content-type '${ct}': ${text.slice(0, 200)}`)` for the non-JSON path.

⚠️ Suggestions

• create-pull-request.ts:46 — runGit has no timeout

  spawnCollect in execute-cli.ts has a configurable EXECUTOR_E2E_EXECUTE_TIMEOUT_MS (default 10 min) and kills with SIGKILL on expiry. runGit has no equivalent. A hung git clone on a slow/unavailable ADO instance would stall the entire scenario indefinitely rather than timing out cleanly. Consider a similar GIT_TERMINAL_PROMPT=0 + AbortSignal.timeout(...) pattern.

• create-pull-request.ts:143–145 — state.prId mutated inside assert

  // Record the PR id up front so cleanup abandons it even if a later
  // assertion ... throws.
  state.prId = prId;

  The intent is clear and the comment explains the semantics. But mutating setup state from within assert is unconventional and could surprise future authors of the Scenario<S> contract. A lighter-weight alternative: return an optional cleanupExtra object from assert that the runner merges into cleanup context (breaking the interface). Or, accept this pattern and document it in the Scenario<State> interface comment in scenario.ts.

• create-pull-request.ts — sourcesDir is not cleaned up

  The cloned checkout in sourcesDir (ctx.workDir/create-pull-request/src-checkout/) is created in setup() but not removed in cleanup(). For ephemeral CI agents this is fine, but on a developer workstation with EXECUTOR_E2E_EXECUTE_TIMEOUT_MS set low for iteration, it will accumulate large git checkouts. A rm -rf sourcesDir in cleanup() would be consistent with the rest of the teardown.

• azure-pipelines.yml:53 — curl | sh for Rust toolchain

  curl -fsSL https://sh.rustup.rs | sh -s -- -y --profile minimal

  This is the standard rustup installation pattern, but piping an external URL directly to sh in a pipeline is a supply chain risk. The 1ES agent pool may already have Rust installed (the if ! command -v cargo guard handles that). If this pipeline is registered on a fully-managed 1ES pool, consider pinning a hash or using a RustInstaller@x task if one is available in the organisation.

✅ What Looks Good

• ##vso injection prevention in index.ts:log() — msg.replace(/^#/gm, "%23") is exactly the right defence; good to see it applied proactively in a test harness.
• Git auth via GIT_CONFIG_* env vars in create-pull-request.ts rather than -c http.extraheader on the command line — token never appears in /proc/<pid>/cmdline. Excellent.
• Issue deduplication logic in github-issue.ts (title keyed on sorted failing-tool set + === exact match after in:title search) — correct and spam-safe.
• NON_BUNDLE_DIRS carve-out in bundle-coverage.test.ts ensures the harness never accidentally ends up in the release ado-script.zip — belt-and-suspenders with the build:executor-e2e output going to test-bin/ and that dir being .gitignored.
• Graceful skip pattern (SkipError) cleanly separates "missing optional precondition" from "real failure" — good UX for a suite that runs against a shared environment where not all infrastructure may be configured.

Generated by Rust PR Reviewer for #1353 · 85.1 AIC · ⌖ 7.56 AIC · ⊞ 6.2K · ◷