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 · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured addition — clean design, good security hygiene, solid unit test coverage. One correctness gap in the runner's error-containment logic is worth fixing; a dead-code fallback in the git scenario setup is worth noting.

---

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/executor-e2e/runner.ts:67–72 — The calls to scenario.config(), scenario.ndjson(), scenario.files(), and scenario.env() are not wrapped in error handling. If any of these throws (e.g., a harness bug or an unexpected null dereference), the outer try...finally propagates the exception out of runScenario and runAll terminates early, skipping all subsequent scenarios. The try/catch on L79 only guards runExecute, not the auxiliary methods immediately above it. The fix is straightforward — wrap those four calls in a try { ... } catch (err) { return finish({ ok: false, phase: "execute", message: errMessage(err) }); } block so a harness-level bug records a failed result and lets the rest of the suite continue. The unit tests in runner.test.ts only exercise setup failures, so this path has no coverage today.

⚠️ Suggestions

• scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts:158–161 — git symbolic-ref --short refs/remotes/origin/HEAD exits non-zero (not empty) when the remote HEAD symref isn't configured (common on fresh HTTPS clones that haven't run git remote set-head). The || "main" fallback is dead code in that case: the git() wrapper throws before it is reached. Consider git remote set-head origin --auto before this call, or wrap it in a try-catch that falls back to "main".

• scripts/ado-script/src/executor-e2e/github-issue.ts:buildIssueTitle — Title truncation at 200 chars is deterministic for a fixed failing-tool set, so dedupe continues to work across re-runs. However, the truncated and un-truncated titles are different strings — a subsequent run that fixes one failing tool could produce the same truncated title as the original, silently deduplicating onto the wrong issue. Unlikely in practice (there aren't that many tools), but worth a comment explaining the invariant.

✅ What Looks Good

• ##vso injection guard in index.ts:log() — percent-encoding leading # on every line before writing to stdout is exactly right for ADO pipeline log contexts.
• Auth token isolation in runGit() — injecting the Authorization header via GIT_CONFIG_KEY_0/VALUE_0 env vars (not argv) keeps the token out of /proc/<pid>/cmdline. Well-done.
• state.prId set before fallible assertions in create-pull-request.ts:assert() — the comment explains the pattern clearly; cleanup correctly abandons the PR even if the getPullRequest call throws.
• SkipError pattern — clean separation between "optional precondition missing" (not a build failure) and genuine scenario failures.
• Packaging carve-out — test-bin/ is gitignored, executor-e2e is in NON_BUNDLE_DIRS, and the guard test is updated. The harness can never accidentally enter ado-script.zip.
• GitHub issue deduplication — exact-match on sorted failing-tool set prevents spam while remaining stable across runs.

Generated by Rust PR Reviewer for #1353 · 75.5 AIC · ⌖ 6.22 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid test infrastructure — good security hygiene, clean architecture. A few items worth fixing before the first live run.

Findings

🐛 Bugs / Logic Issues

• [scenarios/pr.ts:130-134] resolvePrThread assertion is fragile against ADO status enums.
  getThread declares status?: string but ADO's API returns numeric status codes (1=active, 2=fixed, 3=wontfix, 4=closed). The assertion only guards against "active" and "1", so if ADO ever returns an unexpected integer (e.g. 4 for "closed" from an infra race), String(4) = "4" would pass the check and the test would report a false success. The type should be status?: number, and the assertion should be an explicit allowlist (status === 2 || status === "2" || status === "fixed") rather than a denylist of the initial state.

• [scenarios/pr.ts:14 + scenarios/git.ts:13] defaultBranchShortName is copy-pasted verbatim across two files.
  These two functions are byte-for-byte identical. Extract to common.ts — as-is, a future fix to one won't reach the other.

⚠️ Suggestions

• [execute-cli.ts:41-42] renderSourceMarkdown uses bare template literals for two YAML string values but JSON.stringify for the key.

  lines.push(`name: "executor-e2e: ${opts.tool}"`);
  lines.push(`description: "Deterministic Stage 3 executor check for ${opts.tool}"`);

  The YAML key is correctly JSON.stringify'd, but the string values are not. A tool name containing " or \ would produce malformed front-matter. Use the same quoting strategy:

  lines.push(`name: ${JSON.stringify(`executor-e2e: ${opts.tool}`)}`);
  lines.push(`description: ${JSON.stringify(`Deterministic Stage 3 executor check for ${opts.tool}`)}`);

• [all scenarios/*.ts] Scenario collection arrays typed Scenario<any>[] should be Scenario<unknown>[].
  unknown preserves the "heterogeneous collection" semantics without suppressing type errors the way any does. The runAll/runScenario signatures already accept Scenario<unknown>[].

✅ What Looks Good

• ##vso injection guard in index.ts:log() — /^#/gm replace ensures scenario output can't smuggle ##vso[task.setvariable ...] into the pipeline log.
• Git auth via GIT_CONFIG_* env vars (not -c on argv, not embedded in the clone URL) — token never appears in /proc/<pid>/cmdline.
• AbortSignal.timeout(this.timeoutMs) on every fetch call — no hung endpoints can block the suite indefinitely.
• state.prId = prId before any fallible ADO call in createPullRequest.assert — ensures cleanup can abandon the PR even if getPullRequest throws. Good defensive pattern.
• SkipError + setupDone guard — clean separation between "precondition missing" (skip) and "setup crashed" (fail), with cleanup running unconditionally for successfully-set-up scenarios.
• Token not exposed in error messages in both AdoRest.request and github-issue.ts.

Generated by Rust PR Reviewer for #1353 · 99.2 AIC · ⌖ 6.62 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured TypeScript E2E harness with solid security posture. One genuine bug and one minor consistency issue.

---

Findings

🐛 Bugs / Logic Issues

• index.ts:1280 — workDir is never cleaned up. mkdtemp(join(tmpdir(), "ado-aw-e2e-")) creates a scratch directory but main() never removes it. Every CI run and every local invocation leaves a ado-aw-e2e-* directory behind. The individual scenarios clean up their own scenarioDir children (and create-pull-request cleans its sourcesDir), but the parent dir persists. Suggest wrapping the run body in a try/finally with await rm(workDir, { recursive: true, force: true }).

⚠️ Suggestions

• execute-cli.ts:859 — adoRepo is not quoted in the generated YAML. Every other string emitted by renderSourceMarkdown goes through JSON.stringify (the tool name, the config value), but adoRepo is interpolated raw:

  lines.push(`  - ${opts.adoRepo}=${opts.adoRepo}`);

  ADO repo names can't contain characters that would break YAML scalars in practice, but it's inconsistent and would silently produce invalid front matter for a name containing : , a leading {, or a trailing #comment. Suggest wrapping the alias and name in JSON.stringify to match the rest of the function.

---

✅ What Looks Good

• Auth never leaks into process argv. SYSTEM_ACCESSTOKEN is passed via the env map to spawnCollect; the git auth header is injected via GIT_CONFIG_KEY_0 / GIT_CONFIG_VALUE_0 (not -c on the command line), so the token never appears in /proc/<pid>/cmdline. Clear and correct.
• ##vso log sanitization is correct. The gm flag on /^#/ in log() replaces the leading # on every line (including multi-line executor stdout/stderr piped through opts.log), turning ##vso[...] into %23#vso[...] which ADO ignores.
• spawn with args array throughout. All ado-aw execute and git invocations use spawn(cmd, args, ...) rather than exec with interpolated strings — no shell injection surface.
• AbortSignal.timeout + setTimeout/SIGKILL layering. The REST client uses AbortSignal.timeout for a per-request timeout, while spawnCollect and runGit use setTimeout + child.kill("SIGKILL") so a hung subprocess can never stall the whole suite.
• Bundle exclusion guard. Adding "executor-e2e" to NON_BUNDLE_DIRS in bundle-coverage.test.ts closes the loop — the guard will fail CI if someone accidentally adds a build:executor-e2e-style entry to the main build chain that would slip the harness into ado-script.zip.
• SkipError vs failure distinction is clean and well-tested; the runner correctly skips cleanup only for SkipError/setup-failure paths (guarded by setupDone), not for normal teardown.

Generated by Rust PR Reviewer for #1353 · 86 AIC · ⌖ 6.59 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured test harness with one real bug in a utility function and a minor test gap worth addressing.

---

Findings

🐛 Bugs / Logic Issues

• execute-cli.ts:877 — renderNdjsonLine spread order is wrong

  return JSON.stringify({ name: tool, ...entry }) + "\n";

  In JavaScript, later spread properties win, so if any scenario's ndjson() method accidentally includes a name key in its returned object, it silently overrides the injected tool name. The produced NDJSON record would have the wrong name field; parseExecutedRecords would then fail to match it against the expected snake-cased tool name and the scenario would report "no executed record" even though the executor succeeded.

  Should be:

  return JSON.stringify({ ...entry, name: tool }) + "\n";

  The Scenario contract documents "WITHOUT the name field", but that's an implicit invariant with no enforcement — a new scenario author can violate it silently. Reversing the spread makes the injected tool name canonical.

⚠️ Suggestions

• runner.test.ts — "records a setup failure without reaching execute or cleanup" — the test name claims to verify cleanup is skipped, but the guardScenario cleanup body only throws when executed === true, which is always false after a setup failure. If cleanup ran anyway, the test would still pass. The invariant (setupDone gate) is correct in the runner; the test just doesn't cover it. Either add a let cleanupCalled = false flag and assert it remains false, or at minimum rename the test.

✅ What Looks Good

• Git token never touches argv — credentials injected via GIT_CONFIG_* env vars in runGit, not -c http.extraheader=..., with a clear comment explaining why. Solid operational security for the test harness.
• ##vso injection guard — the /^#/gm replace in log() converts every line-initial # to %23, breaking ##vso[...] at the start of any line including mid-message newlines from multi-line error strings.
• YAML injection prevention — renderSourceMarkdown uses JSON.stringify for the tool key, config value, name, and description throughout; valid YAML that closes injection vectors through unusual tool names or descriptions.
• SkipError isolation — the setupDone flag ensures cleanup never runs after a failed/skipped setup, and a skip or failure in one scenario never aborts the rest of the suite.
• Per-request timeouts — both AdoRest.authedFetch (AbortSignal.timeout) and spawnCollect (setTimeout + SIGKILL) are bounded; a hung ADO endpoint or ado-aw execute process can't deadlock the whole suite.

Generated by Rust PR Reviewer for #1353 · 73.4 AIC · ⌖ 6.35 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured test harness — the core architecture is sound and security-conscious. Three issues worth addressing, one of which is a real resource-leak bug.

Findings

🐛 Bugs / Logic Issues

• scenarios/pr.ts — Dangling ADO objects on partial setupPr() failure (setupPr, lines ~35–56): If createThread() throws after pushAddFileBranch() and createPullRequest() have already succeeded, the runner sees a thrown setup() and leaves setupDone = false, so cleanup (which calls abandonPullRequest + deleteRef) never runs. Both the branch and the PR leak in ADO. This affects replyToPrComment and resolvePrThread which both pass withThread = true. Repeated failure bursts (e.g. a flaky ADO REST endpoint) would accumulate dangling objects. Fix: split setupPr into separate branch/PR/thread steps so partial failures only leak objects that are already uniquely prefixed, or guard at least the PR+branch cleanup inside setupPr itself with a try/catch that abandons on createThread failure.

• scenarios/create-pull-request.ts — state.targetBranch is computed but never read (state type CreatePrState): The field is populated from symbolic-ref refs/remotes/origin/HEAD and the "main" fallback, but neither ndjson() nor cleanup() references it — the executor config hardcodes "target-branch": "main". This is dead state. Remove it to avoid the impression that the PR will correctly target whatever the actual default branch is; if agent-definitions ever has a non-main default, the scenario silently submits the PR to a non-existent branch without touching this variable.

⚠️ Suggestions

• runner.test.ts — "cleanup not called" guard is never enforced (guardScenario cleanup): The cleanup function throws "cleanup should not run after setup failure" only when executed === true, but the runner wraps cleanup in try/catch and swallows the error (logged as a warning). If the guard ever fired, it would be invisible to the test assertion. The test only validates the returned ScenarioResult shape (ok=false, phase="setup"), which is correct — but the cleanup guard creates a false sense of invariant enforcement. Either verify the test indirectly (e.g. expect(executed).toBe(false) after runScenario returns) or remove the misleading guard.

• ado-rest.ts — AdoRest.token public field is unused (line ~46, readonly token: string): Exposed with the comment "for callers that must clone via git over HTTPS", but create-pull-request.ts instead uses ctx.token directly. The field could be private or removed to avoid surprising readers into thinking rest.token is the canonical way to retrieve the ADO write token.

✅ What Looks Good

• Token injection via GIT_CONFIG_* env vars in create-pull-request.ts — token never appears in argv//proc/<pid>/cmdline. Correct approach.
• ##vso injection guard in index.ts log() — replace(/^#/gm, "%23") correctly handles multi-line log messages including newline-injected commands.
• Issue deduplication in github-issue.ts — findOpenIssueByTitle uses in:title "quoted phrase" + client-side exact match to avoid both false positives and per-API-page truncation issues.
• SkipError pattern and setupDone guard in runner.ts — scenario isolation is clean; a SkipError or setup failure in one scenario cannot abort later scenarios.
• Shipping carve-out (NON_BUNDLE_DIRS, build:executor-e2e → test-bin/) — the harness correctly stays out of ado-script.zip.

Generated by Rust PR Reviewer for #1353 · 78.8 AIC · ⌖ 7.17 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: All previously-flagged bugs have been addressed — the harness is now in good shape. One genuine deduplication gap remains, plus a minor defensive-coding suggestion.

---

Findings

🐛 Bugs / Logic Issues

• github-issue.ts:122 — per_page=20 is too small for the deduplication search

  const url = `https://api.github.com/search/issues?q=${encodeURIComponent(q)}&per_page=20`;

  findOpenIssueByTitle pages at 20 results and does not follow pagination. GitHub search does partial-phrase matching, so the query for a title like [executor-e2e-failure] add-pr-comment, update-pr could return many open issues that contain those words. If more than 20 exist (possible after weeks of recurring failures), the exact-match .find() on page 1 misses the existing issue and a duplicate is filed. Fix: increase to per_page=100 — the search is already scoped to in:title + repo: + is:open, so 100 is sufficient for the expected scale.

⚠️ Suggestions

• scenarios/pr.ts:112,129 — state.threadId! non-null assertions give confusing errors

  replyToPrComment and resolvePrThread both call ctx.rest.getThread(state.repo, state.prId, state.threadId!). The ! suppresses TypeScript's null check, but at runtime a missing threadId would silently pass undefined to getThread, producing a confusing ADO 404 rather than a clear invariant-violation message. Since both scenarios call setupPr(..., true) which always sets threadId, the risk is low, but a cheap explicit guard is cleaner:

  if (state.threadId === undefined) throw new Error(`[${tool}] threadId not set by setup`);

✅ What Looks Good

• All prior findings addressed: SYSTEM_TEAMPROJECT is now encodeURIComponent-encoded in buildUrl; renderNdjsonLine correctly does { ...entry, name: tool } so the tool name always wins; config/ndjson/files/env calls are wrapped in a shared try/catch so harness errors can't abort runAll early; resolvePrThread uses an allowlist Set covering both numeric (ADO enum) and string status forms; workDir is cleaned up in a try/finally in main(); and the setupPr partial-failure path now properly tears down the branch + PR when createThread throws.
• Token injection via GIT_CONFIG_* in create-pull-request.ts keeps the PAT out of argv//proc/<pid>/cmdline.
• ##vso injection guard in index.ts log() with replace(/^#/gm, "%23") is correct and well-placed.
• state.targetBranch is live — the config correctly passes it as "target-branch": state.targetBranch, so the scenario targets the repo's actual default branch rather than hardcoding "main".

Generated by Rust PR Reviewer for #1353 · 99.2 AIC · ⌖ 6.62 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured deterministic test harness with one concrete test-logic bug and a few minor concerns worth addressing.

Findings

🐛 Bugs / Logic Issues

• scenarios/pr.ts:resolvePrThread.assert — assertion too permissive, would miss executor regressions

  The scenario sends status: "fixed" but the assert accepts any non-active resolved state:

  const resolved = new Set(["2", "3", "4", "5", "fixed", "wontfix", "closed", "bydesign"]);

  If the executor resolves the thread to wontFix/closed/byDesign due to a mapping bug, the test passes anyway. The comment even says "we requested 'fixed'" but then contradicts it by accepting statuses 3–5. The set should be tightened to only the "fixed" equivalents:

  const resolved = new Set(["2", "fixed"]);
  if (!resolved.has(status)) {
    throw new Error(`thread #${state.threadId} not resolved to 'fixed' (got '${status}')`);
  }

• scenarios/work-item.ts:createWorkItem.assert — violates its own Scenario contract

  The documented Scenario.assert contract says "populate fields on state ... before any fallible check so cleanup can still tear the object down if a later assertion throws." But numResult() — which throws on type mismatch — runs before state.createdId = id:

  assert: async (ctx, state, record) => {
    const id = numResult(record, "id");   // ← can throw
    state.createdId = id;                 // ← never reached on throw

  If the executor reports success but returns a non-numeric id, the work item leaks and cleanup is a no-op. The fix is to extract the raw value, populate state first, then validate:

  const id = record.result?.["id"];
  state.createdId = typeof id === "number" ? id : undefined;
  if (!Number.isFinite(id)) throw new Error(`executor result.id is not a number (got ${JSON.stringify(id)})`);

⚠️ Suggestions

• index.ts:main error handler — logs only message, loses stack trace

  (err: unknown) => {
    log(`executor-e2e crashed: ${(err as Error).message}`);

  An unexpected crash (e.g. unhandled promise rejection) will only surface the message string. Add the stack for debuggability:

  log(`executor-e2e crashed: ${(err as Error).stack ?? (err as Error).message}`);

• ado-rest.ts:putWikiPage — ETag limitation undocumented

  putWikiPage does an unconditional PUT with no ETag. ADO wiki requires an ETag for updates to existing pages (returns 412 otherwise). The current caller (updateWikiPage.setup) always creates a new page so this happens to work, but a future caller updating an existing page would get a silent 412 error (currently surfaced as a confusing generic HTTP error). A short comment noting this would prevent misuse.

✅ What Looks Good

• Packaging carve-out is airtight: NON_BUNDLE_DIRS, build:executor-e2e → test-bin/, .gitignore, and the doc cross-reference all agree. The harness can never accidentally ship in ado-script.zip.
• pr.ts:setupPr correctly handles createThread failures — abandons the PR and deletes the branch before rethrowing, preventing dangling ADO objects even on flaky thread creation.
• VSO command injection prevention in log() — msg.replace(/^#/gm, "%23") correctly sanitises ##vso[ commands that could be smuggled into log output from scenario messages.
• Subprocess timeouts are thorough — spawnCollect, runGit, and AbortSignal.timeout in authedFetch all have explicit timeouts with env-var overrides; a hung subprocess can never block the whole suite.
• Deduplication logic in github-issue.ts — exact title match with a 100-item page handles partial-phrase GitHub search correctly without spamming new issues.
• runner.ts failure isolation is clean: SkipError vs generic setup failure is properly distinguished, cleanup always runs after a successful setup, and a failure in one scenario never aborts the rest.

Generated by Rust PR Reviewer for #1353 · 76.5 AIC · ⌖ 8.56 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-architected test harness with correct security practices; two small issues worth addressing.

Findings

🐛 Bugs / Logic Issues

• runner.ts:1385 — mkdir(scenarioDir, { recursive: true }) is called outside the try/catch that records failures, so a filesystem error (full disk, permission issue) propagates out of runScenario and causes runAll's for loop to throw, leaving all subsequent scenarios unrun with no ScenarioResult recorded. The mkdir call should move inside the try block, or runAll should catch per-iteration.

  // runner.ts - runScenario
  const scenarioDir = join(ctx.workDir, tool);
  await mkdir(scenarioDir, { recursive: true });   // <-- can throw and abort runAll
  
  let state: S | undefined;
  let setupDone = false;
  // ...
  try {
    ctx.log(`[${tool}] setup`);
    try {
      state = await scenario.setup(ctx);

• github-issue.ts — findOpenIssueByTitle fetches per_page=100 but doesn't paginate. The comment says this "comfortably covers the expected scale", which is true for now, but if the repo accumulates >100 open [executor-e2e-failure] issues (e.g., sustained incident), the .find() would miss an existing match and file a duplicate. Not a blocker, but worth noting.

⚠️ Suggestions

• runner.ts:1414 — let config, entry, files, extraEnv; without type annotations relies on implicit any during declaration. If the TypeScript config has noImplicitAny, this is fine after assignment, but the intent would be clearer (and safer against accidental capture across branches) as const assignments inside the try block.

• execute-cli.ts:renderNdjsonLine comment — "a scenario that accidentally returns a name key can't override it" reads backwards; the injected name: tool overrides the scenario's key (not vice versa). Minor, but slightly confusing.

• azure-pipelines.yml:2722 — The Rust install falls back to curl -fsSL https://sh.rustup.rs | sh. This is correct for an agent pool that doesn't pre-install Rust, but since always: true schedules run daily, it's worth verifying the pool pre-installs cargo to avoid the latency/network dependency every run.

✅ What Looks Good

• Auth security is correct throughout: ADO token passes via SYSTEM_ACCESSTOKEN env, git auth via GIT_CONFIG_* env vars (not --config argv), GIT_TERMINAL_PROMPT=0 prevents silent hangs on auth failures — all correct.
• YAML injection in renderSourceMarkdown is properly guarded with JSON.stringify() on all user-controlled values (tool name, repo alias, config).
• SkipError vs Error separation is well-designed — infra absence skips cleanly, actual assertion failures go red.
• State-before-assertion pattern (state.prId = prId and state.createdId = ... before the fallible checks) ensures cleanup runs even when assertions fail — good discipline.
• NON_BUNDLE_DIRS carve-out + test-bin/ gitignore correctly prevents the harness from shipping in the release ado-script.zip glob.
• Sequential execution + isolated per-scenario scratch dirs is the right design for a deterministic ADO-write suite.

Generated by Rust PR Reviewer for #1353 · 82.2 AIC · ⌖ 6.58 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-architected test harness with solid security practices and comprehensive scenario coverage. One unresolved bug from the previous review (the mkdir outside the try block in runner.ts) and one new defensive-coding gap in the PR thread assertion.

---

Findings

🐛 Bugs / Logic Issues

• runner.ts:runScenario — mkdir outside the failure-isolation try block (unresolved from previous review)

  const scenarioDir = join(ctx.workDir, tool);
  await mkdir(scenarioDir, { recursive: true });  // ← throws propagate out of runScenario
  
  let state: S | undefined;
  let setupDone = false;
  // ...
  try {
    ctx.log(`[${tool}] setup`);

  If mkdir throws (disk full, permission error), the exception propagates out of runScenario uncaught, causing runAll's for loop to throw. The try...finally in main() has no catch, so fileFailureIssue is never called and no ScenarioResult is recorded for any remaining scenarios. The fix is to move the mkdir inside the outer try block (or add a per-iteration catch in runAll):

  try {
    await mkdir(scenarioDir, { recursive: true });
    ctx.log(`[${tool}] setup`);
    // ...

• scenarios/pr.ts:replyToPrComment.assert — null-content guard missing

  const replied = (thread.comments ?? []).some((c) => c.content.includes(`build ${ctx.buildId}`));

  getThread types comments as { id: number; content: string }[], but the ADO API can return system-generated comments (e.g., vote-change notices) where content is null. The as T cast in request() doesn't validate at runtime, so null.includes(...) would throw a TypeError and fail the assert with a confusing message rather than a clean "reply not found". Guard with optional chaining:

  const replied = (thread.comments ?? []).some((c) => (c.content ?? "").includes(`build ${ctx.buildId}`));

✅ What Looks Good

• Previous findings addressed: resolvePrThread.assert correctly tightened to Set(["2", "fixed"]). createWorkItem.assert correctly populates state.createdId before the fallible type check. putWikiPage has a clear ETag-limitation comment. Crash handler logs e.stack ?? e.message. setupPr correctly tears down the PR+branch before rethrowing on a createThread failure.
• Field name correctness: upload-build-attachment (artifact_name, file_path) and upload-workitem-attachment (work_item_id, file_path, comment) match the corresponding Rust Params structs exactly.
• Auth hygiene: ADO token via SYSTEM_ACCESSTOKEN env, git auth via GIT_CONFIG_* env (never in argv), GIT_TERMINAL_PROMPT=0 prevents interactive hangs.
• VSO injection prevention: log() encodes leading # per line with /^#/gm — correctly catches ##vso[task.setVariable] commands smuggled from child-process output.
• Packaging carve-out: NON_BUNDLE_DIRS + build:executor-e2e → test-bin/ + .gitignore all agree; harness cannot accidentally ship in ado-script.zip.

Generated by Rust PR Reviewer for #1353 · 126.8 AIC · ⌖ 6.27 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured deterministic test harness with sound error handling and correct security practices. Two minor issues worth flagging.

---

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/executor-e2e/index.ts (crash handler): The top-level crash handler casts the rejection to Error and accesses .message:

  (err: unknown) => {
    const e = err as Error;
    log(`executor-e2e crashed: ${e.stack ?? e.message}`);

  If something throws a non-Error (a plain string, null, etc.), e.message is undefined, and the log emits executor-e2e crashed: undefined. Replace with String(err) or the errMessage helper that already exists in runner.ts to get a meaningful log line for any reject type.

• execute-cli.ts / create-pull-request.ts — spawnCollect / runGit timeout path: When the setTimeout fires, child.kill("SIGKILL") is called. The promise only settles inside the close event handler (which fires after the process is actually killed). If the kill signal is silently dropped (edge case on Linux, extremely unlikely with SIGKILL), the close event never fires and the Promise never settles — blocking the scenario runner indefinitely. The ADO pipeline's own job timeout is the last line of defence. Not a practical issue, but worth a comment acknowledging the dependency.

⚠️ Suggestions

• scripts/ado-script/src/executor-e2e/runner.ts — untyped intermediate declarations: The let config, entry, files, extraEnv; block relies on TypeScript's control-flow analysis to infer that these are definitely assigned after the catch-that-returns. This is correct (typecheck passes), but explicit type annotations would make the intent clearer and prevent surprises if the control-flow shape changes. e.g.:

  let config: Record<string, unknown>;
  let entry: Record<string, unknown>;
  let files: Record<string, string> | undefined;
  let extraEnv: Record<string, string> | undefined;

✅ What Looks Good

• ##vso injection guard: The log() function in index.ts percent-encodes a leading # before writing to stdout (/^#/gm → %23). This correctly prevents any ADO REST response or scenario output from smuggling a ##vso[task.setvariable] command into the pipeline log. Well done.

• Token never in process argv: create-pull-request.ts's runGit passes the ADO auth header via GIT_CONFIG_KEY_0 / GIT_CONFIG_VALUE_0 environment variables (the GIT_CONFIG_* protocol). The args array that gets logged contains only ["clone", url, dest] — not the credential. This is the correct approach and worth preserving.

• renderNdjsonLine canonical name: Spreading entry first and then explicitly setting name: tool ensures a scenario that accidentally returns a name key in its ndjson() method can never silently shadow the canonical tool name consumed by the executor and the assert phase. Good defensive design.

• SkipError vs Error: The SkipError sentinel cleanly separates "precondition unavailable" from "scenario failed", and the runner correctly records skips as ok: true / skipped: true without calling cleanup. The setupPr function also proactively tears down the PR/branch if createThread throws after they're already created, preventing object leaks when setupDone would be false. Both patterns are thoughtful.

• Cleanup ordering in assertions: Multiple scenarios (notably create-work-item, queue-build) populate state.createdId / state.queuedBuildId before the fallible numeric check, so cleanup can still delete the object even if a subsequent assertion throws. This pattern matches the documented contract in scenario.ts and is consistently applied.

Generated by Rust PR Reviewer for #1353 · 94.1 AIC · ⌖ 6.38 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-engineered test harness with one real bug in the wiki scenario setup and a couple of minor issues worth tightening.

---

Findings

🐛 Bugs / Logic Issues

• scenarios/wiki.ts:updateWikiPage.setup — putWikiPage is documented as only working for creating a new page (no If-Match ETag, returns HTTP 412 if the page already exists). If a previous run fails after creating the page but before cleanup, the next run will throw a 412 in setup and the scenario goes red — even when the executor is perfectly healthy. Fix: either deleteWikiPage first (it already uses allow404: true, so it's idempotent), or read the ETag first and use a conditional PUT.

  // updateWikiPage.setup — add before putWikiPage:
  await ctx.rest.deleteWikiPage(wiki, path);   // idempotent via allow404
  await ctx.rest.putWikiPage(wiki, path, "original deterministic content");

• scenarios/pr.ts:replyToPrComment.ndjson / resolvePrThread.ndjson — Both pass thread_id: state.threadId where threadId is typed number | undefined on PrState. JSON.stringify silently drops undefined values, so if the guard in assert is the only check, a broken setup that somehow returns without setting threadId would send the executor a missing thread_id field. The withThread: true path in setupPr ensures it's set or throws, so this doesn't manifest today — but the type contract is leaky. Adding a non-null assertion or a guard in ndjson would make the invariant explicit:

  ndjson: async (ctx, state) => ({
    thread_id: state.threadId!,  // or: if (!state.threadId) throw new Error(...)
    ...
  })

⚠️ Suggestions

• execute-cli.ts:runExecute file staging — join(safeOutputDir, rel) where rel comes from scenario.files() has no traversal guard. All current scenario authors are trusted, but a future scenario using "../../../etc/cron.d/evil" as a key would write outside the scratch dir. Since this is test-only code, the risk is low, but a one-liner if (resolve(target) !== resolve(join(safeOutputDir, basename(rel)))) throw ... (or a simple rel.includes('..') check) would be a cheap safety rail.

• github-issue.ts:renderIssueBody (line ~1076) — The env parameter is typed IssueEnv but the function also accepts an object with extra fields project and buildUrl from a call site in fileFailureIssue. This is fine today (TypeScript structural typing), but the in-function documentation says "env: IssueEnv" while the full IssueEnv interface includes those fields anyway — no change needed, just confirming the types align. ✓

✅ What Looks Good

• Auth handling in create-pull-request.ts — git credentials are passed via GIT_CONFIG_VALUE_0 env var rather than argv, so the token never appears in /proc/<pid>/cmdline or process logs. Explicit comment too. Well done.
• ##vso injection prevention — log() in index.ts replaces a leading # with %23 on every line, correctly defanging ##vso[task.setvariable ...] payloads that could otherwise leak env vars into the pipeline log.
• Skip vs. fail separation — SkipError and the setupDone guard interact cleanly; the runner tests verify both cases. Scenarios that need optional infra never inflate the failure count.
• Cleanup-on-partial-setup in pr.ts — setupPr explicitly tears down the PR + branch when createThread throws, rather than leaving orphaned objects. The runner's own setupDone guard also correctly prevents cleanup when setup never started.
• GitHub issue deduplication — sorted, deduped tool set as the title key, exact-match JS filter after a 100-item page-at-once search — covers the expected scale with good reasoning documented in comments.
• Bundle exclusion — NON_BUNDLE_DIRS carve-out, build:executor-e2e emitting to the non-root test-bin/, and test-bin in .gitignore together make it structurally impossible to accidentally ship the harness.

Generated by Rust PR Reviewer for #1353 · 83.2 AIC · ⌖ 7.03 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured test harness with strong isolation, cleanup discipline, and good security hygiene. A few targeted findings below.

---

Findings

⚠️ Suggestions

• create-pull-request.ts:55–62 — GIT_CONFIG_COUNT: "1" unconditionally clobbers pre-existing GIT_CONFIG_* entries

  GIT_CONFIG_COUNT: "1",
  GIT_CONFIG_KEY_0: "http.extraheader",
  GIT_CONFIG_VALUE_0: `Authorization: ${extraHeader}`,

  Spreading ...process.env and then resetting GIT_CONFIG_COUNT to 1 silently drops every GIT_CONFIG_KEY/VALUE_N entry already in the environment. ADO agents sometimes inject these (e.g., for credential helpers or additional auth headers on earlier checkout steps). The risk here is low in practice because ADO step environments don't carry over previous steps' mutations, but the pattern is fragile: any pipeline-level GIT_CONFIG_* variable group or agent-injected var would be dropped without any error.

  Safer approach: read parseInt(process.env.GIT_CONFIG_COUNT ?? "0", 10) and inject the auth header at index N, preserving the existing N entries.

• scenario.ts:72 — the setup() interface doesn't document partial-cleanup responsibility

  The contract in the Scenario interface says nothing about what setup() must do when it partially creates ADO resources before throwing a non-SkipError. The pr.ts scenarios handle this correctly inline (manual teardownPr().catch(() => {}) before rethrowing), but this convention isn't visible at the interface level. Future scenario authors could easily forget it and leave dangling ADO objects on a flaky setup step.

  Suggested addition to the setup JSDoc:

  /**
   * Create ADO preconditions; return remembered state.
   *
   * When this throws (non-SkipError), the runner will NOT call cleanup().
   * Any ADO objects partially created before the throw must be explicitly
   * torn down inside this function (see pr.ts for the pattern).
   */
  setup(ctx: ScenarioContext): Promise<State>;

• scenarios/work-item.ts:132 — upload-workitem-attachment assertion checks for any AttachedFile relation

  const hasAttachment = relations.some((r) => r.rel === "AttachedFile");

  This passes as long as any attached-file relation exists, not specifically the one we just uploaded. It works correctly today because makeScratchWorkItem always creates a fresh work item with no pre-existing attachments. But the assertion doesn't validate the filename or content of the attachment, so a future change that accidentally attaches an empty or wrong file would still pass the test. Consider asserting on r.attributes?.name matching "attachment.txt" (ADO surfaces the filename in the AttachedFile relation attributes).

✅ What Looks Good

• ##vso injection protection in index.ts log() — encoding leading # per line prevents the harness from emitting rogue task commands even if an ADO error body contains one.
• Path traversal guard in execute-cli.ts — resolve()-based containment check on files() keys is correct and correctly uses sep to prevent safeRoot being a prefix of an unrelated path.
• Token never in process argv — runGit() injects the ADO auth header via GIT_CONFIG_* env vars rather than -c http.extraheader=..., preventing it from appearing in /proc/<pid>/cmdline.
• ETag-based wiki updates — putWikiPage correctly threads the If-Match ETag to prevent blind overwrites on update-wiki-page.
• setupDone cleanup guard — cleanup is intentionally skipped for SkipError (no ADO state was created), and the pr.ts scenarios correctly handle inline teardown for partially-created state before rethrowing.
• Timeout on all I/O — both AdoRest (AbortSignal.timeout) and spawnCollect / runGit have bounded timeouts, preventing a single hung endpoint from blocking the whole suite.
• Offline unit test coverage — dedup logic, title building, issue rendering, and runner isolation are all tested without any ADO dependency.

Generated by Rust PR Reviewer for #1353 · 103.5 AIC · ⌖ 6.44 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-isolated test harness with solid security hygiene. Two actionable issues worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• execute-cli.ts:~192-200 and create-pull-request.ts:~60-70 — spawnCollect/runGit silently discard accumulated stdout/stderr when the timeout fires. The rejection message is "ado-aw execute timed out after Nms" with no context about what the process was doing. On a hung CI run you'd have to dig into ADO logs manually instead of seeing partial output in the issue body. Suggest including it:

  reject(new Error(
    `ado-aw execute timed out after ${timeoutMs}ms` +
    (stdout.trim() ? `\n--- partial stdout ---\n${stdout.slice(0, 500)}` : "")
  ));

  Same fix applies to runGit in create-pull-request.ts.

• runner.ts:~75-90 contract gap — When config(), ndjson(), or files() throw (after setupDone = true), the outer finally block correctly runs cleanup() — but the scenario.ts contract docstring only documents the setup-throw case:

  "When this throws (a non-SkipError), the runner will NOT call cleanup()"

  This is right for setup, but implies cleanup never runs on failure — when it actually always runs once setupDone = true. A future scenario that creates an ADO object inside config() or ndjson() (unlikely but not impossible) would get cleanup for free, which could be surprising. Worth adding a note to the setup docstring: "For failures in config()/ndjson()/files() (after setup completes), cleanup() IS called."

⚠️ Suggestions

• github-issue.ts:~120 — findOpenIssueByTitle uses per_page=100 as a hard ceiling. The comment correctly acknowledges this; just noting that for long-running suites where the [executor-e2e-failure] prefix accumulates many historical issues (even closed ones narrow by is:open, so it's fine currently), this approach degrades gracefully. No change needed, just confirm the team accepts the tradeoff.

• create-pull-request.ts:~155 — If ado-aw-det/${buildId}.md somehow already exists in the repo's default branch (e.g., a past run's cleanup failed and someone manually merged the PR), git add -N becomes a no-op on an already-tracked path, git diff produces no output, and setup throws "generated patch is empty". The throw is clean and scenario is correctly recorded as failed, so this is handled — just worth a comment explaining the add -N assumption (file must be untracked).

✅ What Looks Good

• Auth token never in argv: GIT_CONFIG_KEY/VALUE_* env-var injection for git auth is exactly right — token stays out of /proc/<pid>/cmdline.
• ##vso[ injection guard in log() is correctly anchored with ^ + gm flags. Covers all lines including multi-line stderr passhtrough.
• Path traversal guard in execute-cli.ts uses sep-anchored prefix (safeRoot + sep) — immune to the classic /tmp/safe prefix-matching /tmp/safe-evil bypass.
• setupDone gate: cleanup correctly runs if and only if setup succeeded, with inline teardown in setupPr's catch block for partially-created objects — that's the right pattern per the contract.
• SkipError semantics thread cleanly through all build/wiki/queue-build scenarios; the suite will never go red from missing optional infrastructure.
• Packaging carve-out (NON_BUNDLE_DIRS, gitignored test-bin/, build:executor-e2e outside the main build chain) is airtight — no risk of shipping the test harness in ado-script.zip.

Generated by Rust PR Reviewer for #1353 · 84.7 AIC · ⌖ 6.7 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured test harness with one resource-leak bug and a performance concern in the flagship scenario.

---

Findings

🐛 Bugs / Logic Issues

• scenarios/pr.ts:33–48 — Branch leaked if createPullRequest throws

  setupPr wraps only the createThread call in a guard that explicitly tears down on failure (lines 55–61). But if pushAddFileBranch succeeds and createPullRequest subsequently throws (transient ADO 5xx, rate-limit, etc.), setupDone stays false, so the runner skips cleanup() and the created branch ${ctx.prefix(tool)}-src is left dangling in the repo.

  The scenario.ts contract explicitly calls this out: "Any ADO objects partially created before the throw must therefore be torn down explicitly inside this function before rethrowing." The createThread guard shows the right pattern; createPullRequest needs the same treatment:

  let branch: string | undefined;
  // ...
  branch = `${ctx.prefix(tool)}-src`;
  await ctx.rest.pushAddFileBranch(...);
  
  let pr: { pullRequestId: number };
  try {
    pr = await ctx.rest.createPullRequest(...);
  } catch (err) {
    await ctx.rest.deleteRef(repo, `refs/heads/${branch}`).catch(() => {});
    throw err;
  }

  This affects all five PR scenarios (addPrComment, replyToPrComment, resolvePrThread, submitPrReview, updatePr), so a single unlucky transient failure during a daily run silently accumulates branches in agent-definitions.

---

⚠️ Suggestions

• scenarios/create-pull-request.ts:~100 — No --depth 1 on git clone

  await git(ctx, ["clone", cloneUrl, checkout], sourcesDir, authHeader);

  The clone has no depth limit. For the agent-definitions repo today this is probably fine, but a full clone is surprisingly easy to blow past the 5-minute EXECUTOR_E2E_GIT_TIMEOUT_MS default if the repo grows or the ADO agent has constrained network. ["clone", "--depth", "1", cloneUrl, checkout] would be enough since the scenario only needs the current HEAD to build a patch.

---

✅ What Looks Good

• ##vso injection protection in index.ts:log() (/^#/gm with the multiline flag) correctly sanitises multi-line output from the ado-aw execute subprocess before it reaches ADO's log processor.
• Token never in argv: create-pull-request.ts passes the ADO PAT via GIT_CONFIG_KEY/VALUE_N env vars, keeping it out of /proc/<pid>/cmdline.
• Path traversal guard in execute-cli.ts (resolve() + sep-based containment check) on the files() staging step is solid.
• Scenario isolation design (each scenario catches its own errors, suite continues, cleanup is finally-guarded) is exactly right for a live-ADO test.
• SkipError pattern for optional preconditions (wiki, queue-build pipeline id) is clean — a missing handoff never reddens the suite.
• Deduped issue filing by exact title prevents spam on recurring failures.
• renderSourceMarkdown uses JSON.stringify for every user-controlled field so unusual tool names or repo names can't produce broken YAML front matter.

Generated by Rust PR Reviewer for #1353 · 74.5 AIC · ⌖ 6.39 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-designed test harness — solid architecture, thoughtful safety rails, and good coverage. A couple of correctness issues worth addressing before the live run.

Findings

🐛 Bugs / Logic Issues

• scenarios/pr.ts — teardownPr sequential failure leaks branch (teardownPr, lines 2248–2250): abandonPullRequest is called first; if it throws (non-404 network error), deleteRef never runs, leaving an orphaned source branch. The outer runner.ts cleanup catch swallows the error as a WARNING, so the branch only gets cleaned by the janitor backstop. Suggestion: wrap each call independently so both always attempt:

  async function teardownPr(ctx, state) {
    await ctx.rest.abandonPullRequest(state.repo, state.prId).catch(() => {});
    await ctx.rest.deleteRef(state.repo, `refs/heads/${state.branch}`);
  }

• scenarios/build.ts — submitPrReview assert over-accepts (submitPrReview.assert, line 2347): vote > 0 accepts both "Approve" (vote=10) and "Approve with suggestions" (vote=5). The scenario submits event: "approve" which maps to vote=10. The assertion should be r.vote === 10 to catch an executor regression that produces the wrong vote type.

⚠️ Suggestions

• execute-cli.ts — timeout env vars undocumented: EXECUTOR_E2E_REST_TIMEOUT_MS, EXECUTOR_E2E_EXECUTE_TIMEOUT_MS, and EXECUTOR_E2E_GIT_TIMEOUT_MS are useful knobs for slow environments but aren't mentioned in the README or pipeline YAML comments. Worth a one-liner in each section of the README so ops can tune them without source-diving.

• runner.ts — cleanup skipped on partial setup(): The scenario contract (documented in scenario.ts) correctly states that cleanup is NOT called when setup throws, and pr.ts's setupPr handles this well with explicit inline teardown. However the contract is easily violated by future scenario authors who may not read the caveat. Consider a short // IMPORTANT: if setup throws, cleanup WILL NOT run... comment directly above the setupDone = true assignment as a point-of-action reminder.

✅ What Looks Good

• SkipError pattern — graceful degradation without poisoning the suite result; test is comprehensive.
• GIT_CONFIG_* auth injection — token stays out of process argv (avoids /proc/<pid>/cmdline exposure) with the existing-count preservation logic.
• ##vso injection guard in the log() function (/^#/gm → %23) — correctly neutralizes double-# ADO logging commands.
• Path traversal guard in runExecute (safeRoot + sep check) — right to have even for trusted authors.
• Dedup by exact title in fileFailureIssue — prevents issue spam on recurring failures.
• Timeout on all subprocess calls — hung ado-aw execute or git clone can never block the whole suite.
• Partial-setup teardown in setupPr — the inline catch+rethrow pattern correctly handles the source-branch-exists-but-PR-creation-failed case.

Generated by Rust PR Reviewer for #1353 · 76.2 AIC · ⌖ 7.24 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: All previously flagged issues are fixed. Two new issues worth addressing before merge — one correctness bug, one resilience gap.

---

Findings

🐛 Bugs / Logic Issues

• scenarios/work-item.ts:createWorkItem.assert:35 — state.createdId is set using only typeof rawId === "number", but typeof NaN === "number" is true. If the executor returns a NaN-valued id (e.g., from a malformed result payload), state.createdId = NaN, then the !Number.isFinite(rawId) check throws, and cleanup calls ctx.rest.deleteWorkItem(NaN) → _apis/wit/workitems/NaN?.... The numResult helper in common.ts already handles this correctly (checks both typeof and isFinite), and queueBuild.assert uses it — createWorkItem.assert should too:

  // Before
  state.createdId = typeof rawId === "number" ? rawId : undefined;
  if (typeof rawId !== "number" || !Number.isFinite(rawId)) { throw ... }
  
  // After
  state.createdId = numResult(record, "id");

  (Or at minimum: state.createdId = typeof rawId === "number" && Number.isFinite(rawId) ? rawId : undefined;)

⚠️ Suggestions

• github-issue.ts:findOpenIssueByTitle and createGitHubIssue — Both fetch() calls have no timeout (AbortSignal). The AdoRest class consistently passes signal: AbortSignal.timeout(this.timeoutMs) for every ADO call, but the GitHub client functions call raw fetchImpl(url, { headers: ... }) with no signal. A hung GitHub API response would stall main() indefinitely after all scenarios complete, preventing the pipeline step from exiting and consuming the ADO job's wall-clock limit. Fix: add signal: AbortSignal.timeout(30_000) to both calls (matching the AdoRest default), or thread a timeout through GitHubClientOptions.

✅ What Looks Good

• Every issue from the four previous review passes has been addressed: SYSTEM_TEAMPROJECT URL-encoded; prId populated before fallible assertions; spawnCollect and runGit have SIGKILL timeouts; auth token passed via GIT_CONFIG_* env vars; setupDone guard no longer checks state !== undefined; non-JSON ADO responses now throw instead of silent-casting; authedFetch centralises auth and timeout for all ADO calls (including getWikiPage); config/ndjson/files/env wrapped in error-contained try-catch; symbolic-ref fallback in try-catch; YAML tool key now JSON.stringify-quoted; Scenario<unknown>[] throughout.
• updateWikiPage.setup handles the ETag-conditional PUT correctly — the putWikiPage call is the last operation before the return, so a throw leaves setupDone=false with no leaked page.
• pr.ts:setupPr manually tears down the source branch and PR on any post-push failure, correctly keeping setup atomic when setupDone hasn't been set yet.
• create-pull-request.ts cleanup removes sourcesDir so repeated local runs don't accumulate large git checkouts.
• RustInstaller@1 task in the pipeline replaces the previous curl | sh install pattern.

Generated by Rust PR Reviewer for #1353 · 110.3 AIC · ⌖ 8.66 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid, well-structured test harness — looks good to merge with one medium-priority fix and a couple of minor notes.

---

Findings

⚠️ Suggestions

• github-issue.ts (findOpenIssueByTitle / createGitHubIssue) — missing AbortSignal timeout on GitHub API calls

  The ADO REST client in ado-rest.ts correctly uses AbortSignal.timeout(this.timeoutMs) on every fetch. The GitHub client does not:

  // findOpenIssueByTitle
  const res = await fetchImpl(url, { headers: ghHeaders(opts.token) });
  // createGitHubIssue
  const res = await fetchImpl(url, { method: "POST", headers: ..., body: ... });

  If GitHub's API is unreachable or stalls (not a hard connection refusal), neither call times out — they hang until the OS's socket timeout, which can be minutes. The wrapping try/catch in index.ts catches thrown errors but does nothing for hangs. Since issue-filing is in the finally-adjacent block that runs after runAll, a stuck HTTP call here delays pipeline teardown.

  Suggestion: add signal: AbortSignal.timeout(30_000) to both fetchImpl calls, consistent with the ADO client pattern.

• github-issue.ts:35 — ADO_AW_DEBUG_GITHUB_TOKEN fallback is surprising

  token: env.EXECUTOR_E2E_GITHUB_TOKEN?.trim() || env.ADO_AW_DEBUG_GITHUB_TOKEN?.trim(),

  If EXECUTOR_E2E_GITHUB_TOKEN is absent but ADO_AW_DEBUG_GITHUB_TOKEN is present in the environment (e.g. from a debug session or a variable group that includes it), the harness will silently file issues using that token. The two tokens likely have different scopes and identities (the debug token is documented in docs/ado-aw-debug.md for filing issues from dogfood pipelines). For a production CI pipeline that shares a variable group, this could produce confusing cross-token issue filing. Worth a comment or a dedicated EXECUTOR_E2E_GITHUB_TOKEN-or-skip policy.

• runner.ts:133 — state as S type assertion on cleanup

  await scenario.cleanup(ctx, state as S);

  setupDone being true guarantees setup() returned without throwing, but TypeScript sees state as S | undefined because the declaration let state: S | undefined. The cast is safe in practice since all scenarios return non-undefined state, but if a future scenario has State = void it will silently pass undefined to cleanup. A minor refinement: declare state as Awaited<ReturnType<(typeof scenario)['setup']>> or add an explicit if (state === undefined) { ... } guard with a log, which would also give cleanup a chance to run with a no-op default.

---

✅ What Looks Good

• ##vso injection protection in log() — msg.replace(/^#/gm, "%23") is the right defence; the gm flags correctly handle multi-line messages (including partial-output dumps).

• Path traversal guard in runExecute — the target.startsWith(safeRoot + sep) check is correct and handles the OS sep correctly on Linux.

• Git auth via GIT_CONFIG_* env vars — the token is kept out of process.argv (not visible in /proc/<pid>/cmdline), and GIT_CONFIG_COUNT is appended rather than overwritten, preserving any existing entries from the agent pool.

• Teardown class — the pattern of running all cleanup steps and aggregating failures is correct and well-motivated. A single await a(); await b(); chain would silently skip b whenever a rejected, which the code comment explains.

• setupDone guard — the runner correctly distinguishes "setup completed" from "cleanup should run", and pr.ts::setupPr follows the documented contract (inline-teardown on partial failure before rethrowing).

• putWikiPage ETag handling — the update-wiki-page scenario correctly fetches the existing ETag and uses it for the conditional PUT, matching ADO's HTTP 412 semantics.

• Bundle exclusion — executor-e2e is properly carved out of NON_BUNDLE_DIRS so the release glob (ado-script/*.js) never packages it, and build:executor-e2e emits to test-bin/ (gitignored).

Generated by Rust PR Reviewer for #1353 · 68.1 AIC · ⌖ 6.53 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — high-quality test harness with thoughtful error handling and security hardening throughout.

---

Findings

⚠️ Suggestions

• create-pull-request.ts:runGit — GIT_CONFIG_COUNT when existing count is 0 or NaN

  const idx = Number.isFinite(existingCount) && existingCount > 0 ? existingCount : 0;

  When GIT_CONFIG_COUNT is absent or "0", idx = 0 and we correctly append at position 0. However, if it's somehow an invalid non-finite value (e.g. "NaN" from a pathological pipeline variable group), idx resets to 0, and setting GIT_CONFIG_COUNT = "1" silently stops git from reading any pre-existing entries at higher indices. Unreachable in a normal ADO environment, but the comment says "Append at the next free index rather than clobbering to 1" — the NaN branch contradicts that intent. A defensive fix would be: const idx = (Number.isInteger(existingCount) && existingCount > 0) ? existingCount : 0; which also rules out Infinity.

• runner.ts — setup() partial-failure cleanup is a footgun for future scenario authors

  The contract (cleanup doesn't run when setup() throws, so setup() must inline-teardown any partially-created remote state before rethrowing) is documented clearly in scenario.ts and the setupPr pattern in pr.ts is a good reference. But it's easy to miss: a new scenario author who creates two ADO objects in setup() and catches only the second creation could leak the first. Consider adding a brief note to the Scenario.setup JSDoc pointing explicitly to pr.ts:setupPr as the canonical pattern (or extracting a safeSetup helper that enforces the teardown obligation). Not blocking — all current scenarios handle this correctly.

• github-issue.ts:renderIssueBody — skipped filter vs ok flag

  The body renders three buckets: failed = !r.ok, skipped = r.skipped, passed = r.ok && !r.skipped. Since runner.ts sets ok: true for skipped results, a skipped result appears in both the skipped and potentially passed buckets if skipped is truthy but someone reads ok. In renderIssueBody the passed filter is r.ok && !r.skipped, which correctly excludes skipped. ✅ But the skipped filter results.filter((r) => r.skipped) doesn't check r.ok, so a scenario with { ok: false, skipped: true } (currently impossible per runner, but defensible to guard) would appear in both the failure table and the skipped section. Very minor, but aligning skipped to r.ok && r.skipped would be tighter.

---

✅ What Looks Good

• Teardown class — elegant fix for the silent-skip bug in sequential teardown awaits. The aggregated error reporting via a collected failures[] array is exactly right.

• SkipError + setupDone gate — clean separation between "precondition missing" (skip, don't fail) and "unexpected error" (fail). All current scenarios respect the contract.

• Token handling — auth is centralized in AdoRest.authedFetch and in runGit's GIT_CONFIG_* env injection. The token never appears in child process argv or in the rendered source markdown (JSON-stringified config values). ✅

• VSO command injection prevention in log() — replace(/^#/gm, "%23") with the m flag correctly encodes the leading # on each line, preventing ##vso[task.setvariable] injection from scenario output.

• Path traversal guard in execute-cli.ts — the resolve(safeRoot + sep) prefix check is correct on both Linux and Windows.

• numResult / strResult guards — using typeof value !== "number" || !Number.isFinite(value) rather than a bare !== undefined check correctly rejects NaN and null coercions that would silently leak invalid IDs into cleanup.

• Offline unit tests — the execute-cli.test.ts, github-issue.test.ts, and runner.test.ts suites cover the meaningful logic paths (dedup, skip vs fail, record parsing, source rendering) without requiring a live ADO connection. Good coverage for a test harness.

• Sequential scenario execution — avoids ADO rate-limit contention and gives deterministic output ordering. The explicit for...of in runAll makes this obvious and prevents accidental parallelism from a future refactor swapping it for Promise.all.

Generated by Rust PR Reviewer for #1353 · 90.7 AIC · ⌖ 7.07 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured test harness with thoughtful defensive patterns. A few items worth addressing.

Findings

🐛 Bugs / Logic Issues

• runner.ts:62–70 — implicit any on let config, entry, files, extraEnv
let config, entry, files, extraEnv; with no type annotations leaves these inferred as any until assignment. TypeScript can't enforce that the values passed to runExecute (which expects config: Record<string, unknown>) satisfy the contract. This is already masked by the clean typecheck if noImplicitAny isn't enforced — but adding explicit annotations (let config: Record<string, unknown>; let entry: Record<string, unknown>; etc.) would restore the type-safety the rest of the file provides.

• create-pull-request.ts:assert — PR leak if numResult throws before state.prId is set

  const prId = numResult(record, "pull_request_id");  // can throw
  state.prId = prId;  // never reached

  If the executor returns a succeeded record with no numeric pull_request_id, numResult throws before state.prId is set — cleanup then skips the abandonPullRequest step, leaking a PR the executor may have created. Reordering to assign state.prId from record.result?.pull_request_id first (and throwing explicitly afterward on invalid type) would close this.

• scenarios/build.ts:38 — ndjson sends build_id as a number; executor may expect the env var
  addBuildTag.ndjson returns { build_id: state.buildId } where state.buildId: number. Whether the Rust add-build-tag executor reads this field from NDJSON or from BUILD_BUILDID (string) in the environment isn't visible here. Both point to the same build, so the assert will pass either way — but if the executor ignores the NDJSON build_id field entirely, the scenario isn't exercising the executor's build-id-resolution path. Worth checking against src/safe_outputs/add_build_tag.rs.

⚠️ Suggestions

• runner.ts near setupDone = true — comment is slightly misleading
  The inline comment says "cleanup runs ONLY after this point" which implies failures in config()/ndjson()/files() don't run cleanup. They do — the early returns for those failures are inside the outer try, so finally (and therefore cleanup) always executes when setupDone is true. The scenario.ts contract comment already correctly documents this distinction. Adding a one-liner clarification in runner.ts would remove the ambiguity.

• github-issue.ts:findOpenIssueByTitle — single-page search may miss an existing issue when title prefix is very common
  Fetching per_page=100 is noted as sufficient for expected scale, but if search-rate throttling causes a 403, fileFailureIssue propagates the throw (caught by the index.ts warning handler). On a 403 the harness won't file a new issue, so at least duplicates aren't created — but the failure silently goes unlogged to GitHub. Handling 403/429 explicitly with a log message would improve observability.

✅ What Looks Good

• Teardown class: Excellent pattern. Running every cleanup step regardless of earlier failures and aggregating errors is exactly right — prevents the silent resource-leak from await step1(); await step2() when step1 throws.
• ##vso injection guard in log(): msg.replace(/^#/gm, "%23") is a clean one-liner that neutralises both ##vso[task.setvariable ...] and ##[error] log commands that could be smuggled via executor stdout/stderr.
• Token via env, not argv: Auth headers in runGit use GIT_CONFIG_* env vars. spawnCollect and runGit only log args (URLs/branch names), never the token. The GIT_CONFIG_COUNT append correctly preserves any existing entries.
• Path traversal guard in execute-cli.ts (target !== safeRoot && !target.startsWith(safeRoot + sep)) — properly handles ., empty string, and parent-traversal attempts.
• state.prId = prId before fallible calls in assert (for the getPullRequest call) — good defensive pattern; cleanup can abandon the PR even if the REST assertion throws. Just needs to be extended to numResult itself (see bug above).
• SkipError / sequential execution / bundle exclusion: All well-considered.

Generated by Rust PR Reviewer for #1353 · 73.6 AIC · ⌖ 6.55 AIC · ⊞ 6.2K · ◷