feat(safeoutputs): add GitHub issue outputs

[jamesadevine]

Summary

• promote GitHub create-issue from ado-aw-debug to a configured-only public safe output
• add gh-aw-aligned set-issue-type with same-run temporary issue IDs
• isolate PAT and permission-scoped GitHub App credentials to Stage 3, including GHES API routing
• migrate legacy front matter with a codemod and update documentation and dogfood workflows

Closes #1621

Test plan

• cargo test (2,600 tests)
• cargo clippy --all-targets
• cargo test --test compiler_tests
• cargo test --test codemod_tests
• npm test -- --maxWorkers=1 (758 tests)
• npm run typecheck
• npm run build:github-app-token

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

🔍 Rust PR Review

Summary: looks good — solid promotion from debug-only to public safe output, well-structured; one correctness concern and two suggestions worth addressing.

Findings

🐛 Bugs / Logic Issues

• src/safe_outputs/set_issue_type.rs:20-23 — #[serde(untagged)] on GithubIssueNumber may mis-parse
  GithubIssueNumber is #[serde(untagged)], so serde tries variants in declaration order. Number(u64) is listed first, and Temporary(GithubTemporaryId) second. A JSON string like "#aw_bug1" won't parse as u64, so it falls through correctly — but an integer-valued string like "42" will also fail u64 and fall through to GithubTemporaryId, which will then reject it with a confusing error about the #aw_ prefix rather than "expected a positive integer". More concretely: if the MCP layer ever serialises the number as a quoted string (some clients do), the error message will mislead the agent. Consider tagging the enum (e.g. #[serde(rename_all = "lowercase")] + an outer-object {type, value} shape) or explicitly testing that "42" (string) produces a clear error. This is a latent UX bug, not a security issue.

• src/safe_outputs/create_issue.rs — duplicate-temporary_id check runs before resolve_target_repo
  The has_resolved_github_issue guard fires before the target-repo is resolved. If resolve_target_repo would fail (e.g. non-GitHub pipeline with no configured target-repo), a second call with the same temporary_id returns "already used" rather than the config error. Not a security issue, but can confuse diagnostics — the guard and the repo resolution could swap order.

⚠️ Suggestions

• src/compile/common.rs — github_safe_outputs_auth() called twice at compile time
  validate_github_issue_outputs_config (called in build_pipeline_context) ends with let _ = front_matter.github_safe_outputs_auth()?; to surface any auth-config errors early, and then build_safeoutputs_job calls it again. The duplication is harmless but it means the validation logic and the usage logic have to stay in sync. Extracting the auth result into BuiltPipelineContext (like executor_ado_env was) would make the data flow explicit and prevent future drift.

• src/safe_outputs/set_issue_type.rs:369 — response.text() on non-success path uses .unwrap_or_else

  .unwrap_or_else(|_| "<unable to read response body>".to_string())

  This is fine as-is and consistent with create_issue.rs, just flagging it was reviewed and is intentional.

✅ What Looks Good

• Security isolation is well-preserved: ADO_AW_GITHUB_TOKEN / ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN are injected into Stage 3 only (via generate_executor_ado_env) and the github_token field never flows into Agent or Detection env. The old debug_enabled_tools NDJSON gate has been cleanly replaced by the standard tool_configs.contains_key pattern already used by all other tools.
• GithubTemporaryId validated newtype follows project conventions from src/secure.rs exactly — parse-don't-validate, consistent #aw_<id> normalisation, and the canonical() helper avoids double-# bugs.
• Codemod is robust: the errors_without_mutation_when_new_key_already_exists test verifies the snapshot equality on failure, ensuring atomicity is maintained. The github-token bridging of $(ADO_AW_DEBUG_GITHUB_TOKEN) is a good compatibility shim.
• require-approval parity validation in validate_github_issue_outputs_config correctly enforces that create-issue and set-issue-type must share the same job so temporary-ID resolution works; the error message clearly explains why.
• engine.github-app-token write-permission guard (permissions.{name} is write → bail!) prevents the Agent token (which is read-only by design) from inadvertently being reused as a write credential for Stage 3, which would break the security model.

Generated by Rust PR Reviewer for #1670 · 60.2 AIC · ⌖ 5.9 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid promotion of GitHub issue outputs from debug-only to production; a few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safe_outputs/create_issue.rs:311-318 — TOCTOU on temporary ID duplicate check
  The pre-flight check calls has_resolved_github_issue (releases the lock), makes the HTTP call, then calls register_resolved_github_issue (re-acquires the lock). Two concurrent safe-output executors for the same run could both pass the pre-flight check and both file a GitHub issue. register_resolved_github_issue does have a second check inside the lock and bail!s, but that propagates as an anyhow::Error, not an ExecutionResult::failure — so it surfaces as a hard error rather than a graceful operator-facing failure message. Consider converting the inner bail! path to return ExecutionResult::failure at the call site in execute_impl, or document that the duplicate-registration case is intentionally a hard error.

• src/safe_outputs/set_issue_type.rs — empty issue_type sends {"type": ""} to GitHub
  The GitHub Issues REST API for clearing an issue type expects "type": null, not "type": "". Sending an empty string is likely to return a 422 Unprocessable Entity rather than clearing the type. The dry-run path and docstring both describe this as a "clear" operation, so the intent is correct — the payload should use serde_json::Value::Null when resolved_type is empty.

• src/compile/common.rs:1501 — ADO_AW_GITHUB_API_URL written unquoted into generated YAML
  generate_executor_ado_env emits ADO_AW_GITHUB_API_URL: {value} as a bare literal. The URL is validated at compile time via url::Url::parse (https-only), but a URL with a #fragment component — which url::Url permits — would produce malformed YAML. The parse_safe_outputs_github_api_url helper returns parsed.to_string() which preserves fragments. Reject or strip fragment components in that function, or quote the emitted value.

⚠️ Suggestions

• src/safe_outputs/set_issue_type.rs — no allowlist enforcement when allowed is empty
  When the operator omits safe-outputs.set-issue-type.allowed, any agent-supplied issue_type passes through verbatim to GitHub (after reject_pipeline_injection). The analogous create-issue.allowed-labels works the same way, but issue types have no extra structural validation. Consider documenting this clearly in docs/safe-outputs.md so operators know an empty allowed list is fully permissive, not deny-all.

• src/safe_outputs/result.rs:get_tool_config — silent require-approval removal
  The new code strips require-approval from the config value before deserialisation to avoid deny_unknown_fields errors. This is correct, but a brief comment explaining the rationale would help future readers avoid confusion if a config struct ever legitimately needs this field.

✅ What Looks Good

• Credential isolation is well-designed: ADO_AW_GITHUB_TOKEN is strictly Stage 3-only and never injected into Agent or Detection jobs. The compile-time wiring in agentic_pipeline.rs enforces this boundary cleanly.
• GithubTemporaryId as a validated newtype (parse-don't-validate) is exactly the right pattern — format errors fail at deserialisation rather than runtime.
• parsePermissions in TypeScript has solid prototype-pollution guards (__proto__, prototype, constructor blocklist), and the Rust mirror in GithubAppTokenConfig::validate_for is consistent.
• Codemod 0006 is idempotent, preserves the legacy token variable as a migration aid, handles all edge cases (existing auth, conflicting keys) with clear error messages, and is well tested.
• Error body sanitization via neutralize_pipeline_commands on GitHub API error responses is a good defence-in-depth addition.

Generated by Rust PR Reviewer for #1670 · 105.7 AIC · ⌖ 5.83 AIC · ⊞ 6.2K · ◷