feat(compile): replace Python gate evaluator with bundled TypeScript gate.js

[jamesadevine]

Summary

Replaces the Python gate-eval.py trigger-filter evaluator with a
bundled TypeScript artifact scripts/gate.js, produced by a new
scripts/ado-script/ workspace. This is variant A2 from the
design walkthrough
— a per-use-site Node bundle, types codegen'd from the Rust IR via
schemars to prevent drift.

The compiler now emits a NodeTool@0 install step plus
node /tmp/ado-aw-scripts/gate.js instead of python3. The bundle
ships in the existing scripts.zip release asset.

Why

gate-eval.py was a 388-line conflation of fact acquisition, ADO REST
calls, predicate evaluation, and self-cancel — hand-rolled urllib,
no SDK, no real test framework. Moving to TypeScript lets us reuse
azure-devops-node-api for REST calls, run vitest with proper mocks,
and unblock future internal bundles (e.g. poll.js) that follow the
same per-use-site pattern.

What's in the diff (53 files)

Compiler

• src/compile/extensions/trigger_filters.rs — emits NodeTool@0
  step; GATE_EVAL_PATH switched to gate.js.
• src/compile/filter_ir.rs — python3 → node in
  compile_gate_step_external; IR types gain JsonSchema derives;
  new generate_gate_spec_schema().
• src/main.rs — hidden export-gate-schema subcommand consumed by
  the workspace's npm run codegen.

TypeScript workspace — scripts/ado-script/ (40 new files)

• src/shared/ — auth, ado-client (with retries against
  azure-devops-node-api), env-facts, policy state machine,
  vso-logger.
• src/gate/ — bypass, fact acquisition, 11 predicate evaluators,
  self-cancel.
• src/shared/types.gen.ts — auto-generated from
  cargo run -- export-gate-schema → json-schema-to-typescript.
• 173 vitest tests (45 ports of the deleted Python suite + 6 parity
  guards + smoke test + shared-module units).
• ncc-bundled output: scripts/gate.js ≈ 1.1 MB (well under the
  5 MB per-bundle budget).

Pipeline + CI

• .github/workflows/release.yml — adds setup-node +
  npm ci && npm run build before zipping scripts/; excludes
  node_modules, dist/, schema/ from scripts.zip.
• .github/workflows/ado-script.yml — new CI job: install, codegen,
  drift-check (git diff --exit-code on types.gen.ts), tests,
  typecheck, build, smoke test, E2E.

Tests

• tests/gate_e2e.rs (#[ignore]'d) — compiles a real agent,
  extracts GATE_SPEC from the emitted YAML, runs node gate.js
  end-to-end, asserts SHOULD_RUN.
• tests/export_gate_schema.rs — verifies the new subcommand emits
  valid JSON Schema.
• tests/compiler_tests.rs — assertion tightening: was
  compiled.contains("python3") (which silently passed even after
  the migration via base.yml's mcpg-config validation step); now
  asserts the exact node '/tmp/ado-aw-scripts/gate.js' substring.

Docs

• docs/ado-script.md (new) — A2 decision record, codegen pipeline,
  bundle-size budget, instructions for adding new bundles.
• docs/filter-ir.md — Python references replaced with the Node
  evaluator; NodeTool@0 step added; scripts.zip distribution
  documented.
• AGENTS.md — directory tree + tech stack + docs index updated.
• ado-script-design.md (new at repo root) — original design
  walkthrough that produced the A2 decision.

Deleted

• scripts/gate-eval.py (388 lines)
• scripts/gate-spec.schema.json (regenerable; the workspace owns
  its own schema/ copy via codegen)
• tests/gate_eval_tests.py (45 cases, all ported to vitest)

Behavior parity

The TypeScript port is a 1:1 reimplementation. Confirmed parity for
each of the 45 deleted Python test cases (see
scripts/ado-script/src/gate/__tests__/ports/INVENTORY.md). Two
intentional minor deviations are documented inline:

• pr_is_draft returns undefined when pr_metadata is missing
  (Python returned the literal string "unknown"). Topological
  ordering + the policy tracker prevent the divergence from being
  observable.
• The PolicyTracker collapses simultaneous fail_closed +
  skip_dependents references to "skip" for cleaner verdicts. The
  Python evaluator set should_run = false globally on fail_closed
  but produced functionally equivalent output for the realistic fact
  dependency graph.

Drift prevention

The ado-script CI workflow runs npm run codegen and then
git diff --exit-code -- scripts/ado-script/src/shared/types.gen.ts.
If a contributor changes Fact/Predicate/GateSpec shapes in Rust
without regenerating, CI fails with a clear remediation message.

Out of scope (explicit non-goals)

• A user-facing ado-script: front-matter block letting authors run
  arbitrary TypeScript at pipeline runtime (Variant B in the design
  doc). Separate RFC if pursued.
• Migrating safe-output executors or the agent-stats parser to Node.

Test plan

Local validation (all green):

# Rust
cargo build
cargo test
cargo clippy --all-targets --all-features

# TypeScript workspace
cd scripts/ado-script
npm ci
npm run codegen   # regenerate types.gen.ts; should be a no-op diff
npm test          # 173/173 passing
npm run typecheck
npm run build     # produces 1,149,604-byte gate.js

# E2E (requires gate.js to be built)
cd ../..
cargo test --test gate_e2e -- --ignored

CI validates the same flow plus the codegen drift check.

Manual reviewer checklist:

• Look at the emitted YAML diff for an existing PR-filter agent
  and confirm only the python3 → node switch +
  NodeTool@0 prepend changed.
• Inspect INVENTORY.md for the 1:1 mapping of deleted Python
  tests to their vitest counterparts.
• Skim docs/ado-script.md to confirm the decision rationale
  reads coherently.

Note on diff scope

A handful of unrelated Rust files were rustfmt-reformatted by
sub-agents during implementation. Those reflows have been reverted
out of this commit so the diff stays surgical to the migration. If
desired, a follow-up chore: rustfmt the tree PR can land them
separately.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid migration with good test coverage and a clean codegen pipeline. Two actionable issues worth addressing before merge, plus one stale doc string.

---

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/shared/policy.ts:8-12 — FACT_DEPS is not covered by the drift-check CI

  FACT_DEPS is a manually-maintained duplicate of Fact::dependencies() in Rust. If a contributor adds a new derived fact (e.g. a hypothetical pr_author that depends on pr_metadata) in filter_ir.rs without updating FACT_DEPS, the TypeScript PolicyTracker will silently skip transitive-skip propagation for that fact, causing incorrect gate verdicts. The existing drift-check in ado-script.yml only guards types.gen.ts (the schema boundary) — not the dependency graph.

  The cleanest fix is to include the dependency graph in the generated schema: add a dependencies: Vec<String> field to FactSpec in Rust, populate it from Fact::dependencies().iter().map(|f| f.kind()).collect(), re-run codegen, and read it in policy.ts at construction time instead of the hardcoded constant. That would bring FACT_DEPS under the same drift check as everything else. The comment in the file already acknowledges the risk ("keep in sync with Fact::dependencies"); this is a suggestion to close the gap mechanically.

⚠️ Suggestions

• release.yml:64-67 — scripts.zip ships the full TypeScript source tree

  The zip exclusion list removes node_modules/, dist/, and schema/ but leaves ado-script/src/, all test files, package.json, package-lock.json, tsconfig.json, and README.md in the release artifact. At pipeline runtime the only file extracted from the zip that is actually used is gate.js (already at scripts/gate.js). The additional entries are noise that grows with each new bundle. Recommend adding:

  -x "ado-script/src/*" \
  -x "ado-script/test/*" \
  -x "ado-script/package*.json" \
  -x "ado-script/tsconfig.json" \
  -x "ado-script/README.md" \
  -x "ado-script/.gitignore"

  Not a blocker (correct behaviour is unaffected), but worth doing before the first release ships the inflated artifact.

• src/compile/filter_ir.rs:814 — Stale "Python" doc on GateSpec

  /// Serializable gate specification — the JSON document consumed by the
  /// Python gate evaluator at pipeline runtime.
  

  This propagates into scripts/ado-script/src/shared/types.gen.ts via codegen. Should read "Node gate evaluator" (or simply drop the implementation reference).

✅ What Looks Good

• The vso-logger.ts escaping is correct: property values (setOutput name + value) escape %, \r, \n, ], ;; message bodies escape %, \r, \n — matches ADO's logging command spec.
• GATE_SPEC decoded from base64-compiled-in JSON: no user-controlled strings reach the VSO command emitters through an unescaped path.
• is_pipeline_var correctly gated behind #[cfg(test)] — only called inside the test module.
• The codegen drift check for types.gen.ts is well-structured; CI failure message includes the exact remediation command.
• withRetry in ado-client.ts only retries on 5xx, not on auth/validation errors — avoids amplifying bad requests.
• The #[ignore]'d E2E test gracefully degrades (soft-skip) when node is absent from PATH rather than hard-failing, which keeps cargo test clean in environments without Node.

Generated by Rust PR Reviewer for issue #389 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured migration with solid test coverage and drift prevention. Three issues worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/gate/predicates.ts line ~161 — The default: return true branch in evaluatePredicate silently passes unknown predicate types. If a consumer pins gate.js to an older release while using a newer compiler that adds a new predicate, filters would silently evaluate to "pass" rather than failing visibly. The codegen drift-check in CI prevents this during development, but not at deployment time. At minimum this should emit logWarning(unknown predicate type: ${p.type}) before returning true, so the failure shows up in pipeline logs.

⚠️ Suggestions

• scripts/ado-script/src/gate/predicates.ts — globMatch — The regex builder escapes [ and ] as literal characters (\[, \]), so a glob pattern src/[abc].ts would match only the literal string src/[abc].ts instead of src/a.ts etc. Python's fnmatch.fnmatch supports [seq] bracket expressions. This divergence is almost certainly harmless in practice since the IR doesn't emit bracket patterns, but it should be explicitly documented (a comment in globMatch or in the parity deviations list in the PR description) rather than silently omitted from the Python-parity inventory.

• .github/workflows/release.yml — The new setup-node@v4 step in the release job is missing cache: "npm" (compare to the CI workflow which has it). Every release build will therefore re-download the entire node_modules from the network. Not a correctness issue but it adds ~30s+ to every release run and is inconsistent with the CI workflow. Suggest adding:

  cache: "npm"
  cache-dependency-path: scripts/ado-script/package-lock.json

• src/compile/filter_ir.rs line ~136 — Fact::is_pipeline_var() is now #[cfg(test)]-gated without explanation. Since isPipelineVarFact in env-facts.ts serves the same purpose at runtime, this makes sense, but a brief comment (e.g. // only needed for test assertions; TypeScript mirror: isPipelineVarFact in env-facts.ts) would clarify the intent for future readers.

✅ What Looks Good

• VSO logging injection protection — escapeProperty and escapeMessage in vso-logger.ts correctly sanitize %, \r, \n, ], ; before embedding values in ##vso[...] commands. The typed CompleteResult union prevents injection through the result= field.
• Self-cancel is correctly best-effort — selfCancelIfRequested swallows API errors with a logWarning so a cancellation failure doesn't abort the gate step itself.
• Drift prevention is solid — The export-gate-schema → json-schema-to-typescript → git diff --exit-code pipeline is a clean, low-friction mechanism to catch Rust/TypeScript type drift in CI.
• Retry scope is appropriate — withRetry in ado-client.ts retries only on transient 5xx errors with a single 1-second backoff, which avoids hammering on persistent failures.
• export-gate-schema is correctly hidden — #[command(hide = true)] on the subcommand keeps it out of the user-facing --help while still being runnable by the codegen script.
• Test assertions tightened — the compiler_tests.rs change from content.contains("python3") to content.contains("node '/tmp/ado-aw-scripts/gate.js'") is strictly better; the old assertion would silently pass even if the gate step wasn't generated because python3 appeared elsewhere in the YAML (e.g. the mcpg-config validation step).

Generated by Rust PR Reviewer for issue #389 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean migration with solid test coverage. A few stale docstrings and one ignored test writing to a now-deleted file are worth cleaning up.

---

Findings

⚠️ Suggestions

• src/compile/filter_ir.rs:920–923 — Stale docstring on generate_gate_spec_schema()
  The doc comment still says "Python evaluator" and "should be shipped in scripts/gate-spec.schema.json alongside the evaluator". The file is now deleted and codegen is owned by the TS workspace. The comment should be updated to reflect the Node evaluator and point at scripts/ado-script/schema/ (or just say "generated by npm run codegen").

• src/compile/filter_ir.rs:938 — Stale doc comment on Fact::ado_exports()
"Returns ... pairs that must be exported in the bash shim for the **Python** evaluator to read." → should say Node evaluator.

• src/compile/filter_ir.rs:1826–1839 — #[ignore]'d test writes to a deleted file
  test_write_schema_to_scripts writes to scripts/gate-spec.schema.json, which this PR deletes. Running cargo test test_write_schema -- --ignored will silently recreate an untracked file and leave the tree dirty. Since codegen is now fully owned by npm run codegen (with the CI drift-check as the enforcement point), this test can either be removed or updated to write to scripts/ado-script/schema/gate-spec.schema.json so it stays in sync with where the TS workspace actually reads it from.

• src/compile/extensions/trigger_filters.rs:99–107 — NodeTool step embedded as an escaped string literal
  All other step strings use format!(r#"..."#, ...) raw string syntax. The NodeTool step is the only one using a bare string with escaped \" and \n. Minor, but inconsistent and harder to diff. A r#"- task: NodeTool@0\n inputs:\n versionSpec: "20.x"\n..."# raw literal (or the indoc! pattern used elsewhere) would match the surrounding style.

• scripts/ado-script/src/shared/ado-client.ts:47 — _repoId silently dropped
  getPullRequestById(project, _repoId, prId) discards repoId because the ADO SDK's git.getPullRequestById(prId, project) resolves org-wide by PR ID. That's correct, but a one-line comment explaining it ("repoId not required; ADO resolves by org-level PR ID") would prevent future confusion when someone sees the mismatch with the sibling functions.

• tests/compiler_tests.rs — Integration tests don't assert NodeTool@0 presence
  The two integration-test assertions check for node '/tmp/ado-aw-scripts/gate.js' (lines 3512, 3550) but not for the NodeTool@0 install step that now precedes it. The unit tests in trigger_filters.rs cover this, so it's not a gap in confidence — just worth noting if coverage breadth is a goal.

---

✅ What Looks Good

• Drift prevention is airtight: npm run codegen + git diff --exit-code on types.gen.ts in CI is the right approach — hard to accidentally break the Rust ↔ TypeScript contract.
• vso-logger.ts escaping: Properties escape %, \r, \n, ], ;; message bodies escape %, \r, \n. All runtime-user-controlled values (error messages, fact acquisition failures) flow through escapeMessage before hitting stdout. CompleteResult is a closed union type so the result= property value is never user-controlled.
• #[cfg(test)] narrowing of is_pipeline_var(): Correct — the method was only needed for test assertions; the evaluator carries its own mirror via env-facts.ts::isPipelineVarFact.
• dependencies field in FactSpec: Serialising the fact dependency graph into the gate spec is the right call — eliminates the need for the TS evaluator to duplicate the topological-sort logic.
• Release zip exclusions work correctly: Verified — on Linux, zip -r . -x "ado-script/src/*" does correctly exclude the subtree.
• gate_e2e.rs #[ignore] guard with early-return for missing gate.js/node: Proper skip-not-fail semantics for an opt-in E2E test that requires pre-built artefacts.

Generated by Rust PR Reviewer for issue #389 · ● 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid migration overall — the architecture is clean and the 45-case parity inventory gives good confidence. Two issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/shared/vso-logger.ts — setOutput over-escapes the variable value

  export function setOutput(name: string, value: string): void {
    const safeName = escapeProperty(name);
    const safeValue = escapeProperty(value);  // ← wrong helper for the body position
    emit(`##vso[task.setvariable variable=${safeName};isOutput=true]${safeValue}`);
  }

  The value sits in the message body (after the closing ]), not in the properties section. escapeProperty escapes ] → %5D and ; → %3B, but ADO does not URL-decode the message body — so a value containing either character would be stored verbatim as %5D/%3B instead of the literal. The correct helper is escapeMessage (which only escapes %, \r, \n).

  Today's only call passes "true"/"false", so it's latent, but it's semantically wrong and will silently corrupt values if setOutput is ever reused for richer output. One-line fix: const safeValue = escapeMessage(value);

⚠️ Suggestions

• .github/workflows/ado-script.yml — trigger paths don't include trigger_filters.rs

  The workflow's paths: filter covers scripts/ado-script/**, src/compile/filter_ir.rs, Cargo.toml, and Cargo.lock, but not src/compile/extensions/trigger_filters.rs. That file is what emits the NodeTool@0 step and wires GATE_EVAL_PATH into the pipeline YAML. A change there (e.g., adjusting the Node version spec or the gate step condition) won't re-trigger the E2E test. Consider adding "src/compile/extensions/trigger_filters.rs" to the paths list.

✅ What Looks Good

• Dependency graph carried in the spec — FactSpec.dependencies now ships in the base64 GATE_SPEC, so PolicyTracker reads the graph from the spec rather than duplicating the Rust Fact::dependencies() logic in TypeScript. Clean and drift-proof.
• globMatch escaping — the two-pass escape (all regex metacharacters first, then selectively unescape \*/\?) is correct; bracket expressions are correctly reduced to literals as documented.
• PolicyTracker transitive propagation — the fixpoint BFS correctly marks downstream facts unavailable and the verdictForMissingFacts logic handles all three policy combinations (fail_closed, fail_open, skip_dependents) cleanly.
• #[cfg(test)] on is_pipeline_var — good dead-code hygiene; the runtime mirror lives where it should.
• Drift check — git diff --exit-code on types.gen.ts is a clean enforcement mechanism. The error message pointing to npm run codegen is exactly the right remediation hint.
• Release zip exclusions — excluding ado-script/node_modules, dist/, src/, etc. from scripts.zip keeps the artifact lean and ships only gate.js as intended.

Generated by Rust PR Reviewer for issue #389 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid migration — architecture is well-designed and the security-sensitive paths are handled carefully. Two items worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• test_update_schema_file writes to a deleted path (src/compile/filter_ir.rs, tests::test_update_schema_file): This test writes to scripts/gate-spec.schema.json — the exact file this PR deletes. Running cargo test after merge will silently recreate it as an untracked artifact, which is confusing and inconsistent with the new canonical location (scripts/ado-script/schema/). The test should either be removed (the export round-trip is now covered by tests/export_gate_schema.rs) or updated to write to the workspace path.

⚠️ Suggestions

• Inline NodeTool YAML step is fragile (src/compile/extensions/trigger_filters.rs:105): The step is a single string with embedded \n escapes and inline \" quoting — the indentation is invisible until you run the code. A raw string block would make it auditable at a glance and harder to misalign:

  steps.push(indoc::indoc! {r#"
      - task: NodeTool@0
        inputs:
          versionSpec: "20.x"
        displayName: "Install Node.js 20.x for gate evaluator"
        condition: succeeded()"#}.to_string());

  (or just a multiline raw string literal if indoc isn't already a dep). The existing download step uses format!(r#"..."#) — the NodeTool step should be consistent.

• globMatch silently diverges from fnmatch on bracket expressions (scripts/ado-script/src/gate/predicates.ts:153): The comment documents this, but it's a forward-compat trap: if a future Rust IR change emits a [abc]-style glob, the gate will match it literally instead of as a character class, causing silent mismatch. Consider adding a throw/logWarning guard if the incoming pattern contains [ (since the IR currently never emits them, this guard would never fire in practice but would catch the drift early).

✅ What Looks Good

• VSO command escaping (vso-logger.ts): Property values escape %, \r, \n, ], ;; message bodies escape %, \r, \n. This matches ADO's logging-command parser contract. CompleteResult being a TypeScript union type prevents the result= field from ever being user-controlled.
• FactSpec.dependencies in the spec (filter_ir.rs:836+, policy.ts): Carrying the dependency graph in the serialised spec so the TypeScript evaluator doesn't have to mirror Rust's Fact::dependencies() is a clean design. The markUnavailableTransitive BFS correctly propagates fail_closed/skip_dependents across the graph.
• Drift prevention is tight: codegen + git diff --exit-code on types.gen.ts in CI is the right mechanism; the ExportGateSchema hidden subcommand makes it automatable without exposing implementation details.
• #[cfg(test)] on is_pipeline_var(): Correctly scoped now that the runtime mirror lives in TypeScript; eliminates a dead-code path in the production binary.
• anyhow::Result propagation: All new Rust code follows the project convention; ExportGateSchema handler correctly uses ? for I/O and create_dir_all for missing parent directories.

Generated by Rust PR Reviewer for issue #389 · ● 1.7M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean migration with solid test coverage, good security posture, and a clever drift-prevention mechanism. A couple of minor items below.

---

Findings

⚠️ Suggestions

• scripts.zip exclusion list may be incomplete (release.yml)
  The zip -x patterns cover the main source directories and known config files, but any other files at the ado-script/ root that aren't explicitly listed (e.g. future ncc.config.js, .npmrc, etc.) would silently sneak into the release artifact. A simpler approach that closes the gap permanently would be to zip only gate.js explicitly:

  zip ../scripts.zip gate.js

  ...instead of zipping . and excluding everything you don't want.

• ado-script.yml CI has no push: main trigger
  The workflow only fires on pull_request. If a future commit lands on main that touches filter_ir.rs without going through a PR (direct push, repo admin merge), the codegen drift check won't run post-merge. Adding a push: branches: [main] with the same paths: filter closes the gap at low cost.

• changed_file_count returns 0 on dependency failure (gate/facts.ts:109)

  case "changed_file_count": {
    const cf = facts.get("changed_files");
    return Array.isArray(cf) ? cf.length : 0;  // silent fallback
  }

  If changed_files somehow ends up absent from facts despite the dependency graph (e.g. a future fact addition where the dependency in Rust is missing), this silently returns 0 rather than undefined, so tracker.recordFactFailure is never called for it. The predicate would then incorrectly evaluate changed_file_count against 0. A defensive return undefined instead of : 0 in the non-array branch would surface the bug earlier. Low risk given the Rust-side dependency list should prevent this.

---

✅ What Looks Good

• No user input reaches the YAML output unsanitized. All display_name, step_name, tag_prefix, bypass_label, and build_tag_suffix values are &'static str enum variants — the gate spec carries only hardcoded strings plus user patterns/values, and those are base64-encoded before landing in the YAML env: block, preventing YAML and ADO logging command injection entirely.

• vso-logger.ts escaping is correct. Property values escape \r, \n, ], ;; message bodies escape %, \r, \n. The test coverage in vso-logger.test.ts exercises the boundary cases well.

• Unknown predicate types fail-closed with a VSO warning rather than silently auto-passing — good safety net against compiler/evaluator version skew.

• Drift prevention design is solid. Codegen CI (export-gate-schema → json-schema-to-typescript) plus git diff --exit-code makes the schema contract between Rust IR and TypeScript explicit and machine-checked.

• #[cfg(test)] on Fact::is_pipeline_var is the right call; the comment accurately points reviewers to the TypeScript mirror in env-facts.ts.

• withRetry in ado-client.ts is appropriately scoped to 5xx transient errors with a single retry, avoiding retry-storm behaviour on auth failures or bad inputs.

Generated by Rust PR Reviewer for issue #389 · ● 1.9M · ◷