fix(ado-script): address PR review — drift-check fact deps; fail-closed unknown predicates

Closes review findings on #389:

1. FACT_DEPS drift gap: the fact dependency graph in policy.ts was a
   manually-maintained mirror of Fact::dependencies() in Rust, outside
   the schema drift check. Added `dependencies: Vec<String>` to
   FactSpec populated from Fact::dependencies(). Regenerated
   types.gen.ts. PolicyTracker now reads the dep graph from the spec
   at construction; the hardcoded FACT_DEPS constant is removed. The
   CI `git diff --exit-code` on types.gen.ts now covers the dep graph
   too.

2. Unknown predicate silent pass: the default arm of evaluatePredicate
   returned true (auto-pass), so a stale gate.js paired with a newer
   compiler that emits a new predicate would silently pass filters.
   Now: emits a task.logissue type=warning naming the unknown type
   and returns false (fail-closed). Added a regression test.

3. Stale "Python" doc on GateSpec: updated to "Node gate evaluator
   (scripts/gate.js)". Propagates through codegen.

4. scripts.zip ships TS source: added -x patterns for ado-script/src/,
   test/, package*.json, tsconfig.json, vitest.config*.ts, README.md,
   and .gitignore. Only gate.js (already at scripts/gate.js) and any
   future bundle artefacts are shipped.

5. globMatch bracket divergence: documented inline that bracket
   expressions [seq] are escaped to literal characters (Python fnmatch
   supported them). The IR currently never emits bracket patterns; if
   a future predicate needs them, the builder must be extended.

6. release.yml missing npm cache: added cache: "npm" and
   cache-dependency-path to match the CI workflow. Avoids
   re-downloading node_modules on every release run.

7. Fact::is_pipeline_var() #[cfg(test)] opacity: added a doc comment
   noting the runtime mirror is isPipelineVarFact in env-facts.ts, so
   future readers don't wonder why the helper exists only for tests.

Validation: 172/172 vitest tests, full cargo test suite, clippy clean,
e2e gate test passes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jamesadevine

.github/workflows/release.yml

@@ -59,6 +59,8 @@ jobs:
         uses: actions/setup-node@v4
         with:
           node-version: "20"
+          cache: "npm"
+          cache-dependency-path: scripts/ado-script/package-lock.json
 
       - name: Build ado-script TypeScript bundle (gate.js)
         working-directory: scripts/ado-script
@@ -73,10 +75,21 @@ jobs:
         run: |
           set -euo pipefail
           cd scripts
+          # Only ship the bundled artefacts that pipelines actually
+          # download and execute (currently: gate.js). The TS source,
+          # tests, and tooling under ado-script/ are excluded — they
+          # live in the repo for development and CI, not in the release.
           zip -r ../scripts.zip . \
             -x "ado-script/node_modules/*" \
             -x "ado-script/dist/*" \
-            -x "ado-script/schema/*"
+            -x "ado-script/schema/*" \
+            -x "ado-script/src/*" \
+            -x "ado-script/test/*" \
+            -x "ado-script/package*.json" \
+            -x "ado-script/tsconfig.json" \
+            -x "ado-script/vitest.config*.ts" \
+            -x "ado-script/README.md" \
+            -x "ado-script/.gitignore"
 
       - name: Upload release assets
         env:

scripts/ado-script/src/gate/__tests__/ports/integration.test.ts

@@ -23,7 +23,7 @@ describe("evaluatePredicates integration ports", () => {
     const predicate = { type: "equals", fact: "pr_title", value: "ok" } satisfies PredicateSpec;
     const spec = gateSpec(
       [{ name: "title", tag_suffix: "title", predicate }],
-      [{ kind: "pr_title", failure_policy: "fail_closed" }],
+      [{ kind: "pr_title", failure_policy: "fail_closed", dependencies: [] }],
     );
     const tracker = new PolicyTracker(spec.facts);
 

scripts/ado-script/src/gate/facts.test.ts

@@ -30,7 +30,7 @@ vi.mock("../shared/ado-client.js", () => ({
 import { acquireFacts } from "./facts.js";
 
 function fact(kind: string, failure_policy = "fail_closed"): FactSpec {
-  return { kind, failure_policy };
+  return { kind, failure_policy, dependencies: [] };
 }
 
 function gateSpec(facts: FactSpec[]): GateSpec {

scripts/ado-script/src/gate/predicates.test.ts

@@ -350,6 +350,34 @@ describe("predicateFacts", () => {
   });
 });
 
+describe("unknown predicate fallback", () => {
+  let stderrWrites: string[];
+
+  beforeEach(() => {
+    stderrWrites = [];
+    vi.spyOn(process.stdout, "write").mockImplementation((chunk: any) => {
+      stderrWrites.push(typeof chunk === "string" ? chunk : chunk.toString());
+      return true;
+    });
+  });
+
+  afterEach(() => vi.restoreAllMocks());
+
+  it("fails closed and emits a warning when the predicate type is unknown", () => {
+    // Forge a spec a future compiler might emit but this gate.js does
+    // not recognise. The default branch should NOT silently auto-pass.
+    const pred = { type: "future_predicate", fact: "x" } as unknown as PredicateSpec;
+    const result = evaluatePredicate(pred, factMap({}));
+    expect(result).toBe(false);
+    expect(
+      stderrWrites.some(
+        (w) =>
+          w.includes("##vso[task.logissue type=warning;]") && w.includes("future_predicate"),
+      ),
+    ).toBe(true);
+  });
+});
+
 describe("evaluatePredicates", () => {
   let writes: string[];
 
@@ -380,8 +408,8 @@ describe("evaluatePredicates", () => {
         },
       ],
       [
-        { kind: "pr_title", failure_policy: "fail_closed" },
-        { kind: "build_reason", failure_policy: "fail_closed" },
+        { kind: "pr_title", failure_policy: "fail_closed", dependencies: [] },
+        { kind: "build_reason", failure_policy: "fail_closed", dependencies: [] },
       ],
     );
     const tracker = new PolicyTracker(spec.facts);
@@ -406,7 +434,7 @@ describe("evaluatePredicates", () => {
           predicate: { type: "equals", fact: "pr_title", value: "ok" },
         },
       ],
-      [{ kind: "pr_title", failure_policy: "fail_closed" }],
+      [{ kind: "pr_title", failure_policy: "fail_closed", dependencies: [] }],
     );
     const tracker = new PolicyTracker(spec.facts);
     tracker.recordFactFailure("pr_title", "test failure");

scripts/ado-script/src/gate/predicates.ts

@@ -7,7 +7,7 @@
  */
 import type { GateSpec, PredicateSpec } from "../shared/types.gen.js";
 import type { PolicyTracker } from "../shared/policy.js";
-import { addBuildTag } from "../shared/vso-logger.js";
+import { addBuildTag, logWarning } from "../shared/vso-logger.js";
 import { stripRefPrefix } from "../shared/env-facts.js";
 
 type CheckResult = "pass" | "fail" | "skip";
@@ -138,8 +138,18 @@ export function evaluatePredicate(p: PredicateSpec, facts: Map<string, unknown>)
       return p.operands.some((sub) => evaluatePredicate(sub, facts));
     case "not":
       return !evaluatePredicate(p.operand, facts);
-    default:
-      return true;
+    default: {
+      // Unknown predicate type — likely a newer compiler emitted a spec
+      // a bundled gate.js doesn't recognise. Surface in pipeline logs;
+      // fail-closed so the missing logic doesn't silently auto-pass.
+      const unknownType = (p as { type?: unknown }).type;
+      logWarning(
+        `Unknown predicate type '${String(unknownType)}'; failing closed. ` +
+          "Update scripts/gate.js (or the bundled scripts.zip) to a " +
+          "release that supports this predicate.",
+      );
+      return false;
+    }
   }
 }
 
@@ -152,6 +162,12 @@ function stringsFromFact(raw: unknown): string[] {
 }
 
 function globMatch(value: string, pattern: string): boolean {
+  // Glob → regex: only `*` (any chars) and `?` (single char) are
+  // recognised. Bracket expressions like `[abc]` are escaped to literal
+  // characters here. This is a deliberate divergence from Python's
+  // `fnmatch.fnmatch`, which supports `[seq]` ranges. The IR currently
+  // never emits bracket patterns, but if a future predicate needs them,
+  // this builder must be extended (and the parity inventory updated).
   const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
   const regex = `^${escaped.replace(/\\\*/g, ".*").replace(/\\\?/g, ".")}$`;
   return new RegExp(regex, "s").test(value);

scripts/ado-script/src/shared/__tests__/policy.test.ts

@@ -2,8 +2,21 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { PolicyTracker } from "../policy.js";
 import type { FactSpec } from "../types.gen.js";
 
-function spec(kind: string, fp: string): FactSpec {
-  return { kind, failure_policy: fp };
+// Mirror of the real Fact::dependencies() graph. Tests construct
+// FactSpec objects with this map so the dep graph stays implicit at
+// the call sites (matches how the compiler emits it in production).
+const DEFAULT_DEPS: Record<string, readonly string[]> = {
+  pr_is_draft: ["pr_metadata"],
+  pr_labels: ["pr_metadata"],
+  changed_file_count: ["changed_files"],
+};
+
+function spec(kind: string, fp: string, dependencies?: string[]): FactSpec {
+  return {
+    kind,
+    failure_policy: fp,
+    dependencies: dependencies ?? [...(DEFAULT_DEPS[kind] ?? [])],
+  };
 }
 
 describe("PolicyTracker", () => {

scripts/ado-script/src/shared/policy.ts

@@ -3,17 +3,10 @@ import { logWarning } from "./vso-logger.js";
 
 export type FailurePolicy = "fail_closed" | "fail_open" | "skip_dependents";
 
-/** Hard-coded fact dependency graph — keep in sync with `Fact::dependencies` in
- *  `src/compile/filter_ir.rs`. Encoded here because `FactSpec` does not carry
- *  the dep graph; it's a property of the kind, not the spec instance. */
-const FACT_DEPS = {
-  pr_is_draft: ["pr_metadata"],
-  pr_labels: ["pr_metadata"],
-  changed_file_count: ["changed_files"],
-} as const satisfies Record<string, readonly string[]>;
-
 export class PolicyTracker {
   private readonly policyByKind = new Map<string, FailurePolicy>();
+  /** Fact dependency graph derived from the spec — no manual mirror. */
+  private readonly depsByKind = new Map<string, readonly string[]>();
   private readonly failedFacts = new Set<string>();
   private readonly skippedFacts = new Set<string>();
   private readonly unavailablePoliciesByKind = new Map<string, Set<FailurePolicy>>();
@@ -24,6 +17,7 @@ export class PolicyTracker {
   constructor(facts: FactSpec[]) {
     for (const f of facts) {
       this.policyByKind.set(f.kind, this.parsePolicy(f.failure_policy));
+      this.depsByKind.set(f.kind, f.dependencies ?? []);
     }
   }
 
@@ -108,7 +102,7 @@ export class PolicyTracker {
     let changed = true;
     while (changed) {
       changed = false;
-      for (const [kind, deps] of Object.entries(FACT_DEPS)) {
+      for (const [kind, deps] of this.depsByKind.entries()) {
         if (unavailable.has(kind)) continue;
         if (deps.some((dep) => unavailable.has(dep))) {
           unavailable.add(kind);
@@ -132,5 +126,3 @@ export class PolicyTracker {
     this.unavailablePoliciesByKind.set(factKind, new Set([policy]));
   }
 }
-
-export const _internalForTest = { FACT_DEPS };

scripts/ado-script/src/shared/types.gen.ts

@@ -76,7 +76,7 @@ export type PredicateSpec =
 
 /**
  * Serializable gate specification — the JSON document consumed by the
- * Python gate evaluator at pipeline runtime.
+ * Node gate evaluator (`scripts/gate.js`) at pipeline runtime.
  */
 export interface GateSpec {
   checks: CheckSpec[];
@@ -107,6 +107,12 @@ export interface GateContextSpec {
  * Serialized fact acquisition descriptor.
  */
 export interface FactSpec {
+  /**
+   * Kinds of other facts that must be acquired before this one.
+   * Mirrors `Fact::dependencies()`. Carried in the spec so the gate
+   * evaluator does not duplicate the dependency graph.
+   */
+  dependencies: string[];
   failure_policy: string;
   kind: string;
   [k: string]: unknown;

src/compile/filter_ir.rs

@@ -136,6 +136,8 @@ impl Fact {
     }
 
     /// True if this fact is a free pipeline variable (no API/computation).
+    /// Only used for test assertions; the runtime evaluator has its own
+    /// mirror in `scripts/ado-script/src/shared/env-facts.ts::isPipelineVarFact`.
     #[cfg(test)]
     pub fn is_pipeline_var(&self) -> bool {
         matches!(
@@ -812,7 +814,7 @@ use schemars::JsonSchema;
 use serde::Serialize;
 
 /// Serializable gate specification — the JSON document consumed by the
-/// Python gate evaluator at pipeline runtime.
+/// Node gate evaluator (`scripts/gate.js`) at pipeline runtime.
 #[derive(Debug, Clone, Serialize, JsonSchema)]
 pub struct GateSpec {
     pub context: GateContextSpec,
@@ -834,6 +836,10 @@ pub struct GateContextSpec {
 pub struct FactSpec {
     pub kind: String,
     pub failure_policy: String,
+    /// Kinds of other facts that must be acquired before this one.
+    /// Mirrors `Fact::dependencies()`. Carried in the spec so the gate
+    /// evaluator does not duplicate the dependency graph.
+    pub dependencies: Vec<String>,
 }
 
 /// Serialized filter check.
@@ -1065,6 +1071,7 @@ pub fn build_gate_spec(ctx: GateContext, checks: &[FilterCheck]) -> anyhow::Resu
         .map(|f| FactSpec {
             kind: f.kind().into(),
             failure_policy: f.failure_policy().as_str().into(),
+            dependencies: f.dependencies().iter().map(|d| d.kind().into()).collect(),
         })
         .collect();