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

[jamesadevine]

@copilot rebadge scripts.zip as ado-script. Just directly upload the build ado-script folder

[Copilot]

@copilot rebadge scripts.zip as ado-script. Just directly upload the build ado-script folder

Done in cee11de. Release now uploads ado-script.zip built directly from scripts/ado-script/dist, and the runtime download/execution path was updated to use /tmp/ado-aw-scripts/ado-script/dist/gate/index.js.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• spsprodeus21.vssps.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.15/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.174glxxlpjt0xva2ers8pcnsy.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.17qig6dqi1vaxf7w9k7w72xz2.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.17t5u6hjlciq3wj3trc2v3rw1.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1n/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1874yj5l85ja3hxa01327lvnq.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1y/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.18poqkg65qt8eta7vcydyfuoi.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1z/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1n830tgrl16khgk2xz7gqn7na.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.2i/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1r0sp8te3e22mzmys1esgxx0e.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.2l/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.21gzclk3npjexmnoo1vn9bzhw.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.3k9kub�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.4abk5kr7uoq26swnw4e5ggr6n.17myluh.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.4nd1ka0wouej0vox3djhx8uyp.17myluh.rcgu.o g/de�� 1a.rlib e-f30486154d86f6--json=diagnostic-rendered-ansi,artifacts,future-incompat bin/rustc e-c4448fe3a5c46ecc e-c4448fe3a5c46e-m64 e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustc8n0y4m/symbols.o bin/rustc (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 --check-cfg cfg(feature, values()) (dns block)
• spsprodweu4.vssps.visualstudio.com
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.15/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.174glxxlpjt0xva2ers8pcnsy.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.17qig6dqi1vaxf7w9k7w72xz2.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.17t5u6hjlciq3wj3trc2v3rw1.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1n/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1874yj5l85ja3hxa01327lvnq.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1y/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.18poqkg65qt8eta7vcydyfuoi.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.1z/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1n830tgrl16khgk2xz7gqn7na.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.2i/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.1r0sp8te3e22mzmys1esgxx0e.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.2l/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/mcp_http_tests-aad5290340e51baf.21gzclk3npjexmnoo1vn9bzhw.1hzb12w.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.3k9kub�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.4abk5kr7uoq26swnw4e5ggr6n.17myluh.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/init_tests-67f82af7c02abdef.4nd1ka0wouej0vox3djhx8uyp.17myluh.rcgu.o g/de�� 1a.rlib e-f30486154d86f6--json=diagnostic-rendered-ansi,artifacts,future-incompat bin/rustc e-c4448fe3a5c46ecc e-c4448fe3a5c46e-m64 e-c4448fe3a5c46e/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustc8n0y4m/symbols.o bin/rustc (dns block)
  • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-f4dc8202a0574181 --check-cfg cfg(feature, values()) (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[jamesadevine]

@copilot rebadge scripts.zip as ado-script. Just directly upload the build ado-script folder

Done in cee11de. Release now uploads ado-script.zip built directly from scripts/ado-script/dist, and the runtime download/execution path was updated to use /tmp/ado-aw-scripts/ado-script/dist/gate/index.js.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

@copilot can we just upload the dist folder directly as ado-script? Say no if it is a bad idea. Zip is fine.

[Copilot]

@copilot rebadge scripts.zip as ado-script. Just directly upload the build ado-script folder

Done in [cee11de](cee11de...

No—keep the zip. GitHub release assets are files, not directories, so we can’t upload a raw folder as an asset. ado-script.zip is the right format for distribution and download in the pipeline.

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean architecture with strong drift prevention, but a few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• predicates.ts:17 — BRANCH_FACTS duplicated from env-facts.ts: predicates.ts defines its own BRANCH_FACTS constant independently of the canonical one in env-facts.ts. This constant controls whether stripRefPrefix is applied when evaluating glob_match predicates. If a new branch fact is added to env-facts.ts but not mirrored here, fact values will have the refs/heads/ prefix stripped at read time but NOT at pattern-match time — causing patterns like feature/* to silently fail for the new fact kind. Suggest importing/re-exporting from env-facts.ts instead.

• facts.ts:96–99 — changed_files path stripping only strips leading /: ADO iteration-change paths come back as /foo/bar.ts. The code strips leading / correctly, but paths with a different prefix form (e.g., on non-git repos) could produce empty entries that filter((p) => !!p) removes silently. Low-risk but worth a comment documenting the expected ADO path format.

🔒 Security Concerns

• vso-logger.ts:35 — escapeProperty does not encode = or space: setOutput emits ##vso[task.setvariable variable=${safeName};isOutput=true]${safeValue}. If safeName contains = or ;, the command header becomes ambiguous. Currently only called with the hardcoded string "SHOULD_RUN", so not exploitable today. But as a shared utility this is a latent injection path — worth hardening before this module grows more callers.

• bypass.ts:14 — spec.context.bypass_label written to stdout unescaped: The plain process.stdout.write call skips vso-logger's escaping entirely. bypass_label is compiler-controlled (e.g., "Pull Request"), but a user-supplied agent name that feeds the label through the compiler's template could embed ##vso[ in the line. Replacing with logWarning(...) or emit(...) via the vso-logger would be consistent and safe.

⚠️ Suggestions

• auth.ts:22 — _resetCacheForTesting is a test-seam leak: Exporting a reset function for test teardown is a code smell that pollutes the public API. Dependency injection (accepting a WebApi factory parameter) or test-mocking the module at the vitest level would be cleaner and prevent accidental calls in production.

• filter_ir.rs:138 — Severity::Info gated with #[allow(dead_code)]: The enum variant is intentionally kept but unused. If it's part of a public diagnostic API, keeping it makes sense. If it's truly not needed, removing it is cleaner. #[allow(dead_code)] on a variant in a private-use enum is fine but should have a comment explaining why.

• trigger_filters.rs — NodeTool@0 step has no timeoutInMinutes: This mirrors the existing download step (also no timeout), but for a Node install step on a cold image, a hung package install could block the pipeline indefinitely. Adding timeoutInMinutes: 5 (or whatever the project standard is) to both the Node install and download steps would be more resilient.

• ado-script.yml — workflow only triggers on pull_request: No push: branches: [main] trigger means drift on main won't be detected post-merge (e.g., if someone force-pushes or cherry-picks a Rust IR change without regenerating types). Adding a main-push trigger would close this gap.

✅ What Looks Good

• JSON Schema codegen + drift-check (git diff --exit-code on types.gen.ts in CI) is excellent engineering — eliminates the entire class of compiler/evaluator type-contract drift bugs.
• vso-logger.ts properly escapes %, \r, \n, ], ; in property values and %, \r, \n in message bodies — no ADO logging command injection possible through the module's public API (current callers).
• PolicyTracker carries the dependency graph in the spec itself (FactSpec.dependencies) rather than duplicating the Rust topological logic in TypeScript — smart design that prevents drift at the cost of a slightly larger GATE_SPEC payload.
• fail_closed default for unknown predicate types (predicates.ts:162–173) is the correct safe default; the warning message is actionable and points to the right remediation.
• SHA256 checksum verification before extraction in the download step provides meaningful supply-chain integrity even though the URL is pinned.
• #[cfg(test)] on is_pipeline_var correctly removes a now-unnecessary production symbol without breaking tests.

Generated by Rust PR Reviewer for issue #389 · ● 884.5K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-engineered migration — the TypeScript port is clean and the Rust side is surgical. Two actionable items below, both low-to-medium severity.

---

Findings

⚠️ Suggestions

trigger_filters.rs:L122-125 — Zip-slip defense-in-depth regression

Old extraction was:

unzip -jo scripts.zip gate-eval.py

• -j (junk paths) prevented any ../ entries in the zip from writing outside the working directory
• Only the single known file was extracted

New extraction is:

unzip -o ado-script.zip

• Full archive extracted preserving the internal path structure
• No path-junk flag

This is necessary because the module tree is required at runtime, so -j can't be used as-is. However, some hardening options:

• unzip -o ado-script.zip -d /tmp/ado-aw-scripts is explicit about the target (already implicit via cd, but belt-and-suspenders)
• Alternatively, verify the zip's member paths don't escape the target prefix before extracting: zipinfo -1 ado-script.zip | grep -qP '^\.\.' && exit 1 — cheap and catches the pathological case

The SHA-256 verification before extraction provides strong mitigation (a tampered zip would have to also forge the checksums entry), so this is a defense-in-depth suggestion rather than a blocking concern.

---

env-facts.ts + filter_ir.rs:L136 — FactKind/ENV_BY_FACT drift not covered by codegen

When is_pipeline_var() was pub, it served as the canonical Rust-side source of truth that reviewers could visually cross-reference with the TS. Now that it's #[cfg(test)], the authoritative runtime list lives in two places:

1. Fact::ado_exports() in filter_ir.rs (Rust)
2. ENV_BY_FACT in env-facts.ts + FactKind type (TypeScript)

The codegen drift check in ado-script.yml only guards types.gen.ts (the GateSpec shape). It does not cover FactKind/ENV_BY_FACT. A new pipeline-variable Fact added to Rust without a matching entry in env-facts.ts would silently fail at runtime — readEnvFact returns undefined, acquireFacts calls tracker.recordFactFailure, and if the fact's failure policy is fail_open, the gate passes without actually checking the predicate. That's a quiet wrong-answer bug.

Options:

1. Add a comment on Fact::ado_exports() (or the FactKind type) naming the other file to update and linking this gap
2. Add a test in the TypeScript suite that enumerates the kinds emitted by export-gate-schema that are pipeline-variable facts (determinable from the spec's FactSpec.dependencies being empty + absence from the ADO-REST fact list) and asserts each has an entry in ENV_BY_FACT — though this requires a spec snapshot
3. Add a Rust integration test that runs export-gate-schema, parses the JSON, and asserts each fact kind that Fact::ado_exports() would return is present in a hard-coded expected set (fails loudly when a new fact is added without updating TS)

Options 1 alone is enough to prevent a silent miss in code review; the others provide automation.

---

filter_ir.rs:L351 — #[allow(dead_code)] on Severity::Info

Minor: if Info is never emitted, removing it is cleaner than suppressing the warning. If it's intended as future scaffolding, a // TODO comment makes the intent clearer than #[allow(dead_code)].

---

✅ What Looks Good

• SHA-256 before extraction: checksums.txt is fetched separately and verified before unzip runs — this is the right order and makes the zip-slip surface very hard to exploit in practice.
• validatePredicateTree pre-flight: catching unknown predicate types before fact acquisition prevents silent skip-on-missing-fact masking of version drift. Well-reasoned.
• PolicyTracker carries deps from spec: no duplicated dependency graph in the evaluator — Rust is the single source of truth, and the new FactSpec.dependencies field flows cleanly from Fact::dependencies() via build_gate_spec.
• Glob hardening: pattern-length cap, wildcard-count cap, warn-on-bracket, and an LRU-style cache all in one self-contained function with clear constant naming and comments. Notably fail-closed on cap violation.
• VSO logging escaping: escapeProperty (for command property slots) vs escapeMessage (for the body) used correctly throughout — setOutput name in the property slot uses escapeProperty, value in the body uses escapeMessage.
• Paginated getIterationChanges: silent truncation at 100 files was a correctness hazard in the Python version; the explicit TOP/SKIP pagination loop with a defensive page cap is the right fix.
• Lazy azure-devops-node-api import: startup latency optimization with a clear comment; ncc still bundles it so no runtime resolution cost.
• Drift check placement: ado-script.yml path filter includes src/compile/filter_ir.rs, so a Rust IR change that would invalidate types.gen.ts will always trigger the codegen+diff check.
• 173 tests, INVENTORY.md mapping, 1:1 parity documentation: thorough.

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

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — clean migration with thorough tests and solid security handling. One real maintenance hazard to flag.

Findings

⚠️ Suggestions

• ENV_BY_FACT drift gap is CI-unenforceable [scripts/ado-script/src/shared/env-facts.ts, src/compile/filter_ir.rs]

  The CI drift check only covers GateSpec schema shape (via types.gen.ts codegen). It does not cover the Fact::ado_exports() → ENV_BY_FACT mapping. Adding a new pipeline-variable Fact variant in Rust requires four manual updates: the Fact enum, ado_exports(), Fact::kind(), and ENV_BY_FACT in TypeScript. The fourth has no compiler or CI guard. The code's **Drift note:** docstring documents the risk clearly, but the failure mode is silent wrong-answer: readEnvFact returns undefined, the policy tracker fires fail_open, and the gate passes without ever checking the predicate.

  A low-cost mitigation: add a smoke test in env-facts.test.ts (or facts.test.ts) that cross-checks the ENV_BY_FACT key set against the kinds in a reference GateSpec fixture exported by cargo run -- export-gate-schema. This would make the gap testable without requiring runtime integration.

• withTimeout leaves a dangling Promise on timeout [scripts/ado-script/src/shared/ado-client.ts:56-69]

  When fn() exceeds the timeout, the TimeoutError is thrown but the original Promise is not (and cannot be) cancelled. On a transient-retry path, two concurrent in-flight ADO REST calls briefly coexist. For read calls (getPullRequestById, getPullRequestIterations, getIterationChanges) this is harmless. For cancelBuild — the only mutating call — two simultaneous PATCH status=Cancelling requests are idempotent, so it's safe in practice. Worth a brief comment in withTimeout noting the dangling-promise caveat so future callers (e.g. a poll.js bundle) don't inadvertently rely on cancellation semantics that don't exist.

• addBuildTag in predicates.ts:4511 passes check.tag_suffix through escapeMessage — escapeMessage encodes %, \r, \n but not ]. For addBuildTag the ] in the payload is safe (ADO's command parser already closed the header at the ] after build.addbuildtag), so this is fine today. Worth a brief comment distinguishing "header-escaping" from "body-escaping" for future maintainers — the asymmetry between escapeProperty (includes ]) and escapeMessage (omits ]) is non-obvious.

✅ What Looks Good

• Release packaging path is consistent end-to-end: cd scripts && zip -r ../ado-script.zip ado-script/dist → unzip -o ... -d /tmp/ado-aw-scripts/ → file lands at /tmp/ado-aw-scripts/ado-script/dist/gate/index.js which matches GATE_EVAL_PATH exactly. The sha256 verification also correctly checks ado-script.zip after cd /tmp/ado-aw-scripts.

• VSO command injection is handled correctly in vso-logger.ts: escapeProperty encodes %, \r, \n, ], ;, =, SPACE; escapeMessage handles %, \r, \n; logInfo additionally percent-encodes a leading #. The ordering (encode % first) prevents double-encoding bugs. The bypass.test.ts adversarial bypass_label test (##vso[task.complete result=Failed;]X\nY) validates the boundary.

• PolicyTracker dependency propagation correctly reads FactSpec.dependencies from the spec (no manual mirror of the Rust Fact::dependencies() DAG needed in TypeScript). The new dependencies: Vec<String> field on FactSpec carries the graph across the wire — elegant solution.

• Fact::is_pipeline_var() scoped to #[cfg(test)]: the public surface is correct — runtime classification lives entirely in isPipelineVarFact in TypeScript; no production Rust call-site needed.

• generate_gate_spec_schema() using expect(): acceptable for a build-time developer tool invoked by export-gate-schema; not on any user-facing runtime path.

• 173 vitest tests with 45 Python parity ports and the INVENTORY.md traceability log is a good standard to set for future bundle additions.

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