feat(engine): GitHub App-backed Copilot engine auth

[jamesadevine]

Summary

Implements first-class GitHub App-backed Copilot engine auth for #1326 — source-clean, with no VSIX and no ADO service connection. Mirrors gh-aw''s create-github-app-token model, adapted to Azure DevOps: the compiler mints an installation token at runtime from a private key stored as an ADO secret.

Front matter

engine:
  id: copilot
  github-app-token:
    app-id: 1234567             # literal App ID or client ID (required)
    owner: octo-org             # installation owner (org or user)
    repositories: [octo-repo]   # optional; scopes the token
    # api-url: https://ghe.example.com/api/v3   # optional (GHES)
    # skip-token-revocation: false              # optional (revoke by default)
    # private-key: MY_SECRET_VAR                # optional override

ado-aw secrets set GITHUB_APP_PRIVATE_KEY "$(cat key.pem)"

Contract (consistent with GITHUB_TOKEN)

The compiler owns variable names; the user owns values:

• private-key defaults to the compiler-owned secret GITHUB_APP_PRIVATE_KEY (set via ado-aw secrets set); optional override names a different secret.
• app-id is a literal (numeric App ID or alphanumeric client ID) — non-secret per-app config, never indirected through a variable.
• The minted token lands in the fixed internal GITHUB_APP_TOKEN.

How it works

• New github-app-token ado-script bundle builds an RS256 JWT (node:crypto, no openssl/npm deps) → resolves the installation for owner → POST /app/installations/{id}/access_tokens (optionally repo-scoped) → masked same-job GITHUB_APP_TOKEN.
• Compiler emits the mint step immediately before the Copilot run in the Agent and Detection jobs and sources GITHUB_TOKEN from $(GITHUB_APP_TOKEN). The token is never provided to SafeOutputs, user steps:, ManualReview, Teardown, or Conclusion.
• A best-effort (always() + continueOnError) revoke step (DELETE /installation/token) runs after the Copilot run unless skip-token-revocation: true.
• Non-secret inputs are single-quoted argv flags (--app-id, --owner, --output-var, --repositories, --api-url) — immune to ADO pipeline-variable shadowing; only the private key rides in masked env.
• Mint/revoke steps run outside the AWF sandbox — no network.allowed entry. Bundle download is driven by a first-class github_app_token_active predicate in the ado-script extension (Agent) and a detection_job_needs_ado_script_bundle predicate in Detection — one download per job by construction.
• github-app-token is Copilot-engine only — a hard compile error on any other engine.id (prevents a silent no-op). A non-blocking compile advisory reminds the author to store the private-key variable as a secret.
• ADO permissions.read/write stay separate; the App token is Copilot-engine-auth only. Copilot capability is granted App/org-side (no permissions: sub-field).

Design notes

• No VSIX: storing the key as an ADO secret variable (same trust model as GITHUB_TOKEN) avoids a Marketplace extension.
• Rebased onto main (includes refactor(compile): centralize ado-script bundle env contract #1315) and conformed to src/compile/ado_bundle.rs: Bundle::GithubAppToken registered with BundleAuth::None (authenticates to GitHub, not ADO REST).

Tests / validation

• Rust: schema parse/validation (literal + client-id app-id, negative-app-id rejection, default vs override private-key, GHES api-url, skip-revocation, non-copilot-engine rejection), step rendering (argv + masked secret + pinned output-var), cross-target integration (standalone/1es/job/stage), bundle-download-by-construction, secrecy advisory.
• ado-script: vitest for the bundle (JWT signing, installation resolution, mint, revoke, argv parsing incl. forward-compat).
• Gate green: cargo build, cargo test, cargo clippy --all-targets --all-features (0 warnings), bash_lint_tests, ado-script typecheck + full vitest suite. Default (no config) output is byte-identical.

Docs

docs/engine.md, docs/ado-script.md, AGENTS.md, and the create/update/debug authoring prompts under prompts/.

Closes #1316

[jamesadevine]

Rebased onto main (now includes #1315) and conformed to the new src/compile/ado_bundle.rs bundle env contract:

• Registered Bundle::GithubAppToken in the enum + ALL + path(), with auth() = BundleAuth::None — the bundle authenticates to the GitHub API (App JWT / installation token), not ADO REST, so it needs no SYSTEM_ACCESSTOKEN and projects no System.*/Build.* mirrors (verified against the churn guard).
• The mint/revoke steps only carry GH_APP_* inputs (App ID, $(...)-sourced private key, owner/repositories, optional api-url) — they satisfy the Agent-job SYSTEM_ACCESSTOKEN trust-boundary guard and the auto-injected-var churn guard.
• Added an inline conformance assertion in the wiring test + a note in docs/ado-script.md''s "Bundle env contract" section.

Validation re-run green after rebase: cargo build, cargo test, cargo clippy --all-targets --all-features (0 warnings), bash_lint_tests, ado-script typecheck + 456 vitest, plus the #1315 ado_bundle contract test (now iterating the new variant) and the churn guard.

[github-actions]

🔍 Rust PR Review

Summary: Looks good with one maintainability concern worth noting. Well-designed feature overall — strong security posture, thorough validation, and solid test coverage across all targets.

---

Findings

⚠️ Suggestions

• agentic_pipeline.rs ~line 918 — Fragile display-name dedup heuristic

  The bundle_already_staged check for the Agent job identifies an existing download by matching step display names:

  matches!(s, Step::Bash(b) if b.display_name.starts_with("Download ado-aw scripts")
      || b.display_name.starts_with("Stage ado-aw scripts"))

  This is a string-literal coupling to internal step names. If either display name is renamed in a future refactor, the dedup silently regresses to redundant downloads (not incorrect behaviour, but wasted time per run). The PR description acknowledges this ("Should rebase onto refactor(compile): centralize ado-script bundle env contract #1315 to route through the new ado_bundle.rs contract + churn guard"), so it's on the radar — just flagging it as the one fragile spot.

  Notably, the Detection job doesn't attempt dedup at all (install_and_download_steps_typed always runs there), so the risk of diverging behaviour is asymmetric across jobs until the refactor(compile): centralize ado-script bundle env contract #1315 follow-up lands.

• src/compile/types.rs — validate() called only at step-gen time, not at deserialization

  GithubAppTokenConfig::validate() is invoked inside github_app_token_step_typed, so it fires during compilation. If someone calls parse_markdown but not compile (e.g., in a future lint-only code path), the structural validation is silently skipped. Not a current bug, but the pattern elsewhere in the codebase for front-matter types is to integrate validation into the compile entry. Worth a follow-up annotation or an explicit early-validate call in FrontMatter's post-deserialize hook if one is ever added.

✅ What Looks Good

• Secret-never-inlined invariant is airtight: private_key is is_valid_env_var_name-validated then always emitted as EnvValue::secret(name) (→ $(NAME) macro). Numeric app_id goes verbatim; variable-form app_id goes as $(NAME). No key material ever touches the generated YAML. ✓

• Same-job variable scoping is correct: setSecretVar emits issecret=true (masked by ADO's scrubber) and is not an output variable, so GITHUB_APP_TOKEN is invisible to other jobs, including SafeOutputs, ManualReview, Teardown, and Conclusion. ✓

• ##vso[ command injection is prevented: setSecretVar reuses escapeProperty/escapeMessage from the existing vso-logger.ts, identical to setVar. The token value is masked before any log line can contain it (mask is set first, then logInfo). ✓

• URL path injection is prevented: owner and each repositories entry go through is_safe_path_segment before compilation, and encodeURIComponent is used in the TypeScript bundle when constructing installation-lookup URLs. ✓

• api_url validation is complete: https:// scheme + non-empty host required; trailing slashes stripped at both Rust (url::Url::parse) and TypeScript (resolveApiUrl) layers. ✓

• Best-effort revoke design is correct: condition: always() + continueOnError: true ensures revocation never fails the build and fires even if the Copilot run errored, matching actions/create-github-app-token's default behaviour. ✓

• detection_provider_env / run_threat_analysis_step correctly receives github_token_source_var: The threat-analysis job's GITHUB_TOKEN is sourced from $(GITHUB_APP_TOKEN) when App auth is configured — not forgotten like it easily could have been in a second-job wiring change. ✓

• Test coverage is comprehensive: JWT signing correctness, org/user installation fallback, repo scoping, literal vs variable app_id, skip-token-revocation, GHES api-url, all four targets (standalone/1es/job/stage), and default-absent baseline are all covered. ✓

Generated by Rust PR Reviewer for #1326 · 75 AIC · ⌖ 6.78 AIC · ⊞ 6.1K · ◷

[jamesadevine]

Addressed the fragile display-name dedup finding.

Why not traverse the graph? The IR dependency graph (ir/graph.rs) doesn''t exist yet at build_agent_job time — it''s a later pass over the fully-assembled pipeline — and even then it models job dependency edges + output references, not a "does a step that stages bundle X exist" index (the download step has no StepId to query). So step inspection could only ever pattern-match display names or script text — both fragile.

Fix: stop inspecting steps entirely. Whether the Agent job stages the bundle is a pure function of front matter, so I made github-app-token a first-class contributor to the ado-script extension''s Agent-job download predicate — exactly like safe_outputs_summary_active (whose consuming step is likewise emitted by build_agent_job, not the extension):

• Added github_app_token_active to AdoScriptExtension, OR''d into the agent-prepare download predicate, set in collect_extensions from engine.github_app_token().is_some().
• Deleted the bundle_already_staged display-name heuristic; build_agent_job now just appends the mint/revoke steps, trusting the extension staged the bundle.
• Detection keeps its self-contained download (no extension-prepare phase); the asymmetry is now intentional and documented. Both jobs emit exactly one download by construction.

Verified a github-app-token-only workflow (no other bundle feature) compiles to exactly 2 downloads (Agent + Detection), 2 mint + 2 revoke. Gate green: cargo build/test/clippy (0 warnings), bash_lint_tests, ado-script vitest.

The second suggestion (validate-at-deserialize) is left as a separate optional follow-up.

[github-actions]

🔍 Rust PR Review

Summary: Implementation is solid with good security controls — one actionable correctness concern to address before merge.

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/github-app-token/index.ts:833-835 — GH_APP_OUTPUT_VAR not pinned by the compiler

  The TypeScript reads GH_APP_OUTPUT_VAR from process.env but the Rust step builder (github_app_token_step_typed) never sets it in the step's env: block. In ADO, all pipeline variables are injected as environment variables. A user with a pipeline variable named GH_APP_OUTPUT_VAR (or an adversary controlling pipeline variables) would silently redirect the masked token to a different variable name. The $(GITHUB_APP_TOKEN) macro in copilot_env would then resolve to empty string, breaking App auth with no error at mint time.

  Two viable fixes:

  1. Pin it in the compiler: add .with_env("GH_APP_OUTPUT_VAR", EnvValue::literal(crate::engine::GITHUB_APP_TOKEN_VAR)) to github_app_token_step_typed.
  2. Remove the env-var override in the TypeScript entirely — the output variable name is an implementation detail not surfaced via front matter.

⚠️ Suggestions

• src/compile/extensions/ado_script.rs — github_app_token_revoke_step_typed has no validate() call

  The mint function (github_app_token_step_typed) guards via cfg.validate()? upfront. The revoke function does not, even though it reads cfg.api_url. Currently safe because both functions are called from the same if let Some(app_token) arms back-to-back. Worth adding cfg.validate()? to the revoke function for symmetry, so neither function silently assumes the other ran first.

• src/compile/extensions/ado_script.rs — revoke script lacks set -eo pipefail

  Mint step: set -eo pipefail\nnode '...'. Revoke step: node '...' revoke\n. The asymmetry is intentional (continueOnError: true, TypeScript revoke() always returns 0), but the bash_lint_tests shellcheck run may surface this. A brief inline comment explaining the omission is intentional would silence any future "why is this missing?" questions.

✅ What Looks Good

• Token masking ordering is correct: setSecretVar is called before any logInfo in main() — the ADO scrubber is ahead of any potential leak.
• Input validation is thorough: is_safe_path_segment on owner/repos, HTTPS+host check on api-url, env-var name validation on private-key and variable-form app-id, with clear error messages.
• Box<EngineOptions>: boxing the new GithubAppTokenConfig field is the right call to avoid a stack-size regression in the EngineConfig enum.
• Detection-job bundle dedup: the design ("by construction, not by inspecting emitted steps") is clean, and the test_github_app_token_reuses_staged_bundle_in_agent test pin exactly the right invariant.
• encodeURIComponent(owner) in the TS URL construction and JSON.stringify(body) for the repository-scoped token request are both correct.
• github_token_source_var is plumbed through run_threat_analysis_step: the detection Copilot run and the agent Copilot run now use the same source-of-truth for GITHUB_TOKEN, which prevents a silent no-op detection run when App auth is configured.

Generated by Rust PR Reviewer for #1326 · 149.2 AIC · ⌖ 6.42 AIC · ⊞ 6.1K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation — the security design is well-reasoned and thoroughly tested. One forward-compatibility trap in the TypeScript arg parser worth fixing, and a few minor observations.

---

Findings

⚠️ Suggestions

scripts/ado-script/src/github-app-token/index.ts — parseArgs forward-compatibility trap

The parser assumes every flag is a --flag value pair and unconditionally advances by 2:

for (let i = 0; i < argv.length; i += 2) {
    const flag = argv[i];
    const value = argv[i + 1];
    if (value === undefined) break;
    // default: skip (but also skips the NEXT flag as the "value")
}

The comment says "Unknown flags are ignored so the compiler can add new ones without breaking an older bundle." However, if a future compiler adds a boolean flag with no value (e.g. --debug), an older bundle running that argv would:

1. Reach --debug — unknown, i += 2
2. Skip --debug and the following --app-id (consumed as its "value")
3. Remaining flag-value pairs shift out of alignment — appId, owner, etc. all silently undefined

Since requireArg throws on undefined args, this would fail loudly rather than silently — but the error message ("required argument --app-id is missing") would be confusing. Consider checking if the next token looks like a flag before consuming it as a value:

const value = argv[i + 1];
if (value === undefined || value.startsWith("--")) {
    i += 1; // boolean flag — skip only the flag itself
    continue;
}
i += 2;

src/compile/agentic_pipeline.rs (line ~1142) — Detection job's inline ado-script download

The Detection job calls install_and_download_steps_typed only when github_app_token is configured. If a future Detection-job feature also needs the ado-script bundle, there will be two install_and_download_steps_typed calls. The unzip -o in the download script handles idempotency (overwrite), so correctness is not at risk — but the double-download is wasteful. A brief comment noting this is currently the only ado-script download path in Detection would help future contributors avoid inadvertently adding a second one.

src/compile/types.rs — private_key validation only checks variable name, not secrecy

validate() correctly ensures private_key matches [A-Za-z_][A-Za-z0-9_]*. It can't enforce that the ADO variable is actually marked secret (there's no compile-time introspection into ADO variable metadata). The docstring says "ADO secret variable name" and the field description recommends ado-aw secrets set, which is the right place for this guidance. Consider surfacing a lint warning (not a compile error) if private_key looks like a plain pipeline variable name that may not be secret — similar to how other fields emit WarningKind diagnostics. Even a non-blocking "ensure this variable is marked secret" advisory would help.

---

✅ What Looks Good

• Security design: argv for non-secret inputs (immune to pipeline-variable shadowing), env for secret material, --output-var pinned as a compiler constant (GITHUB_APP_TOKEN_VAR) so no ADO variable can redirect the minted token. Thoroughly reasoned and documented.

• Token masking order: setSecretVar(outputVar, token) is emitted before the logInfo(...) call — ADO's scrubber is registered before any subsequent log line that could echo the token. Correct.

• Revoke step design: set -eo pipefail is intentionally absent from the revoke script (best-effort cleanup), continueOnError: true, Condition::Always — all three are correct and the comments explain the reasoning. revoke() in TS gracefully handles an empty/missing GH_APP_TOKEN (e.g. when the mint step failed).

• resolveInstallationId tries the org endpoint first then user — matches GitHub App installation semantics.

• Box<EngineOptions>: correctly reduces the EngineConfig enum variant size; the test constructors are updated throughout common.rs.

• Detection job github_token_source_var: cleanly threads the token source through run_threat_analysis_step — no more hardcoded "GITHUB_TOKEN" in the detection path.

• build_detection_job is self-contained: the comment explaining why Detection downloads the bundle inline (no extension-prepare phase to piggyback) is clear and the "by construction, not by inspecting emitted steps" invariant is preserved.

• Test coverage for the Rust side is comprehensive: literal vs variable-name app-id, GHES api-url, skip-revocation, env-var validation rejection, revoke-step continueOnError/Condition::Always assertions, path constant consistency test.

Generated by Rust PR Reviewer for #1326 · 115.8 AIC · ⌖ 6.92 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with one logic gap worth fixing and one minor error message inaccuracy.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs:GithubAppTokenConfig::validate() — error message omits _ from allowed set
  The two error messages for owner and repositories describe the allowed character set as [A-Za-z0-9._-], but is_safe_path_segment (line 42 of validate.rs) also permits _. GitHub org and repo names regularly contain underscores (octo_org, my_repo). Functionally these names are accepted (validation passes), but the error message is misleading — anyone reading the rejection text to understand the constraint will conclude underscores are forbidden.

• src/compile/agentic_pipeline.rs / src/compile/mod.rs — github-app-token is silently a no-op for non-Copilot engines
  There is no cross-field validation rejecting (or even warning) when engine.github-app-token is set alongside a non-Copilot engine ID. The mint and revoke steps are unconditionally emitted whenever front_matter.engine.github_app_token().is_some(), regardless of engine ID. However, copilot_env() — the only place GITHUB_TOKEN is sourced from $(GITHUB_APP_TOKEN) — is only called on the Copilot code path. The result: a non-Copilot agent with github-app-token configured will mint (and revoke) an installation token that is never wired into the engine's env, while advertising "Mint GitHub App token (Copilot engine auth)" steps in the pipeline. A compile-time error like:

  engine.github-app-token is only supported for engine.id = copilot
  

  would prevent this confusion. No test covers the non-Copilot case, so the silent behaviour is undetected.

⚠️ Suggestions

• src/compile/mod.rs — GithubAppTokenConfig::validate() fires late
  The config's structural validation (HTTPS url, env var name format, safe path segments) isn't called until deep inside github_app_token_step_typed, which is invoked from build_agent_job. The secrecy advisory fires early in compile_pipeline_inner, but field-level validation errors only surface after the compilation pipeline has progressed substantially. A small guard near the advisory:

  if let Some(cfg) = front_matter.engine.github_app_token() {
      cfg.validate().context("engine.github-app-token config is invalid")?;
  }

  would give a cleaner error path and make it trivial to add the Copilot-only guard above in the same place.

• src/compile/agentic_pipeline.rs:build_detection_job — divergent bundle-staging pattern
  The Agent job's bundle download is driven by the github_app_token_active flag on AdoScriptExtension (the "flag owns the download" pattern). The Detection job inlines the download inside the if let Some(app_token) block because Detection has no extension-prepare phase. The comment accurately warns about the future double-download hazard. If the Detection job ever needs the bundle for a second feature, the inline-conditional pattern would silently emit two downloads. A boolean github_app_token_active field on a DetectionContext struct (even a trivial one) would mirror the Agent pattern and prevent accidental duplication — but this is low-priority given the comment is clear.

✅ What Looks Good

• Security design is excellent. Secrets-in-masked-env / non-secrets-as-argv is the right call. sh_single_quote correctly implements the '\'' escape. Compiler-pinned --output-var 'GITHUB_APP_TOKEN' prevents pipeline-variable shadowing of the token destination. EnvValue::secret in the revoke step ensures the minted token is never printed.
• cfg.validate() is called in both step builders as defense-in-depth — a future caller that skips the compile-time validation still gets an error before an invalid step is emitted.
• JWT timing (60 s clock backdate + 9 min window) correctly targets GitHub's 10-minute cap while tolerating agent drift.
• encodeURIComponent(owner) in TypeScript adds defense-in-depth even though the Rust side has already validated the name.
• setSecretVar uses the existing escapeProperty/escapeMessage infrastructure — no raw ##vso[ embedding.
• Revoke step is correctly best-effort: Condition::Always + continueOnError: true + TypeScript warns-not-throws on failure.
• Integration tests are comprehensive: all four targets, skip-revocation, literal numeric App ID, GHES api-url, bundle-dedup delta test (count_downloads(&with) == count_downloads(&without) + 1).

Generated by Rust PR Reviewer for #1326 · 102.7 AIC · ⌖ 7.09 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid security design and thorough test coverage. One documentation bug in types.rs that could silently misconfigure app-id for users following the struct-level example.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs ~line 360 — GithubAppTokenConfig struct doc example contradicts the field spec

  The struct-level Rust doc comment shows:

  github-app-token:
    app-id: GH_APP_ID          # ADO variable name holding the App ID
    private-key: GH_APP_KEY    # ADO *secret* variable name holding the PEM

  but app-id is documented at the field level as a literal value (e.g. 1234567 or Iv23liABC...), not an ADO variable name. This directly contradicts docs/engine.md and the PR description. A user reading the struct doc first would write app-id: GH_APP_ID (a literal string), the JWT iss claim would be "GH_APP_ID", and GitHub API auth would fail with a cryptic error — and validation would not catch it (the string GH_APP_ID satisfies [A-Za-z0-9._-]+). The example should be:

  github-app-token:
    app-id: 1234567            # literal App ID (numeric) — NOT a variable name
    private-key: GH_APP_KEY    # ADO *secret* variable name holding the PEM (optional override)

⚠️ Suggestions

• scripts/ado-script/src/github-app-token/index.ts — unhandled rejection on entry guard

  run.then((rc) => process.exit(rc));

  Both main and revoke wrap their bodies in try/catch and return number, so this won't reject in practice. Adding .catch((e) => { console.error(e); process.exit(1); }) would make the defensive intent explicit and protect against future code additions that bypass the try/catch.

• index.ts:resolveInstallationId — GitHub API error body in thrown message

  lastBody (the raw response body from the org and user endpoints) is included verbatim in the thrown Error message, which is then logged via logError. escapeMessage properly handles newline injection, but if a GitHub API response body ever included partial token material (unusual but theoretically possible on some GHES configurations), it would appear in the build log. Truncating lastBody to ~200 chars would be a small improvement.

✅ What Looks Good

• argv-vs-env split for non-secret/secret inputs is the right call. ADO injects pipeline variables into every step's env, so routing non-secret, compiler-owned inputs through single-quoted argv flags (not env vars) prevents variable shadowing. sh_single_quote correctly handles embedded quotes.
• setSecretVar in vso-logger.ts properly uses escapeProperty + escapeMessage, so the token can't break out of the ##vso[task.setvariable ...] command even for unusual token shapes.
• validate() called symmetrically in both github_app_token_step_typed and github_app_token_revoke_step_typed — neither silently assumes the other ran first.
• Revoke step design (Condition::Always + continueOnError: true, no set -eo pipefail, GH_APP_TOKEN gracefully tolerates empty) handles the "mint failed → revoke runs anyway" edge case correctly.
• Detection job self-contained download with a clear comment warning future maintainers to consolidate if a second bundle is ever needed — this is the right tradeoff given current scope.
• Test coverage is comprehensive: schema round-trips, rejection cases, step rendering assertions (argv vs env, pinned --output-var, revoke shape), and cross-target integration.

Generated by Rust PR Reviewer for #1326 · 118.5 AIC · ⌖ 6.88 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the security design is solid. One minor validation gap worth fixing, plus a structural observation.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs — de_string_or_number accepts negative integers

  The visit_i64 handler converts v to v.to_string(), producing strings like "-1234567". The subsequent validate() charset check ([A-Za-z0-9._-]) passes because - is in the allowed set. A YAML input app-id: -7654321 silently survives validation and gets emitted as --app-id '-7654321' — producing a JWT with a nonsensical iss claim that the GitHub API rejects at runtime. Simple fix: add if v == 0 || v.to_string().starts_with('-') { return Err(...) } in visit_i64, or restrict the charset to disallow a leading - (e.g., require the first character to be alphanumeric).

  This is low severity (no security impact, just a confusing runtime failure), but the fix is trivial.

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — Detection-job bundle download diverges structurally from Agent-job download

  The Agent job gets its bundle staged by AdoScriptExtension (driven by github_app_token_active). The Detection job calls install_and_download_steps_typed inline, self-contained. The NOTE comment is good documentation, but this creates two separate code paths that must stay in sync. If a future detection-only feature also needs the bundle, it's not obvious that it should be gated together with the GitHub App token predicate. Consider extracting a small detection_job_needs_ado_script_bundle() predicate (currently equivalent to github_app_token().is_some()) that can be ||'d as new detection features arrive, mirroring the pattern in AdoScriptExtension::declarations_agent_prepare_steps.

• src/compile/extensions/ado_script.rs — continue_on_error field mutation breaks the builder chain

  let mut step = BashStep::new(...).with_condition(Condition::Always);
  step.continue_on_error = true;
  step = step.with_env(...);

  Minor: if BashStep lacks a with_continue_on_error(bool) builder method, this is the right workaround, but the inconsistency (builder chain interrupted by direct mutation, then resumed) is slightly surprising. If adding a builder method is easy, it would clean this up.

✅ What Looks Good

• Argv-flag design (--app-id, --owner, --output-var, --repositories, --api-url) is the right call — ADO injects pipeline variables into env, so pinning non-secret inputs as argv prevents shadowing attacks. The --output-var being compiler-pinned rather than user-configurable in practice is especially important.
• sh_single_quote correctly handles shell metacharacters, and '\'' escaping is the textbook-correct approach.
• setSecretVar called before logInfo — the masking command precedes any log line referencing the installation, which is the correct defensive ordering.
• Revocation as Condition::Always + continueOnError — mirrors actions/create-github-app-token's behavior. The TS-side graceful no-op when GH_APP_TOKEN is empty (mint was skipped) is the right defense-in-depth.
• validate_engine_feature_support runs before get_engine — gives the author a precise error message instead of a generic "unsupported engine" fallback.
• Box<EngineOptions> reduces stack pressure for the common no-full-config path; all pattern match sites are correctly updated.
• Test coverage is comprehensive: unit-level JWT signing/parse, resolver fallback, mint/revoke rendering, secrecy advisory, cross-target integration, bundle-deduplication delta, and skip-token-revocation.

Generated by Rust PR Reviewer for #1326 · 93.9 AIC · ⌖ 8.74 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-designed and securely implemented. One documentation bug that will mislead users at authoring time.

Findings

🐛 Bugs / Logic Issues

• src/compile/types.rs:368-380 — Struct-level doc contradicts field-level doc and PR description for app-id

  The opening paragraph says "the App ID and private key are supplied as ADO pipeline variable names" and the code example shows app-id: GH_APP_ID # ADO variable name holding the App ID. This directly contradicts the app_id field-level doc ("a literal value, either a numeric App ID (e.g. 1234567) or an alphanumeric client ID") and the PR description ("app-id: 1234567 # literal App ID or client ID (required)").

  The impact is not theoretical: GH_APP_ID passes validate() (matches [A-Za-z0-9._-]+ and starts with alphanumeric) so compilation succeeds silently, but the runtime call to GitHub fails because GH_APP_ID is not a valid JWT iss. Authors following the struct docs — or using IDE hover docs — would be misled.

  Fix: change the struct-level example to app-id: 1234567 (matching the PR description) and update the opening sentence to clarify that only private-key is a variable name while app-id is a literal value.

✅ What Looks Good

• sh_single_quote + argv flags for non-secret inputs — Passing --app-id, --owner, --repositories, --api-url, and --output-var as single-quoted argv (immune to pipeline-variable shadowing) rather than env vars is the right call and is well-documented in both the Rust and TS sources.

• api_url validation — Using url::Url::parse + scheme=https + host presence check (types.rs:517-531) is solid; the test covering `(redacted) rejection is a good regression guard.

• Token masked before first log line — setSecretVar(outputVar, token) in main() emits the ##vso[task.setvariable ...;issecret=true] command before any logInfo call that could contain the token, keeping ADO's log scrubber ahead of potential leaks.

• continueOnError + Condition::Always on revoke — The revoke step correctly cannot fail the build, and the comment explaining why set -eo pipefail is intentionally absent from the revoke script is exactly the kind of contextual note that prevents accidental future "cleanup".

• encodeURIComponent(owner) in the TypeScript bundle — Correctly prevents URL injection into the installation lookup endpoint.

• Box(EngineOptions) — Good fix; avoids large-enum-variant stack growth as EngineOptions accumulates fields.

• Test depth — Rust unit tests (step rendering, argv pinning, default vs. override private-key var, GHES api-url, rejects HTTP, rejects non-Copilot engine) + TS vitest (JWT sign/verify, parseArgs forward-compat, mint + revoke flows) + cross-target integration tests + skip-token-revocation path. Coverage is thorough.

Generated by Rust PR Reviewer for #1326 · 82.9 AIC · ⌖ 9.06 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-designed feature with sound security architecture — the argv/env split for secret isolation is particularly solid. Two findings worth addressing.

---

Findings

🐛 Bugs / Logic Issues

• scripts/ado-script/src/github-app-token/index.ts — revoke bundle: unset variable resolves to its literal in ADO, bypassing the "no token" guard

  In ADO, a $(GITHUB_APP_TOKEN) reference in a step's env: block resolves to the literal string "$(GITHUB_APP_TOKEN)" when that variable was never set (e.g. because the mint step was skipped after a prior failure, or because the mint failed before setSecretVar was called). The revoke bundle's guard:

  if (!token || token.length === 0) {
      logWarning("[github-app-token] no token to revoke (GH_APP_TOKEN empty)");
      return 0;
  }

  ...misses this case because "$(GITHUB_APP_TOKEN)".length > 0. The bundle then sends DELETE /installation/token with Authorization: Bearer $(GITHUB_APP_TOKEN), receives a 401, and logs revoke returned HTTP 401 (ignored). continueOnError: true absorbs the failure, so this is not a build-breaking bug, but the 401 noise can confuse authors diagnosing unrelated failures.

  Simple fix — extend the guard:

  if (!token || token.length === 0 || token.startsWith('$(')) {
      logWarning("[github-app-token] no token to revoke (GH_APP_TOKEN not set)");
      return 0;
  }

  This matches the pattern already used for detecting un-expanded ADO macros elsewhere in similar bundles.

⚠️ Suggestions

• tests/compiler_tests.rs — compile_inline_agent temp dir is process-unique, not test-unique

  let temp_dir = std::env::temp_dir().join(format!(
      "agentic-pipeline-{tag}-{}",
      std::process::id()
  ));

  Two different tests with the same tag string running in parallel within the same test process would silently stomp each other's input/output files. All current callers use distinct tags, so this is not a current bug, but adding a test in future that reuses a tag (or copying this pattern) would produce a hard-to-diagnose race. Using std::time::Instant nanos or a per-test UUID (or just thread::current().id()) would make it robust.

✅ What Looks Good

• argv/env security split is correct: non-secret inputs (--app-id, --owner, --output-var, --repositories, --api-url) as single-quoted argv flags immune to pipeline-variable shadowing; only the private key rides in masked env. The sh_single_quote implementation and the escape-single-quote-as-'\'' pattern are both correct.
• setSecretVar injection safety: escapeMessage correctly encodes \r/\n → %0D/%0A, preventing a crafted token value from embedding a second ##vso[...] command on a new line. Layered with issecret=true scrubbing.
• compile_inline_agent assertions are comprehensive: the assert_github_app_token_wiring helper verifies mint/revoke counts, GITHUB_TOKEN source, argv flag placement, env-only private key, and single-quoted output-var pin. The deduplication test (bundle staged once in Agent, once in Detection) is a particularly good regression guard.
• validate_engine_feature_support hard-error pattern: rejecting github-app-token on non-Copilot engines before compilation starts (not as a silent no-op) matches the gh-aw convention and the error message is actionable.
• Box<EngineOptions>: correct size reduction for a variant that would otherwise inflate every EngineConfig enum value on the stack.

Generated by Rust PR Reviewer for #1326 · 101.4 AIC · ⌖ 9.15 AIC · ⊞ 6.2K · ◷