test(trigger-e2e): add deterministic gate/synth-PR trigger-condition E2E suite

[jamesadevine]

Summary

Adds an ADO-native, deterministic (non-agentic) E2E test framework for the runtime front-matter trigger conditions managed by ado-script — the gate.js PR filter evaluator and the exec-context-pr-synth.js synthetic-PR promotion bundle — modeled on the existing executor-e2e native bundle.

The executor-e2e suite covers Stage 3 safe-output execution but nothing decides whether the agent runs at all. This suite closes that gap with no LLM in the loop:

1. creates real ADO PR context (branch + PR + labels + draft) via REST,
2. queues a hand-authored parameterized victim pipeline on the PR's source branch with a per-scenario base64 GATE_SPEC / PR_SYNTH_SPEC as template parameters,
3. polls the build and asserts the observable gate decision via build tags + build result,
4. cleans up everything, and files a GitHub issue on failure.

Key design: a real open PR lets exec-context-pr-synth set AW_SYNTHETIC_PR=true, so the gate runs full PR-filter evaluation even on an API-queued (Manual-reason) build — one victim covers synth-promotion and every gate filter authentically, with no server-side build-validation policy. Failing filters self-cancel to a terminal canceled result.

What's added

• scripts/ado-script/src/trigger-e2e/ — test-only harness bundle:
  • gate-spec.ts — faithful TS port of the Rust build_gate_spec/lower_pr_filters spec construction; imports the codegen'd types.gen.ts so schema drift is a compile error.
  • scenario.ts / runner.ts / queue.ts — scenario contract + isolated always-cleanup runner + victim queue/poll.
  • scenarios/ — synth-PR (promote / branch- & path-mismatch), gate filters pass+skip (labels, changed-files, draft, target-branch, build-reason, change-count, time-window), bypass, self-cancel.
  • github-issue.ts / index.ts — failure-issue filing (reusing executor-e2e GitHub primitives) + env-driven entry point.
• tests/trigger-e2e/ — hand-authored victim-pipeline.yml (parameterized), azure-pipelines.yml orchestrator, and README.md with the one-time ADO registration handoff.
• scripts/ado-script/src/executor-e2e/ado-rest.ts — added shared queueBuild / waitForBuild / setPullRequestLabels helpers + optional isDraft on createPullRequest.

Not shipped

Excluded from ado-script.zip exactly like executor-e2e: kept out of the build chain, emits to gitignored test-bin/trigger-e2e.js, and added to NON_BUNDLE_DIRS in bundle-coverage.test.ts. The runtime zip still contains only production bundles.

Test plan

• npm run typecheck (scripts/ado-script) ✅
• New unit tests: 18 pass — gate-spec fact derivation/ordering/policies/tag-suffixes/encoding + runner isolation/assertions/cleanup ✅
• npm run build:trigger-e2e → 38 KB bundle emitted to test-bin/ ✅
• bundle-coverage guard test ✅
• Full ado-script vitest suite: 527 pass; the single failure (executor-e2e upload-pipeline-artifact rejection) is pre-existing and reproduces on a clean baseline — a Windows-only node-shebang spawn issue, unrelated to this change.

Live end-to-end validation (running against an ADO project + one-time victim/orchestrator pipeline registration) is a manual handoff documented in tests/trigger-e2e/README.md and cannot be run in CI/local without ADO access.

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured test harness with a few worth-noting items.

Note: this PR is TypeScript, not Rust — reviewing against the repo's quality standards regardless.

Findings

⚠️ Suggestions

• gate-spec.ts:FACT_META fidelity drift — the FACT_META map is a hand-maintained port of filter_ir.rs::Fact::{kind,failure_policy,dependencies}. The types.gen.ts import catches schema drift in the serialized shape, but policy/dependency changes in Rust (e.g. a new fact, a changed failure_policy) are silent until a live E2E run regresses. A comment in filter_ir.rs pointing back to gate-spec.ts (or a // SYNC: gate-spec.ts::FACT_META annotation) would make the coupling explicit and easier to catch in code review.

• common.ts:createPrContext — entries[0]! non-null assertion (scenarios/common.ts ~line 88): Object.entries(files)[0]! is safe for the generated default, but callers who pass an explicit opts.files: {} would hit a runtime undefined-destructuring crash despite the !. A guard or a pre-check (if (entries.length === 0) throw ...) would be more robust.

• Leaked victim builds on queue-phase timeout (runner.ts ~line 60): when runVictim throws (e.g. waitForBuild times out), the runner returns phase: "queue" and still executes cleanup — but cleanup only tears down the PR context, not the queued-but-still-running victim build, since the BuildOutcome.buildId is not threaded into scenario state. For a long-running suite this can accumulate orphaned builds. Consider storing the queued build id in scenario state (or a separate cleanupBuildId in TriggerContext) so cleanup can attempt a cancel.

✅ What Looks Good

• The ##vso command-injection guard in index.ts::log (replace(/^#/gm, "%23")) is a nice defensive touch.
• createPrContext / teardownPrContext correctly follow the executor-e2e cleanup contract: partial-failure teardown happens inline before rethrowing, so the runner's "no cleanup on setup failure" rule doesn't leak ADO objects.
• Excluding the harness from ado-script.zip via NON_BUNDLE_DIRS + test-bin/ gitignore is clean — no accidental runtime surface.
• buildGateSpec emitting facts in FACT_META insertion order (Map preserves ES2015 insertion order) is correct and intentional; the topological comment makes this explicit.
• Unit test coverage of fact derivation, ordering, deduplication, and tag-suffix alignment with Rust is solid for a test-harness module.

Generated by Rust PR Reviewer for #1504 · 49.6 AIC · ⌖ 5.81 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured test-only addition — looks good overall with one potential flakiness concern worth addressing.

Findings

🐛 Bugs / Logic Issues

• pr-filters.ts & synth-pr.ts: PROMOTE_ALL / synth spec hardcode "main" — createPrContext in common.ts dynamically resolves the repo's default branch via defaultBranchShortName, but PROMOTE_ALL = encodePrSynthSpec({ branches: { include: ["main"] } }) and all three synth scenarios hardcode branches: { include: ["main"] }. If the ADO repo's default branch is anything other than main, every filter scenario and all synth-promote/mismatch scenarios will be silently wrong (the PR correctly targets the real default branch, but the synth spec won't match it). Consider threading state.targetBranch through to the queue() call, or resolving the target branch in PROMOTE_ALL at runtime rather than as a module-level constant.

⚠️ Suggestions

• ado-rest.ts setPullRequestLabels: no per-label error isolation — the sequential POST loop stops on first failure, potentially leaving the PR with only some labels. For a test harness this is tolerable, but a failed label attach will surface as a confusing gate failure rather than a setup failure. Wrapping each POST in a try/rethrow with labels[i] context in the message would make diagnostics much clearer.

• index.ts log(): ##vso guard is line-start-only — msg.replace(/^#/gm, "%23") correctly escapes leading # per line, but only guards against logs that start with ##vso. ADO processes ##vso[...] anywhere in a line when the task runner scans stdout. The existing guard is still useful for the most common injection vector (a message that is entirely a ##vso command), and the harness controls all log() call sites today, so this is low risk — just worth noting if the log API is ever opened to scenario-author-controlled strings.

✅ What Looks Good

• NON_BUNDLE_DIRS guard in bundle-coverage.test.ts cleanly ensures the harness never lands in ado-script.zip.
• SkipError / requirePrRepo pattern: scenarios skip gracefully (not fail) when TRIGGER_E2E_VICTIM_REPO is absent — good for environments without ADO access.
• Cleanup isolation in runner.ts: each scenario's cleanup always runs after setupDone, and a failure in one scenario never prevents later ones from running.
• waitForBuild timeout is env-configurable with a sensible 15-minute default; the deadline-check-before-sleep ordering is correct.
• gate-spec.ts fidelity contract is clearly documented and the import of types.gen.ts makes schema drift a compile error rather than a silent wrong answer — good design.
• Victim pipeline trigger: none; pr: none: correctly prevents any accidental CI trigger on this hand-authored file.

Generated by Rust PR Reviewer for #1504 · 39.4 AIC · ⌖ 5.8 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured E2E framework with one latent bug and a minor coupling concern.

Findings

🐛 Bugs / Logic Issues

• scenarios/common.ts — multi-file push will fail on the second file (createPrContext). The pushAddFileBranch helper always passes oldObjectId: "0000000000000000000000000000000000000000" (new-branch semantics). The loop for entries.slice(1) calls pushAddFileBranch again on the same branch — but it already exists at that point, so ADO will reject the push with a ref-conflict error. The bug is currently masked because every scenario passes at most one file, so entries.slice(1) is always empty. Adding a two-file scenario (e.g., a change-count-pass case) would surface it. The fix is to either batch all files into a single push, or use a separate "push to existing branch" call (with the real current HEAD as oldObjectId) for subsequent files.

⚠️ Suggestions

• executor-e2e/ado-rest.ts — trigger-e2e env vars leak into the shared client. waitForBuild now reads TRIGGER_E2E_BUILD_TIMEOUT_MS and TRIGGER_E2E_BUILD_POLL_MS from process.env. These are trigger-e2e–specific variables embedded in the shared executor-e2e AdoRest class. An executor-e2e caller that happens to have those variables set (e.g., from a parent shell) will silently get different poll timing. Consider either moving the env-var defaults out to index.ts and passing them as opts, or renaming to a more generic key like ADO_BUILD_TIMEOUT_MS.

• filter_ir.rs / gate-spec.ts — FACT_META sync is doc+convention only. The gate-spec.test.ts fact-ordering and policy tests provide good coverage for currently-used facts, but only for the cases exercised in tests. A new Fact variant added to Rust without updating FACT_META will throw a runtime error ("unknown fact kind '...'") rather than a compile error. This is loud (not silent), so acceptable — just worth knowing when adding facts.

• scenarios/pr-filters.ts — missing change-count-pass scenario. change-count-skip is present but there's no corresponding pass case to verify that a count within the range does NOT self-cancel. One small scenario (pr: { files: { ... } }, check: changeCountCheck({ max: 10 }), outcome: "pass") would complete the predicate coverage. (Also: fixing the multi-file bug above would unblock a more faithful count-based test.)

✅ What Looks Good

• Runner isolation (runner.ts) is excellent: guaranteed finally-block cleanup, orphaned-build cancellation via onQueued callback so a waitForBuild timeout never leaks a running build, and SkipError propagation that lets missing-precondition scenarios skip cleanly without polluting the failure count. The unit tests in runner.test.ts cover all these paths.
• Schema drift guard: the gate-spec.test.ts test that asserts encodePrSynthSpec() produces the exact base64 hardcoded in victim-pipeline.yml is a nice way to catch silent format divergence without needing a full integration run.
• common.ts partial-setup cleanup: the try/catch that deletes the source branch if PR creation fails correctly handles the partially-created state and re-throws, which is the documented pattern for setup in the scenario contract.
• filter_ir.rs change is purely additive (doc comment); no logic is touched.

Generated by Rust PR Reviewer for #1504 · 63.7 AIC · ⌖ 5.86 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-structured E2E harness with a handful of minor observations worth tracking.

Findings

⚠️ Suggestions

• gate-spec.ts / FACT_META drift is undiscoverable until runtime. The types.gen.ts codegen protects the serialized GateSpec shape, but FACT_META's failure_policy and dependencies values are invisible to any type-checker or CI gate. The comment in filter_ir.rs documents the manual sync, which is good — but if a future contributor changes failure_policy() or adds a Fact variant in Rust without updating FACT_META, the TypeScript mirror will happily compile and only silently produce wrong specs at runtime. Consider a // @keep-in-sync`` count assertion in the test file (e.g. assert FACT_META.size === <N>) as a cheap tripwire, since the Rust enum's variant count is at least machine-verifiable.

• scenario.ts partial-setup cleanup is caller responsibility — document the invariant more visibly in common.ts. The TriggerScenario.setup doc says "Any ADO objects partially created before the throw must be torn down explicitly inside this function." createPrContext in common.ts uses the executor-e2e Teardown helper, which is the right pattern, but the Teardown guarantee is only in scope inside createPrContext. If a scenario's setup calls createPrContext successfully and then does extra work that throws, the PrContext will be leaked (runner won't call cleanup because setupDone is still false at the throw point). This is a pre-existing executor-e2e convention rather than a new bug, but scenarios that do multi-step setup should note the risk.

• victim-pipeline.yml — hardcoded pool name (AZS-1ES-L-Playground-ubuntu-22.04) is reasonable for a manually-registered pipeline, but tests/trigger-e2e/README.md should call this out explicitly as something the operator may need to change for their ADO project. Worth a one-liner in the registration instructions.

• index.ts log() ##vso guard (msg.replace(/^#/gm, "%23")) is good defensive hygiene, but note this only covers output written via the harness logger. The errMessage path in runner.ts is routed through the same logger, so user-controlled strings from ADO (e.g. a PR title in an error message) would also be sanitized — that looks correct.

✅ What Looks Good

• FACT_META completeness: cross-checked against the live Fact enum in filter_ir.rs — all 14 variants are present, in the same declaration order, with correct failure_policy and dependencies. The fidelity is solid at this snapshot.
• Orphaned-build cleanup in runner.ts: capturing queuedBuildId before awaiting waitForBuild and cancelling in finally is exactly the right pattern for preventing leaked builds on timeout.
• pollOptions() in queue.ts: using || undefined correctly handles NaN / 0 from malformed env vars without crashing.
• NON_BUNDLE_DIRS guard in bundle-coverage.test.ts: explicitly excluding trigger-e2e prevents the harness from accidentally landing in the release zip.
• The overall three-phase (setup / queue+poll / assert) isolation with per-phase error attribution in ScenarioResult is clean and will make triage of failures straightforward.

Generated by Rust PR Reviewer for #1504 · 51.6 AIC · ⌖ 8.29 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-engineered test infrastructure with solid compile-time guardrails.

Findings

⚠️ Suggestions

• src/main.rs:1437–1441 — std::fs::write(&path, &catalog)? and std::fs::create_dir_all(parent)? propagate raw OS errors without .context(). Consistent with ExportGateSchema above it, but since this is a CLI path, adding .with_context(|| format!("writing fact catalog to {}", path.display())) would make failures immediately actionable.

• src/compile/filter_ir.rs — [Fact; 14] in Fact::ALL is a double compile-time guard (count + the exhaustiveness match below it), which is intentional. It's worth a one-line comment explaining that the 14 must be kept in sync, since it's the only non-obvious maintenance burden in the new code. The exhaustiveness reminder already has a good doc comment but doesn't mention updating the size literal.

• scripts/ado-script/src/trigger-e2e/gate-spec.ts — predicateFacts uses (p as { fact?: unknown }).fact to duck-type the fact field. If a future PredicateSpec variant renames that field, this cast silently returns undefined and the fact goes unregistered — causing under-specified FactSpec without a compile error. Since this is test code it's not a blocker, but a narrowed discriminant (p.type === "label_set_match" etc.) would be more resilient.

✅ What Looks Good

• _fact_all_exhaustiveness_reminder — elegant compile-time completeness guard; a new Fact variant is a hard compile error until both the wildcard-free match and Fact::ALL are updated.
• generate_fact_catalog() → fact-catalog.gen.json → gate-spec.test.ts drift chain — machine-verifiable sync between Rust and TypeScript. The pattern of using serde_json::to_string_pretty(...).expect(...) mirrors the existing generate_gate_spec_schema and is fine here since the type is trivially serializable static data.
• log() ##vso injection guard in index.ts — msg.replace(/^#/gm, "%23") is the right defence; multiline mode correctly catches injected \n##vso[...] sequences in user-controlled log content.
• pushAddFilesBranch — correctly documents why oldObjectId = zeros is intentional (avoids the ref-conflict the per-file helper would hit on a second call) and throws clearly if no files are provided.
• Runner isolation — finally block always cancels orphaned builds and runs cleanup; a setup-phase skip or queue-phase timeout cannot leak running builds or PR context.
• ExportFactCatalog is #[command(hide = true)], consistent with ExportGateSchema — correctly kept off the user-visible help surface.

Generated by Rust PR Reviewer for #1504 · 43.7 AIC · ⌖ 8.24 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed test infrastructure with thoughtful cleanup guarantees and solid codegen-drift detection. A couple of minor nits.

Findings

⚠️ Suggestions

• scripts/ado-script/src/trigger-e2e/fact-catalog.gen.json (last line): The generated JSON file is missing a trailing newline. The diff shows } as the last character without \n. Some editors, git diff, and line-count tools flag this. Consider adding a \n to the serde_json::to_string_pretty output in generate_fact_catalog() — e.g. format!("{}\n", serde_json::to_string_pretty(...)) — to match the convention used for types.gen.ts.

• tests/trigger-e2e/victim-pipeline.yml (Tag synth outcome step): The bash case pattern for detecting unexpanded ADO macros is correct ('$('*), but the pattern relies on the literal $ not being shell-expanded because it's single-quoted inside a double-quoted heredoc context. The step uses set -euo pipefail with script:, which is fine. No bug — just worth a comment in the bash explaining why '$('* is the correct glob form for ADO macro detection, since it's non-obvious to future editors.

• scripts/ado-script/src/trigger-e2e/index.ts (log function): The ##vso sanitization encodes only a leading # per line (/^#/gm). A multi-line message with a ##vso[...] command on a non-first line would slip through. Since log() is used for scenario progress messages that ultimately come from the harness's own code (not user-controlled), this is low risk — but worth noting for defensiveness.

✅ What Looks Good

• Rust generate_fact_catalog() / ExportFactCatalog command: Clean implementation mirroring ExportGateSchema. The with_context() error propagation added to both commands is an improvement over the bare ? that was there before.
• pushAddFilesBranch in ado-rest.ts: The doc comment clearly explains why batching into a single commit avoids the ref-conflict that a loop of pushAddFileBranch calls would hit. The zero-objectid new-branch convention is correct.
• runner.ts cleanup guarantee: The finally block runs unconditionally (even on early return from the nested setup/queue catch blocks), and the queuedBuildId is captured via the onQueued callback before waitForBuild is awaited — so an orphaned build gets cancelled even if the poll throws a timeout. Robust pattern.
• Codegen drift-detection mechanism: Committing fact-catalog.gen.json and CI-checking it alongside types.gen.ts is the right approach. The _fact_all_exhaustiveness_reminder exhaustive match ensuring Fact::ALL stays complete is a good compile-time guard.
• setPullRequestLabels error context: Naming the specific label that failed in the thrown error message makes setup failures actionable rather than silent gate mismatches downstream.
• NON_BUNDLE_DIRS guard in bundle-coverage.test.ts: Correctly excludes trigger-e2e from the production zip, matching the executor-e2e precedent.

Generated by Rust PR Reviewer for #1504 · 54.2 AIC · ⌖ 8.26 AIC · ⊞ 6.2K · ◷