diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000..72d3c64 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,12 @@ +# Code owners for security-sensitive surfaces. +# +# Live-test fixtures + the CI workflow that drives them deal with +# secrets that, if leaked, would let an attacker post comments, +# cancel PRs, or mutate fixture state in our public test orgs. +# Require explicit review on those paths. + +/.github/workflows/live-tests.yml @goldenwitch +/test-env/ @goldenwitch +/crates/devdev-test-env/ @goldenwitch +/scripts/validate.ps1 @goldenwitch +/claims.toml @goldenwitch diff --git a/.github/workflows/live-tests.yml b/.github/workflows/live-tests.yml index 7fb1dda..d278885 100644 --- a/.github/workflows/live-tests.yml +++ b/.github/workflows/live-tests.yml @@ -143,11 +143,15 @@ jobs: cargo run -p devdev-test-env --quiet -- --skip-ado print-env >> "$GITHUB_ENV" - name: seed gh CLI auth (consumer token) + # The `live_credential_chain::gh_cli_provider_yields_token` + # test exercises `gh auth token` and needs an authenticated + # gh CLI. We use a non-`GH_TOKEN` env var name because + # `gh auth login --with-token` refuses to run while + # `GH_TOKEN` is set in its environment. env: - GH_TOKEN: ${{ steps.gh_consumer.outputs.token }} + DEVDEV_LIVE_GH_LOGIN_TOKEN: ${{ steps.gh_consumer.outputs.token }} run: | - # Ensure the token never echoes (set -x off; redirect via stdin only). - printf '%s' "$GH_TOKEN" | gh auth login --with-token + printf '%s' "$DEVDEV_LIVE_GH_LOGIN_TOKEN" | gh auth login --with-token gh auth status - name: cargo test (live, --ignored) @@ -161,11 +165,18 @@ jobs: DEVDEV_LIVE_WRITE: ${{ inputs.run_writes && '1' || '' }} run: | cargo test --workspace --locked --tests \ - -- --ignored --skip live_workspace_cwd + -- --ignored \ + --skip live_workspace_cwd \ + --skip cargo_builds_hello_world_inside_mount cleanup: - needs: live-tests - if: always() + # Depend on `provision` (not just `live-tests`) so we can gate on + # whether the manifest-lock artifact actually exists. If `gate` + # skips the workflow or `provision` fails before upload, there's + # nothing to reset and downloading a missing artifact would turn a + # clean skip/failure into a confusing secondary cleanup failure. + needs: [provision, live-tests] + if: ${{ always() && needs.provision.result == 'success' }} runs-on: ubuntu-latest timeout-minutes: 10 environment: live-tests-admin diff --git a/Cargo.lock b/Cargo.lock index 1e27947..ed2db5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -393,6 +393,7 @@ dependencies = [ "devdev-workspace", "serde", "serde_json", + "serial_test", "tempfile", "thiserror 2.0.18", "tokio", @@ -427,6 +428,7 @@ name = "devdev-integrations" version = "0.1.0" dependencies = [ "async-trait", + "base64", "reqwest", "serde", "serde_json", @@ -462,6 +464,23 @@ dependencies = [ "tracing", ] +[[package]] +name = "devdev-test-env" +version = "0.1.0" +dependencies = [ + "anyhow", + "base64", + "clap", + "reqwest", + "serde", + "serde_json", + "tempfile", + "thiserror 2.0.18", + "tokio", + "tracing", + "tracing-subscriber", +] + [[package]] name = "devdev-tui" version = "0.1.0" @@ -712,8 +731,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ff2abc00be7fca6ebc474524697ae276ad847ad0a6b3faa4bcb027e9a4614ad0" dependencies = [ "cfg-if", + "js-sys", "libc", "wasi", + "wasm-bindgen", ] [[package]] @@ -723,9 +744,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd" dependencies = [ "cfg-if", + "js-sys", "libc", "r-efi 5.3.0", "wasip2", + "wasm-bindgen", ] [[package]] @@ -861,6 +884,7 @@ dependencies = [ "tokio", "tokio-rustls", "tower-service", + "webpki-roots", ] [[package]] @@ -1140,6 +1164,12 @@ version = "0.4.29" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897" +[[package]] +name = "lru-slab" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" + [[package]] name = "matchers" version = "0.2.0" @@ -1389,6 +1419,22 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "pr-reviewer" +version = "0.1.0" +dependencies = [ + "anyhow", + "async-trait", + "clap", + "devdev-acp", + "devdev-cli", + "devdev-daemon", + "devdev-integrations", + "tokio", + "tracing", + "tracing-subscriber", +] + [[package]] name = "predicates" version = "3.1.4" @@ -1435,6 +1481,61 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "quinn" +version = "0.11.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9e20a958963c291dc322d98411f541009df2ced7b5a4f2bd52337638cfccf20" +dependencies = [ + "bytes", + "cfg_aliases 0.2.1", + "pin-project-lite", + "quinn-proto", + "quinn-udp", + "rustc-hash", + "rustls", + "socket2", + "thiserror 2.0.18", + "tokio", + "tracing", + "web-time", +] + +[[package]] +name = "quinn-proto" +version = "0.11.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "434b42fec591c96ef50e21e886936e66d3cc3f737104fdb9b737c40ffb94c098" +dependencies = [ + "bytes", + "getrandom 0.3.4", + "lru-slab", + "rand", + "ring", + "rustc-hash", + "rustls", + "rustls-pki-types", + "slab", + "thiserror 2.0.18", + "tinyvec", + "tracing", + "web-time", +] + +[[package]] +name = "quinn-udp" +version = "0.5.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "addec6a0dcad8a8d96a771f815f0eaf55f9d1805756410b39f5fa81332574cbd" +dependencies = [ + "cfg_aliases 0.2.1", + "libc", + "once_cell", + "socket2", + "tracing", + "windows-sys 0.52.0", +] + [[package]] name = "quote" version = "1.0.45" @@ -1555,6 +1656,8 @@ dependencies = [ "native-tls", "percent-encoding", "pin-project-lite", + "quinn", + "rustls", "rustls-pki-types", "serde", "serde_json", @@ -1562,6 +1665,7 @@ dependencies = [ "sync_wrapper", "tokio", "tokio-native-tls", + "tokio-rustls", "tower", "tower-http", "tower-service", @@ -1569,6 +1673,7 @@ dependencies = [ "wasm-bindgen", "wasm-bindgen-futures", "web-sys", + "webpki-roots", ] [[package]] @@ -1629,6 +1734,12 @@ dependencies = [ "syn", ] +[[package]] +name = "rustc-hash" +version = "2.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94300abf3f1ae2e2b8ffb7b58043de3d399c73fa6f4b73826402a5c457614dbe" + [[package]] name = "rustix" version = "1.1.4" @@ -1649,6 +1760,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69f9466fb2c14ea04357e91413efb882e2a6d4a406e625449bc0a5d360d53a21" dependencies = [ "once_cell", + "ring", "rustls-pki-types", "rustls-webpki", "subtle", @@ -1661,6 +1773,7 @@ version = "1.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "be040f8b0a225e40375822a563fa9524378b9d63112f53e19ffff34df5d33fdd" dependencies = [ + "web-time", "zeroize", ] @@ -1687,6 +1800,15 @@ version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f" +[[package]] +name = "scc" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46e6f046b7fef48e2660c57ed794263155d713de679057f2d0c169bfc6e756cc" +dependencies = [ + "sdd", +] + [[package]] name = "schannel" version = "0.1.29" @@ -1728,6 +1850,12 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" +[[package]] +name = "sdd" +version = "3.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" + [[package]] name = "security-framework" version = "3.7.0" @@ -1845,6 +1973,32 @@ dependencies = [ "winapi", ] +[[package]] +name = "serial_test" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "911bd979bf1070a3f3aa7b691a3b3e9968f339ceeec89e08c280a8a22207a32f" +dependencies = [ + "futures-executor", + "futures-util", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0a7d91949b85b0d2fb687445e448b40d322b6b3e4af6b44a29b21d9a5f33e6d9" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -2069,6 +2223,21 @@ dependencies = [ "zerovec", ] +[[package]] +name = "tinyvec" +version = "1.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e61e67053d25a4e82c844e8424039d9745781b3fc4f32b8d55ed50f5f667ef3" +dependencies = [ + "tinyvec_macros", +] + +[[package]] +name = "tinyvec_macros" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" + [[package]] name = "tokio" version = "1.52.0" @@ -2474,6 +2643,25 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "web-time" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a6580f308b1fad9207618087a65c04e7a10bc77e02c8e84e9b00dd4b12fa0bb" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + +[[package]] +name = "webpki-roots" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52f5ee44c96cf55f1b349600768e3ece3a8f26010c05265ab73f945bb1a2eb9d" +dependencies = [ + "rustls-pki-types", +] + [[package]] name = "winapi" version = "0.3.9" diff --git a/Cargo.toml b/Cargo.toml index a39a3a3..e7b502f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,9 +6,11 @@ members = [ "crates/devdev-integrations", "crates/devdev-daemon", "crates/devdev-tasks", + "crates/devdev-test-env", "crates/devdev-tui", "crates/devdev-scenarios", "crates/devdev-workspace", + "samples/pr-reviewer", ] [workspace.package] @@ -44,6 +46,9 @@ clap = { version = "4", features = ["derive"] } # HTTP reqwest = { version = "0.12", features = ["json"] } +# Base64 (used by ADO Basic-auth header construction) +base64 = "0.22" + # MCP (Model Context Protocol) — used by devdev-daemon to expose DevDev # state to Copilot agents via Streamable HTTP. PoC at target/tmp/poc-mcp-rs/. rmcp = { version = "0.9", features = [ @@ -65,6 +70,7 @@ portable-pty = "0.9" # Testing tempfile = "3" +serial_test = "3" # --------------------------------------------------------------------------- # Release packaging (cargo-dist). Linux + Windows x86_64 only; no macOS diff --git a/README.md b/README.md index a518a5b..bca0cb0 100644 --- a/README.md +++ b/README.md @@ -126,6 +126,16 @@ installed to run the mount-heavy tests (`cargo test -- --ignored`). Live Copilot CLI tests are also `#[ignore]` and require a logged-in Copilot CLI. +Live multi-host integration tests (against real github.com and +Azure DevOps fixtures) are gated behind the +[`live-tests` workflow](.github/workflows/live-tests.yml). The +fixture environment is fully reproducible from +[`test-env/manifest.json`](test-env/manifest.json) — see +[`docs/internals/live-test-fixtures.md`](docs/internals/live-test-fixtures.md) +for bootstrap, principals, and cost. GHE is intentionally absent; +[`docs/internals/ghe-gap.md`](docs/internals/ghe-gap.md) explains +why and how to close it. + ## License MIT. See [LICENSE](LICENSE). diff --git a/VALIDATION.md b/VALIDATION.md index 70d2aee..deee89c 100644 --- a/VALIDATION.md +++ b/VALIDATION.md @@ -49,6 +49,23 @@ the DLL delay-load resolves. | id | what it proves | gate | |---|---|---| | `AGENT-FS-WRITE` | A live Copilot session's tool calls update the mounted workspace Fs, verified through both the host mount and the Fs directly. | `DEVDEV_LIVE_COPILOT=1` | +| `DAEMON-AGENT-FS-WRITE` | A `devdev up` daemon routes a live Copilot session through an injected MCP tool to mutate daemon-owned Fs state. | `DEVDEV_LIVE_COPILOT=1` | +| `FIXTURE-MANIFEST-INTEGRITY` | The CI-resettable live-test fixture manifest enforces its structural invariants and the `reset-comments` keep/delete decisions are correct (deterministic side; the fixture-state-matches-manifest side runs in CI only). | none | +| `LIVE-HOST-PROBE-GH` | `GitHubAdapter` round-trips the canonical fixture PR through real github.com REST. | `DEVDEV_LIVE_HOSTS=1` + fixture env | +| `LIVE-HOST-PROBE-ADO` | `AzureDevOpsAdapter` round-trips the canonical fixture PR through real `dev.azure.com` REST. | `DEVDEV_LIVE_HOSTS=1` + fixture env | +| `LIVE-CREDENTIAL-CHAIN-GH` | `GhCliProvider` produces a non-empty token from a real signed-in `gh` CLI, stamped with `TokenSource::GhCli`. | `DEVDEV_LIVE_CRED_GH=1` | +| `LIVE-CREDENTIAL-CHAIN-ADO` | `AzCliProvider` produces a non-empty AAD token from a real signed-in `az` CLI for the ADO resource. | `DEVDEV_LIVE_CRED_AZ=1` | +| `LIVE-ADO-PR-WRITE` | `AzureDevOpsAdapter::post_comment` lands a tagged comment on the canonical PR; `list_pr_comments` sees it. Cleanup removes it. | `DEVDEV_LIVE_HOSTS=1`, `DEVDEV_LIVE_WRITE=1` + fixture env | + +## Live tests in CI + +The four-stage live-tests pipeline lives in +[`.github/workflows/live-tests.yml`](.github/workflows/live-tests.yml). +Manual `workflow_dispatch` + nightly cron + label-gated PRs. The +fixture environment it provisions is documented in +[`docs/internals/live-test-fixtures.md`](docs/internals/live-test-fixtures.md); +the deliberate GHE gap and how to close it is documented in +[`docs/internals/ghe-gap.md`](docs/internals/ghe-gap.md). The list is deliberately short. Adding a claim means writing a real test that clears the rubric — not padding the manifest. diff --git a/claims.toml b/claims.toml index e895a27..d150db9 100644 --- a/claims.toml +++ b/claims.toml @@ -56,3 +56,101 @@ Scope of this claim: test = "cargo test -p devdev-cli --test live_daemon_fs_write -- --ignored devdev_up_agent_fs_write_lands_in_daemon_fs" requires_env = ["DEVDEV_LIVE_COPILOT=1"] requires_host = ["copilot on PATH, signed in"] + +[[claim]] +id = "FIXTURE-MANIFEST-INTEGRITY" +source = "docs/internals/live-test-fixtures.md" +line = 1 +text = """ +The live-test fixture environment is defined declaratively in +`test-env/manifest.json`. The manifest's structural invariants +(non-empty org/project/repo, bracketed comment-tag prefix, +canonical PR base equals default branch, no unknown fields) are +enforced by `devdev_test_env::manifest::Manifest::validate`, +and the comment-keep/delete decisions made by +`reset-comments` are pure functions (admin-login case-folded; +non-admin comments always swept; admin-tagged comments swept; +admin-untagged canonical notes preserved). + +This is the deterministic side of the live-test claim. The +fixture-state-matches-manifest side is a CI-only claim +(`cargo run -p devdev-test-env -- verify`) that requires admin +tokens and is therefore not in this manifest. +""" +test = "cargo test -p devdev-test-env --lib" +requires_env = [] +requires_host = [] + +[[claim]] +id = "LIVE-HOST-PROBE-GH" +source = "crates/devdev-cli/tests/live_host_probe.rs" +line = 1 +text = """ +`GitHubAdapter` (host_id = github_com) authenticates against +github.com with a fine-grained PAT, fetches the canonical fixture +PR by number, and returns it in `list_open_prs`. Asserts the +adapter's `host_id` stamp matches the host classification of the +fixture URL parsed by `PrRef`. +""" +test = "cargo test -p devdev-cli --test live_host_probe -- --ignored github_canonical_pr_round_trips" +requires_env = ["DEVDEV_LIVE_HOSTS=1", "DEVDEV_GH_TOKEN", "DEVDEV_GH_PR_URL"] +requires_host = ["live github.com fixture (goldenwitch/devdev-test-environment)"] + +[[claim]] +id = "LIVE-HOST-PROBE-ADO" +source = "crates/devdev-cli/tests/live_host_probe.rs" +line = 1 +text = """ +`AzureDevOpsAdapter` authenticates against dev.azure.com via +HTTP Basic, fetches the canonical fixture PR by number, and +returns it in `list_open_prs`. Asserts the adapter's +`host_id` stamp matches the host classification of the fixture +URL parsed by `PrRef` (`RepoHostId::azure_devops()`). +""" +test = "cargo test -p devdev-cli --test live_host_probe -- --ignored ado_canonical_pr_round_trips" +requires_env = ["DEVDEV_LIVE_HOSTS=1", "DEVDEV_ADO_TOKEN", "DEVDEV_ADO_PR_URL"] +requires_host = ["live ADO fixture (devdev-fixtures/DevDev-Live/devdev-test-environment)"] + +[[claim]] +id = "LIVE-CREDENTIAL-CHAIN-GH" +source = "crates/devdev-cli/tests/live_credential_chain.rs" +line = 1 +text = """ +`GhCliProvider` shells out to a real signed-in `gh` and +returns a `Credential` stamped with `TokenSource::GhCli` and +`host_id == github_com`. Proves the production credential path +works end-to-end against the actual CLI, not just a mock. +""" +test = "cargo test -p devdev-cli --test live_credential_chain -- --ignored gh_cli_provider_yields_token" +requires_env = ["DEVDEV_LIVE_CRED_GH=1"] +requires_host = ["gh CLI on PATH, signed in"] + +[[claim]] +id = "LIVE-CREDENTIAL-CHAIN-ADO" +source = "crates/devdev-cli/tests/live_credential_chain.rs" +line = 1 +text = """ +`AzCliProvider` shells out to a real signed-in `az` and +returns a `Credential` stamped with `TokenSource::AzCli` and +`host_id == azure_devops`. The token is requested with the ADO +well-known resource id (`499b84ac-...`). +""" +test = "cargo test -p devdev-cli --test live_credential_chain -- --ignored az_cli_provider_yields_token" +requires_env = ["DEVDEV_LIVE_CRED_AZ=1"] +requires_host = ["az CLI on PATH, signed in"] + +[[claim]] +id = "LIVE-ADO-PR-WRITE" +source = "crates/devdev-cli/tests/live_ado_pr.rs" +line = 1 +text = """ +`AzureDevOpsAdapter::post_comment` posts a tagged comment +(`[devdev-live-test:live_ado_pr:]`) on the canonical +fixture PR, and `list_pr_comments` returns it on the next read. +The cleanup job removes the comment after the run via +`devdev-test-env reset-comments`. This is the write-side proof +of the ADO adapter — the read-side claim is `LIVE-HOST-PROBE-ADO`. +""" +test = "cargo test -p devdev-cli --test live_ado_pr -- --ignored ado_canonical_pr_write_path" +requires_env = ["DEVDEV_LIVE_HOSTS=1", "DEVDEV_LIVE_WRITE=1", "DEVDEV_ADO_TOKEN", "DEVDEV_ADO_PR_URL"] +requires_host = ["live ADO fixture with reset-comments cleanup hook"] diff --git a/crates/devdev-cli/Cargo.toml b/crates/devdev-cli/Cargo.toml index 343c77c..12aa4a1 100644 --- a/crates/devdev-cli/Cargo.toml +++ b/crates/devdev-cli/Cargo.toml @@ -33,3 +33,4 @@ devdev-workspace = { path = "../devdev-workspace" } tempfile = { workspace = true } tokio = { workspace = true, features = ["full"] } tracing-subscriber = { workspace = true } +serial_test = { workspace = true } diff --git a/crates/devdev-cli/src/acp_backend.rs b/crates/devdev-cli/src/acp_backend.rs index 1b62865..ca99147 100644 --- a/crates/devdev-cli/src/acp_backend.rs +++ b/crates/devdev-cli/src/acp_backend.rs @@ -62,17 +62,12 @@ impl AcpSessionBackend { // On Windows, Copilot's `copilot(.cmd|.exe)` launcher // spawns a Node SEA that ignores `NODE_OPTIONS=--require`, // so our WinFSP realpath shim never reaches the agent. - // Rewrite to `node /index.js ...` so the shim - // applies. Leaves other agents and non-Windows hosts - // untouched. - let (program, args): (String, Vec) = - match crate::realpath_shim::rewrite_copilot_invocation( - &self.program, - &self.args, - ) { - Some(pair) => pair, - None => (self.program.clone(), self.args.clone()), - }; + // The shared `agent_command::prepare` helper resolves + // the binary via PATH+PATHEXT and applies the SEA-bypass + // rewrite so we always end up with an absolute path + // that `Command::new` can spawn directly. + let (program, args) = + crate::agent_command::prepare(&self.program, &self.args); let argv: Vec<&str> = args.iter().map(String::as_str).collect(); let client_config = AcpClientConfig { env_overrides: crate::realpath_shim::prepare_nodejs_options(), diff --git a/crates/devdev-cli/src/agent_command.rs b/crates/devdev-cli/src/agent_command.rs new file mode 100644 index 0000000..8bedd92 --- /dev/null +++ b/crates/devdev-cli/src/agent_command.rs @@ -0,0 +1,225 @@ +//! Resolve a user-supplied agent program (`copilot`, `./copilot.cmd`, +//! some custom binary) into a `(program, args)` pair that +//! [`tokio::process::Command::new`] can spawn reliably across hosts. +//! +//! The resolution is the **single canonical entry-point** for every +//! call site that spawns the ACP agent subprocess (the daemon's +//! [`crate::acp_backend`], live integration tests, ad-hoc PoCs). +//! Splitting this responsibility was costing us: +//! +//! 1. Bare `Command::new("copilot")` on Windows fails because the +//! OS resolver doesn't apply `PATHEXT` to `CreateProcess` for +//! extensionless names — every caller has to expand to +//! `copilot.cmd` itself. +//! 2. The Copilot CLI's `copilot(.cmd|.exe)` launcher invokes a +//! Node SEA prebuilt that ignores `NODE_OPTIONS=--require`, so +//! our WinFSP realpath shim never reaches the process that +//! handles `session/new`. We work around it by invoking +//! `node /index.js` directly. That rewrite needs the +//! *resolved* launcher path, not the bare name, so it must live +//! after the PATH search. +//! +//! The pipeline is: +//! +//! ```text +//! prepare(program, args) +//! = rewrite_copilot_sea_launcher( // Windows-only Node-SEA bypass +//! resolve_on_path(program), // PATH + PATHEXT lookup +//! args +//! ) +//! ``` +//! +//! Both steps are no-ops when not relevant. On non-Windows hosts the +//! function is a thin pass-through. + +use std::path::{Path, PathBuf}; + +/// Resolve the agent program and apply any Windows-specific launch +/// rewrites. See module docs for the rationale. +/// +/// On a missing executable the original `program` is returned +/// unchanged and `Command::spawn` will be the one that fails — that +/// keeps the error surface in one well-known place (the spawn site) +/// rather than splitting it between resolve-time and spawn-time. +pub fn prepare(program: &str, args: &[String]) -> (String, Vec) { + let resolved = resolve_on_path(program).unwrap_or_else(|| program.to_string()); + if let Some(rewritten) = rewrite_copilot_sea_launcher(&resolved, args) { + rewritten + } else { + (resolved, args.to_vec()) + } +} + +/// Walk `PATH` (and `PATHEXT` on Windows) to find an executable whose +/// stem matches `program`. Returns the absolute path on success. +/// +/// If `program` already contains a path separator or an extension, +/// it's treated as a path: returned verbatim if it exists, else +/// `None`. +pub fn resolve_on_path(program: &str) -> Option { + let prog_path = Path::new(program); + let has_separator = program.contains('/') || program.contains('\\'); + let has_extension = prog_path.extension().is_some(); + if has_separator || has_extension { + return prog_path + .is_file() + .then(|| prog_path.to_string_lossy().into_owned()); + } + + let path_var = std::env::var_os("PATH")?; + let exts = path_extensions(); + + for dir in std::env::split_paths(&path_var) { + for ext in &exts { + let candidate: PathBuf = if ext.is_empty() { + dir.join(program) + } else { + dir.join(format!("{program}{ext}")) + }; + if candidate.is_file() { + return Some(candidate.to_string_lossy().into_owned()); + } + } + } + None +} + +#[cfg(windows)] +fn path_extensions() -> Vec { + // PATHEXT is `.COM;.EXE;.BAT;.CMD;...`. Lowercase + strip leading + // dot so we can format `{program}{ext}` cleanly with the dot. + let raw = std::env::var("PATHEXT").unwrap_or_else(|_| ".COM;.EXE;.BAT;.CMD".into()); + let mut exts: Vec = raw + .split(';') + .map(|e| e.trim().to_ascii_lowercase()) + .filter(|e| !e.is_empty()) + .collect(); + // Also try the bare name in case the user registered a script + // with no extension. + exts.push(String::new()); + exts +} + +#[cfg(not(windows))] +fn path_extensions() -> Vec { + // Unix: no PATHEXT; the binary is named exactly as written. + vec![String::new()] +} + +/// Rewrite a `(program, args)` pair that launches Copilot via its +/// `copilot(.cmd|.ps1|.exe)` launcher so that it invokes +/// `node /index.js` directly instead. +/// +/// Why: Copilot's launcher runs `npm-loader.js`, which `spawnSync`s +/// the platform-specific **Node SEA** prebuilt (`@github/copilot- +/// win32-x64/copilot.exe`) as the actual agent process. Node SEAs +/// intentionally ignore `NODE_OPTIONS=--require ` as a +/// security measure, so our WinFSP realpath shim never reaches the +/// process that handles `session/new` — meaning Copilot rejects +/// every WinFSP cwd. +/// +/// Invoking `node index.js --acp ...` directly keeps all behaviour +/// identical (index.js sees `--acp` and imports `app.js` in-process, +/// which is the same code path the SEA runs) but preserves our +/// NODE_OPTIONS injection. Returns `None` on non-Windows hosts or +/// when the program is not recognizable as a Copilot launcher. +pub fn rewrite_copilot_sea_launcher( + program: &str, + args: &[String], +) -> Option<(String, Vec)> { + if !cfg!(target_os = "windows") { + return None; + } + let prog_path = Path::new(program); + let stem = prog_path + .file_stem() + .and_then(|s| s.to_str()) + .map(|s| s.to_ascii_lowercase()); + if stem.as_deref() != Some("copilot") { + return None; + } + let parent = prog_path.parent().filter(|p| !p.as_os_str().is_empty())?; + let index_js = parent + .join("node_modules") + .join("@github") + .join("copilot") + .join("index.js"); + if !index_js.is_file() { + tracing::warn!( + target: "devdev_cli::agent_command", + expected = %index_js.display(), + "copilot index.js not found next to launcher; leaving invocation as-is" + ); + return None; + } + let node_exe = parent.join("node.exe"); + let node = if node_exe.is_file() { + node_exe.display().to_string() + } else { + "node".to_string() + }; + let mut new_args = Vec::with_capacity(args.len() + 1); + new_args.push(index_js.display().to_string()); + new_args.extend(args.iter().cloned()); + tracing::info!( + target: "devdev_cli::agent_command", + node = %node, + index_js = %index_js.display(), + "bypassing Copilot SEA launcher so NODE_OPTIONS=--require applies" + ); + Some((node, new_args)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_returns_none_for_clearly_missing_binary() { + assert!(resolve_on_path("definitely-not-a-real-binary-xyz").is_none()); + } + + #[test] + fn resolve_passes_through_existing_path_with_separator() { + // Use a path that's guaranteed to exist on every supported host. + let exists = if cfg!(windows) { + "C:/Windows/System32/cmd.exe" + } else { + "/bin/sh" + }; + let resolved = resolve_on_path(exists).expect("path exists"); + assert!( + resolved.eq_ignore_ascii_case(exists) + || resolved.replace('\\', "/").eq_ignore_ascii_case(exists) + ); + } + + #[cfg(target_os = "windows")] + #[test] + fn rewrite_ignores_non_copilot_program() { + assert!(rewrite_copilot_sea_launcher("C:/Windows/System32/cmd.exe", &[]).is_none()); + assert!(rewrite_copilot_sea_launcher("node.exe", &[]).is_none()); + } + + #[cfg(target_os = "windows")] + #[test] + fn rewrite_returns_none_when_index_missing() { + assert!(rewrite_copilot_sea_launcher("C:/Windows/System32/copilot.exe", &[]).is_none()); + } + + #[cfg(target_os = "windows")] + #[test] + fn rewrite_returns_none_when_program_has_no_parent() { + // Bare "copilot" used to slip past the parent check via + // `prog_path.parent() == Some("")`; the explicit empty filter + // is what makes this case a clean None now. + assert!(rewrite_copilot_sea_launcher("copilot", &[]).is_none()); + } + + #[test] + fn prepare_falls_through_when_program_unresolvable() { + let (prog, args) = prepare("definitely-not-a-real-binary-xyz", &["--foo".into()]); + assert_eq!(prog, "definitely-not-a-real-binary-xyz"); + assert_eq!(args, vec!["--foo".to_string()]); + } +} diff --git a/crates/devdev-cli/src/daemon_cli.rs b/crates/devdev-cli/src/daemon_cli.rs index edd9f06..45a9d6b 100644 --- a/crates/devdev-cli/src/daemon_cli.rs +++ b/crates/devdev-cli/src/daemon_cli.rs @@ -35,7 +35,7 @@ use devdev_daemon::ledger::NdjsonLedger; use devdev_daemon::mcp::{DaemonToolProvider, McpServer}; use devdev_daemon::router::SessionRouter; use devdev_daemon::{Daemon, DaemonConfig, DaemonError, server}; -use devdev_integrations::{GitHubAdapter, LiveGitHubAdapter, MockGitHubAdapter}; +use devdev_integrations::{GitHubAdapter, MockAdapter, RepoHostAdapter}; use devdev_tasks::approval::{ApprovalPolicy, approval_channel}; use devdev_tasks::events::EventBus; use devdev_tasks::registry::TaskRegistry; @@ -176,21 +176,43 @@ fn resolve_data_dir(explicit: Option) -> PathBuf { explicit.unwrap_or_else(DaemonConfig::default_data_dir) } -fn select_github_adapter(flag: Option<&str>) -> Arc { +/// Build the default repo-host adapter from environment. +/// +/// Selection precedence: explicit `flag` > `DEVDEV_REPO_HOST_ADAPTER` +/// env var > `DEVDEV_GITHUB_ADAPTER` (legacy alias) > `"live"`. +/// `"live"` resolves to a github.com [`GitHubAdapter`] using the +/// `CredentialStore` snapshot (which already considered `GH_TOKEN` +/// and the `gh` CLI); if no credential is available we fall back to +/// the host-agnostic [`MockAdapter`] so dev/test flows still +/// progress. +/// +/// Multi-host wiring (GHE, ADO) is configured per repo in +/// preferences and resolved through the daemon-side host registry; +/// this function only seeds the *default* adapter for the legacy +/// single-host code paths that haven't migrated yet. +fn select_github_adapter( + flag: Option<&str>, + credentials: &devdev_daemon::credentials::CredentialStore, +) -> Arc { + use devdev_integrations::host::RepoHostId; + let choice = flag .map(ToOwned::to_owned) + .or_else(|| std::env::var("DEVDEV_REPO_HOST_ADAPTER").ok()) .or_else(|| std::env::var("DEVDEV_GITHUB_ADAPTER").ok()) .unwrap_or_else(|| "live".to_string()); match choice.as_str() { - "mock" => Arc::new(MockGitHubAdapter::new()), - _ => match LiveGitHubAdapter::from_env() { - Ok(adapter) => Arc::new(adapter), - Err(e) => { + "mock" => Arc::new(MockAdapter::new()), + _ => match credentials.get(&RepoHostId::github_com()) { + Some(cred) => Arc::new(GitHubAdapter::github_com( + cred.token().expose().to_string(), + )), + None => { eprintln!( - "devdev: GH_TOKEN not available ({e}); falling back to mock GitHub adapter" + "devdev: no github.com credential available; falling back to mock adapter (set GH_TOKEN or run `gh auth login`)" ); - Arc::new(MockGitHubAdapter::new()) + Arc::new(MockAdapter::new()) } }, } @@ -260,31 +282,47 @@ pub async fn run_up(args: UpArgs) -> Result<()> { // and a user-driven `fs/read` IPC call observe the same bytes. let fs = Arc::clone(&daemon.fs); - // Build the shared approval channel + secrets slot up-front so - // both the MCP provider (sender side, via `devdev_ask`) and the - // dispatch IPC (receiver side, via `approval_response`) can hold - // halves. + // Build the shared approval channel + credential snapshot up- + // front so both the MCP provider (sender side, via `devdev_ask`) + // and the dispatch IPC (receiver side, via `approval_response`) + // can hold halves. let policy = ApprovalPolicy::AutoApprove; let (gate, handle) = approval_channel(policy, APPROVAL_TIMEOUT); let approval_gate = Arc::new(Mutex::new(gate)); let approval_handle = Arc::new(Mutex::new(handle)); - let agent_secrets = Arc::new(Mutex::new(devdev_daemon::secrets::AgentSecrets::default())); - // Best-effort `gh auth token` sample — non-fatal if absent. - match devdev_daemon::secrets::try_read_gh_token().await { - Ok(Some(token)) => { - agent_secrets.lock().await.set_gh_token(Some(token)); - eprintln!("DevDev: gh token sampled (devdev_ask will hand it out on approval)"); - } - Ok(None) => { - eprintln!("DevDev: no gh token (run `gh auth login` to enable PR posting)"); + // Sample credentials exactly once. After this point the store is + // immutable; mutating env vars or `gh auth login`-ing will not + // affect tokens served from the snapshot until the daemon + // restarts. + let credentials = { + use devdev_daemon::credentials::{ + CredentialProvider, CredentialStore, EnvVarProvider, GhCliProvider, + }; + use devdev_integrations::host::RepoHostId; + + let github = RepoHostId::github_com(); + // Env var first (deterministic, scriptable), then `gh` CLI. + let providers: Vec> = vec![ + Arc::new(EnvVarProvider::new(github.clone(), "GH_TOKEN")), + Arc::new(GhCliProvider::new(github.clone())), + ]; + let store = CredentialStore::snapshot(providers).await; + match store.get(&github) { + Some(c) => eprintln!( + "DevDev: github.com credential captured (source: {:?}); devdev_ask will release on approval", + c.source() + ), + None => eprintln!( + "DevDev: no github.com credential (set GH_TOKEN or run `gh auth login`)" + ), } - Err(e) => eprintln!("DevDev: gh auth token failed: {e}"), - } + Arc::new(store) + }; let mcp_provider = Arc::new( DaemonToolProvider::new(Arc::clone(&tasks), Arc::clone(&fs)) - .with_ask(Arc::clone(&approval_gate), Arc::clone(&agent_secrets)), + .with_ask(Arc::clone(&approval_gate), Arc::clone(&credentials)), ); let mcp_server = McpServer::start(mcp_provider) .await @@ -298,7 +336,18 @@ pub async fn run_up(args: UpArgs) -> Result<()> { Some(mcp_endpoint), )); let router = Arc::new(SessionRouter::new(backend)); - let github = select_github_adapter(args.github.as_deref()); + let github = select_github_adapter(args.github.as_deref(), &credentials); + // Multi-host registry. Today we only seed the github.com slot + // from the default adapter; preferences-driven population (one + // entry per `[[repo]]` block) lands as a follow-up. + let host_registry = { + use devdev_daemon::host_registry::RepoHostRegistry; + use devdev_integrations::host::RepoHostId; + Arc::new(RepoHostRegistry::single( + RepoHostId::github_com(), + Arc::clone(&github), + )) + }; let event_bus = EventBus::new(); let ledger_path = data_dir.join("ledger.ndjson"); @@ -318,12 +367,13 @@ pub async fn run_up(args: UpArgs) -> Result<()> { router, tasks, github, + host_registry, approval_gate, approval_handle, event_bus, ledger, policy, - agent_secrets, + credentials, shutdown_tx.clone(), fs, )); diff --git a/crates/devdev-cli/src/lib.rs b/crates/devdev-cli/src/lib.rs index db88e76..e1d03bb 100644 --- a/crates/devdev-cli/src/lib.rs +++ b/crates/devdev-cli/src/lib.rs @@ -1,6 +1,7 @@ //! DevDev CLI library: daemon subcommands. pub mod acp_backend; +pub mod agent_command; pub mod daemon_cli; pub mod preferences; pub mod realpath_shim; diff --git a/crates/devdev-cli/src/realpath_shim.rs b/crates/devdev-cli/src/realpath_shim.rs index 8097ee9..40d21eb 100644 --- a/crates/devdev-cli/src/realpath_shim.rs +++ b/crates/devdev-cli/src/realpath_shim.rs @@ -129,67 +129,6 @@ pub fn prepare_nodejs_options() -> Vec<(String, String)> { result } -/// Rewrite a `(program, args)` pair that launches Copilot via its -/// `copilot(.cmd|.ps1|.exe)` launcher so that it invokes -/// `node /index.js` directly instead. -/// -/// Why: Copilot's launcher runs `npm-loader.js`, which `spawnSync`s the -/// platform-specific **Node SEA** prebuilt (`@github/copilot-win32-x64/ -/// copilot.exe`) as the actual agent process. Node SEAs intentionally -/// ignore `NODE_OPTIONS=--require ` as a security measure, so our -/// WinFSP realpath shim never reaches the process that handles -/// `session/new` — meaning Copilot rejects every WinFSP cwd. -/// -/// Invoking `node index.js --acp ...` directly keeps all behaviour -/// identical (index.js sees `--acp` and imports `app.js` in-process, -/// which is the same code path the SEA runs) but preserves our -/// NODE_OPTIONS injection. Returns `None` on non-Windows hosts or when -/// the program is not recognizable as a Copilot launcher, so callers -/// can leave the invocation unchanged in that case. -pub fn rewrite_copilot_invocation(program: &str, args: &[String]) -> Option<(String, Vec)> { - if !cfg!(target_os = "windows") { - return None; - } - let prog_path = std::path::Path::new(program); - let stem = prog_path - .file_stem() - .and_then(|s| s.to_str()) - .map(|s| s.to_ascii_lowercase()); - if stem.as_deref() != Some("copilot") { - return None; - } - let parent = prog_path.parent()?; - let index_js = parent - .join("node_modules") - .join("@github") - .join("copilot") - .join("index.js"); - if !index_js.is_file() { - tracing::warn!( - target: "devdev_cli::realpath_shim", - expected = %index_js.display(), - "copilot index.js not found next to launcher; leaving invocation as-is" - ); - return None; - } - let node_exe = parent.join("node.exe"); - let node = if node_exe.is_file() { - node_exe.display().to_string() - } else { - "node".to_string() - }; - let mut new_args = Vec::with_capacity(args.len() + 1); - new_args.push(index_js.display().to_string()); - new_args.extend(args.iter().cloned()); - tracing::info!( - target: "devdev_cli::realpath_shim", - node = %node, - index_js = %index_js.display(), - "bypassing Copilot SEA launcher so NODE_OPTIONS=--require applies" - ); - Some((node, new_args)) -} - fn write_shim_to_temp() -> std::io::Result { let dir = std::env::temp_dir().join("devdev-realpath-shim"); std::fs::create_dir_all(&dir)?; @@ -235,18 +174,4 @@ mod tests { let body = std::fs::read_to_string(path).expect("shim file readable"); assert!(body.contains("patchedPromisesRealpath")); } - - #[cfg(target_os = "windows")] - #[test] - fn rewrite_ignores_non_copilot_program() { - assert!(rewrite_copilot_invocation("C:/Windows/System32/cmd.exe", &[]).is_none()); - assert!(rewrite_copilot_invocation("node.exe", &[]).is_none()); - } - - #[cfg(target_os = "windows")] - #[test] - fn rewrite_returns_none_when_index_missing() { - // A real path that exists but has no adjacent copilot index.js. - assert!(rewrite_copilot_invocation("C:/Windows/System32/copilot.exe", &[]).is_none()); - } } diff --git a/crates/devdev-cli/tests/live_ado_pr.rs b/crates/devdev-cli/tests/live_ado_pr.rs new file mode 100644 index 0000000..b51ffe5 --- /dev/null +++ b/crates/devdev-cli/tests/live_ado_pr.rs @@ -0,0 +1,137 @@ +//! Live ADO PR test — proves [`AzureDevOpsAdapter`] talks to real +//! Azure DevOps Services and that the per-PR thread/comment surface +//! works end-to-end. +//! +//! ## Read-mode (default) +//! +//! Resolves `DEVDEV_ADO_PR_URL` via [`PrRef::parse`], constructs the +//! adapter, and exercises: +//! +//! 1. `get_pr` — round-trips title and number. +//! 2. `list_pr_comments` — succeeds (empty or non-empty is fine; the +//! cleanup job deletes tagged comments after every run). +//! +//! Gated by `DEVDEV_LIVE_HOSTS=1`. +//! +//! ## Write-mode +//! +//! Additionally posts a *tagged* comment of the form +//! `[devdev-live-test:live_ado_pr:] hello from `, then +//! verifies it appears in the next `list_pr_comments` response. +//! +//! Gated by `DEVDEV_LIVE_WRITE=1`. The CI workflow's `cleanup` job +//! sweeps the comment afterwards via `devdev-test-env reset-comments`. +//! Locally, you have to clean up by hand or re-run with the same +//! admin token. +//! +//! Marked `#[serial]` because the comment-tag nonce uses a wall-clock +//! second; running concurrent invocations could (in theory) collide. + +use devdev_integrations::host::RepoHostId; +use devdev_integrations::{AzureDevOpsAdapter, RepoHostAdapter}; +use devdev_tasks::pr_ref::PrRef; +use serial_test::serial; + +fn flag(var: &str) -> bool { + std::env::var(var) + .ok() + .map(|v| matches!(v.to_lowercase().as_str(), "1" | "true" | "yes")) + .unwrap_or(false) +} + +fn require_env(name: &str) -> Option { + match std::env::var(name) { + Ok(v) if !v.is_empty() => Some(v), + _ => { + eprintln!("SKIP: {name} not set"); + None + } + } +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: requires ADO fixture; run with DEVDEV_LIVE_HOSTS=1 --ignored"] +#[serial] +async fn ado_canonical_pr_read_path() { + if !flag("DEVDEV_LIVE_HOSTS") { + eprintln!("SKIP: DEVDEV_LIVE_HOSTS not set"); + return; + } + let token = match require_env("DEVDEV_ADO_TOKEN") { + Some(v) => v, + None => return, + }; + let pr_url = match require_env("DEVDEV_ADO_PR_URL") { + Some(v) => v, + None => return, + }; + + let parsed = PrRef::parse(&pr_url).expect("PrRef::parse fixture URL"); + assert_eq!(parsed.host_id, RepoHostId::azure_devops()); + + let adapter = AzureDevOpsAdapter::new(token); + let pr = adapter + .get_pr(&parsed.owner, &parsed.repo, parsed.number) + .await + .expect("get_pr"); + assert_eq!(pr.number, parsed.number); + + // Read-side smoke: list_pr_comments must succeed regardless of + // whether the cleanup left it empty. + let _ = adapter + .list_pr_comments(&parsed.owner, &parsed.repo, parsed.number) + .await + .expect("list_pr_comments"); +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: posts a tagged comment; run with DEVDEV_LIVE_HOSTS=1 DEVDEV_LIVE_WRITE=1 --ignored"] +#[serial] +async fn ado_canonical_pr_write_path() { + if !flag("DEVDEV_LIVE_HOSTS") { + eprintln!("SKIP: DEVDEV_LIVE_HOSTS not set"); + return; + } + if !flag("DEVDEV_LIVE_WRITE") { + eprintln!("SKIP: DEVDEV_LIVE_WRITE not set"); + return; + } + let token = match require_env("DEVDEV_ADO_TOKEN") { + Some(v) => v, + None => return, + }; + let pr_url = match require_env("DEVDEV_ADO_PR_URL") { + Some(v) => v, + None => return, + }; + + let parsed = PrRef::parse(&pr_url).expect("PrRef::parse fixture URL"); + let adapter = AzureDevOpsAdapter::new(token); + + let nonce = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let body = format!( + "[devdev-live-test:live_ado_pr:{nonce}] hello from devdev-cli live test" + ); + + adapter + .post_comment(&parsed.owner, &parsed.repo, parsed.number, &body) + .await + .expect("post_comment"); + + // Verify it landed. + let comments = adapter + .list_pr_comments(&parsed.owner, &parsed.repo, parsed.number) + .await + .expect("list_pr_comments after post"); + + let needle = format!("live_ado_pr:{nonce}"); + let found = comments.iter().any(|c| c.body.contains(&needle)); + assert!( + found, + "tagged comment not visible in PR thread (n={}, needle={needle})", + comments.len() + ); +} diff --git a/crates/devdev-cli/tests/live_credential_chain.rs b/crates/devdev-cli/tests/live_credential_chain.rs new file mode 100644 index 0000000..22dd3b3 --- /dev/null +++ b/crates/devdev-cli/tests/live_credential_chain.rs @@ -0,0 +1,91 @@ +//! Live credential chain — proves the production credential +//! providers resolve real tokens against real CLIs. +//! +//! This test does **not** assert provider precedence (that's a unit +//! test in `devdev-daemon::credentials`). It just proves each +//! provider, on its own, can talk to the CLI it shells out to and +//! produce a non-empty token under realistic conditions. +//! +//! ## Scope +//! +//! * `gh_cli_provider_yields_token` — runs `gh auth token` via +//! [`GhCliProvider`]. Gated by `DEVDEV_LIVE_CRED_GH=1`. Requires +//! `gh` on PATH and an authenticated session (the CI workflow +//! primes this with `gh auth login --with-token `). +//! * `az_cli_provider_yields_token` — runs `az account get-access-token` +//! via [`AzCliProvider`]. Gated by `DEVDEV_LIVE_CRED_AZ=1`. Requires +//! `az` on PATH and a logged-in session (CI uses `azure/login`). +//! +//! Both tests assert the resulting credential's `host_id` matches the +//! one the provider was constructed for; the token itself is touched +//! through `expose()` only to verify it's non-empty. + +use devdev_daemon::credentials::{AzCliProvider, CredentialProvider, GhCliProvider, TokenSource}; +use devdev_integrations::host::RepoHostId; + +fn flag(var: &str) -> bool { + std::env::var(var) + .ok() + .map(|v| matches!(v.to_lowercase().as_str(), "1" | "true" | "yes")) + .unwrap_or(false) +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: requires gh CLI signed in; run with DEVDEV_LIVE_CRED_GH=1 --ignored"] +async fn gh_cli_provider_yields_token() { + if !flag("DEVDEV_LIVE_CRED_GH") { + eprintln!("SKIP: DEVDEV_LIVE_CRED_GH not set"); + return; + } + let host = RepoHostId::github_com(); + let provider = GhCliProvider::new(host.clone()); + + let cred = provider + .fetch() + .await + .expect("gh CLI provider returned an I/O error") + .expect("gh CLI provider returned None — is `gh auth login` complete?"); + + assert_eq!(cred.host_id(), &host, "credential stamped wrong host_id"); + assert!( + matches!(cred.source(), TokenSource::GhCli), + "credential source not stamped as GhCli: {:?}", + cred.source() + ); + let token = cred.token().expose(); + assert!(!token.is_empty(), "gh auth token returned an empty token"); + // Light shape check: tokens issued by gh start with one of a few + // known prefixes. Don't assert the prefix value — that's an + // implementation detail of GitHub. Just that it parses as ASCII. + assert!( + token.is_ascii(), + "non-ASCII payload in gh-issued token (suspicious)" + ); +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: requires az CLI signed in; run with DEVDEV_LIVE_CRED_AZ=1 --ignored"] +async fn az_cli_provider_yields_token() { + if !flag("DEVDEV_LIVE_CRED_AZ") { + eprintln!("SKIP: DEVDEV_LIVE_CRED_AZ not set"); + return; + } + let host = RepoHostId::azure_devops(); + let provider = AzCliProvider::new(host.clone()); + + let cred = provider + .fetch() + .await + .expect("az CLI provider returned an I/O error") + .expect("az CLI provider returned None — is `az login` complete?"); + + assert_eq!(cred.host_id(), &host); + assert!( + matches!(cred.source(), TokenSource::AzCli), + "credential source not stamped as AzCli: {:?}", + cred.source() + ); + let token = cred.token().expose(); + assert!(!token.is_empty(), "az get-access-token returned empty"); + assert!(token.is_ascii(), "non-ASCII payload in AAD token"); +} diff --git a/crates/devdev-cli/tests/live_daemon_fs_write.rs b/crates/devdev-cli/tests/live_daemon_fs_write.rs index 2cd314c..5e65b02 100644 --- a/crates/devdev-cli/tests/live_daemon_fs_write.rs +++ b/crates/devdev-cli/tests/live_daemon_fs_write.rs @@ -57,24 +57,12 @@ fn live_enabled() -> bool { .unwrap_or(false) } -#[cfg(windows)] +/// Resolve the `copilot` binary using the same logic the daemon +/// uses at spawn time. Keeps the test in sync with production: a +/// Windows-specific PATHEXT search via +/// [`devdev_cli::agent_command::resolve_on_path`]. fn which_copilot() -> Option { - let path = std::env::var("PATH").ok()?; - for dir in path.split(';') { - for ext in &[".cmd", ".bat", ".exe"] { - let candidate = std::path::Path::new(dir).join(format!("copilot{ext}")); - if candidate.is_file() { - return Some(candidate.display().to_string()); - } - } - } - None -} - -#[cfg(not(windows))] -fn which_copilot() -> Option { - // Trust PATH; the child process resolves it. - Some("copilot".to_string()) + devdev_cli::agent_command::resolve_on_path("copilot") } fn init_tracing() { diff --git a/crates/devdev-cli/tests/live_host_probe.rs b/crates/devdev-cli/tests/live_host_probe.rs new file mode 100644 index 0000000..0ca1012 --- /dev/null +++ b/crates/devdev-cli/tests/live_host_probe.rs @@ -0,0 +1,153 @@ +//! Live host probe — smoke test that the hand-rolled REST clients in +//! `devdev-integrations` can authenticate against both fixture hosts +//! and round-trip a known PR. +//! +//! ## Scope +//! +//! Read-only. Resolves the canonical fixture PR on each host, calls +//! `get_pr` + `list_open_prs`, and asserts the host_id stamp on the +//! returned record matches the host the adapter was constructed for. +//! +//! ## Running +//! +//! Opt-in, `--ignored`, gated by `DEVDEV_LIVE_HOSTS=1`. The CI +//! workflow's `live-tests` job exports the per-host fixture +//! coordinates via `devdev-test-env print-env` after `provision` +//! lands the manifest.lock. Locally: +//! +//! ```pwsh +//! $env:DEVDEV_LIVE_HOSTS = "1" +//! $env:DEVDEV_GH_TOKEN = "" +//! $env:DEVDEV_ADO_TOKEN = "" +//! cargo run -p devdev-test-env -- print-env | Out-File .env.live +//! . ./.env.live +//! cargo test -p devdev-cli --test live_host_probe -- --ignored --nocapture +//! ``` +//! +//! If `DEVDEV_LIVE_HOSTS` is not `1`, both tests skip with a clear +//! message rather than masquerading as a pass. + +use devdev_integrations::host::RepoHostId; +use devdev_integrations::{AzureDevOpsAdapter, GitHubAdapter, RepoHostAdapter}; + +fn live_enabled() -> bool { + std::env::var("DEVDEV_LIVE_HOSTS") + .ok() + .map(|v| matches!(v.to_lowercase().as_str(), "1" | "true" | "yes")) + .unwrap_or(false) +} + +fn require_env(name: &str) -> Option { + match std::env::var(name) { + Ok(v) if !v.is_empty() => Some(v), + _ => { + eprintln!( + "SKIP: {name} not set; cannot run live host probe (did `devdev-test-env print-env` run?)" + ); + None + } + } +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: requires fixture environment; run with DEVDEV_LIVE_HOSTS=1 --ignored"] +async fn github_canonical_pr_round_trips() { + if !live_enabled() { + eprintln!("SKIP: DEVDEV_LIVE_HOSTS not set"); + return; + } + let token = match require_env("DEVDEV_GH_TOKEN") { + Some(v) => v, + None => return, + }; + let pr_url = match require_env("DEVDEV_GH_PR_URL") { + Some(v) => v, + None => return, + }; + + // Parse: https://github.com///pull/ + let parsed = devdev_tasks::pr_ref::PrRef::parse(&pr_url) + .unwrap_or_else(|e| panic!("PrRef::parse({pr_url:?}): {e}")); + assert_eq!( + parsed.host_id, + RepoHostId::github_com(), + "fixture URL classified as the wrong host" + ); + + let adapter = GitHubAdapter::new(RepoHostId::github_com(), token); + let pr = adapter + .get_pr(&parsed.owner, &parsed.repo, parsed.number) + .await + .unwrap_or_else(|e| panic!("get_pr against canonical fixture failed: {e}")); + + assert_eq!(pr.number, parsed.number, "round-tripped wrong PR number"); + assert!( + pr.title.contains("Canonical fixture"), + "fixture PR title drifted: {:?}", + pr.title + ); + assert_eq!( + adapter.host_id(), + &RepoHostId::github_com(), + "adapter host_id stamp drifted" + ); + + let open = adapter + .list_open_prs(&parsed.owner, &parsed.repo) + .await + .unwrap_or_else(|e| panic!("list_open_prs failed: {e}")); + assert!( + open.iter().any(|p| p.number == parsed.number), + "canonical PR not in list_open_prs result (n={})", + open.len() + ); +} + +#[tokio::test(flavor = "current_thread")] +#[ignore = "live: requires fixture environment; run with DEVDEV_LIVE_HOSTS=1 --ignored"] +async fn ado_canonical_pr_round_trips() { + if !live_enabled() { + eprintln!("SKIP: DEVDEV_LIVE_HOSTS not set"); + return; + } + let token = match require_env("DEVDEV_ADO_TOKEN") { + Some(v) => v, + None => return, + }; + let pr_url = match require_env("DEVDEV_ADO_PR_URL") { + Some(v) => v, + None => return, + }; + + let parsed = devdev_tasks::pr_ref::PrRef::parse(&pr_url) + .unwrap_or_else(|e| panic!("PrRef::parse({pr_url:?}): {e}")); + assert_eq!( + parsed.host_id, + RepoHostId::azure_devops(), + "fixture URL classified as the wrong host" + ); + + let adapter = AzureDevOpsAdapter::new(token); + let pr = adapter + .get_pr(&parsed.owner, &parsed.repo, parsed.number) + .await + .unwrap_or_else(|e| panic!("get_pr against canonical ADO fixture failed: {e}")); + + assert_eq!(pr.number, parsed.number); + assert!( + pr.title.contains("Canonical fixture"), + "fixture PR title drifted: {:?}", + pr.title + ); + assert_eq!(adapter.host_id(), &RepoHostId::azure_devops()); + + let open = adapter + .list_open_prs(&parsed.owner, &parsed.repo) + .await + .unwrap_or_else(|e| panic!("list_open_prs failed: {e}")); + assert!( + open.iter().any(|p| p.number == parsed.number), + "canonical PR not in list_open_prs result (n={})", + open.len() + ); +} diff --git a/crates/devdev-daemon/src/credentials.rs b/crates/devdev-daemon/src/credentials.rs new file mode 100644 index 0000000..56cb2ee --- /dev/null +++ b/crates/devdev-daemon/src/credentials.rs @@ -0,0 +1,597 @@ +//! Credential snapshot for repository-host authentication. +//! +//! # Lifecycle contract +//! +//! Credentials are sampled **once**, at `devdev up` time, by running +//! a fixed set of [`CredentialProvider`]s against a fixed set of +//! [`RepoHostId`]s. The resulting [`CredentialStore`] is then +//! frozen — its inner `HashMap` is wrapped in `Arc`, callers receive +//! shared references, and there is no public mutation API after +//! [`CredentialStore::snapshot`] returns. +//! +//! This makes per-request credential reads deterministic. Mutating +//! `GH_TOKEN` (or any other env var or CLI session) after the daemon +//! has booted has **no effect** on tokens served from the snapshot. +//! Callers that need rotation must restart the daemon. +//! +//! # Provider model +//! +//! A [`CredentialProvider`] is bound to one [`RepoHostId`] and knows +//! how to fetch a token for it. Built-in providers: +//! * [`EnvVarProvider`] — reads a named environment variable. +//! * [`GhCliProvider`] — shells out to `gh auth token` (github.com only). +//! * [`AzCliProvider`] — shells out to `az account get-access-token` +//! with the ADO resource id (Azure DevOps only). +//! +//! The [`CredentialProvider`] trait is `async` and returns `Result< +//! Option, io::Error>`: `Ok(None)` for "this provider had +//! nothing to offer" (so the next provider for the same host is +//! tried), `Err` only for genuine I/O failures. +//! +//! # Token redaction +//! +//! Tokens are wrapped in [`RedactedString`], which redacts on `Debug` +//! and `Display` and only releases the raw value via +//! [`RedactedString::expose`]. This keeps tokens out of trace logs +//! and panic messages by default; callers must opt in explicitly. + +use std::collections::HashMap; +use std::sync::Arc; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +use async_trait::async_trait; +use devdev_integrations::host::RepoHostId; +use tokio::process::Command; + +/// A `String` newtype whose `Debug`/`Display` impls redact the value. +/// +/// Construct via [`RedactedString::new`]; expose the raw value via +/// [`RedactedString::expose`]. `Clone` is intentional; equality is +/// not implemented (callers should not branch on token contents). +#[derive(Clone)] +pub struct RedactedString(String); + +impl RedactedString { + pub fn new(s: impl Into) -> Self { + Self(s.into()) + } + + /// Borrow the raw string. Use sparingly; the redaction is the + /// whole point of this type. + pub fn expose(&self) -> &str { + &self.0 + } + + pub fn into_inner(self) -> String { + self.0 + } +} + +impl std::fmt::Debug for RedactedString { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "RedactedString([redacted; {} bytes])", self.0.len()) + } +} + +impl std::fmt::Display for RedactedString { + fn fmt(&self, _: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + Ok(()) // empty — never print a token by accident + } +} + +/// Where a credential came from. Recorded for diagnostics and +/// observable via [`Credential::source`]; not used for routing. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum TokenSource { + /// Read from environment variable `name`. + EnvVar { name: String }, + /// Captured via `gh auth token` (github.com). + GhCli, + /// Captured via `az account get-access-token` (Azure DevOps). + AzCli, + /// Test-only: injected directly via [`CredentialStore::with_entry`]. + Injected, +} + +/// One credential entry in the snapshot. `sampled_at` is the wall- +/// clock time at snapshot construction; `expires_at_hint` exists to +/// train downstream consumers not to cache without bound, but is not +/// enforced here. +#[derive(Debug, Clone)] +pub struct Credential { + host_id: RepoHostId, + token: RedactedString, + source: TokenSource, + sampled_at: SystemTime, +} + +impl Credential { + /// Construct a credential from raw parts. Callers in production + /// should let [`CredentialProvider`]s build credentials; this + /// constructor is exposed for test injection and provider impls. + pub fn new( + host_id: RepoHostId, + token: impl Into, + source: TokenSource, + ) -> Self { + Self { + host_id, + token: RedactedString::new(token), + source, + sampled_at: SystemTime::now(), + } + } + + pub fn host_id(&self) -> &RepoHostId { + &self.host_id + } + + pub fn token(&self) -> &RedactedString { + &self.token + } + + pub fn source(&self) -> &TokenSource { + &self.source + } + + pub fn sampled_at(&self) -> SystemTime { + self.sampled_at + } + + /// Wall-clock seconds since epoch when this credential was sampled. + pub fn sampled_at_unix(&self) -> Option { + self.sampled_at + .duration_since(UNIX_EPOCH) + .ok() + .map(|d| d.as_secs()) + } + + /// Conservative one-hour validity hint, surfaced to MCP clients + /// via the `expires_at` field on `AskResponse::Approved`. + pub fn expires_at_hint(&self) -> Option { + const ONE_HOUR_SECS: u64 = 3600; + self.sampled_at_unix().map(|t| t + ONE_HOUR_SECS) + } +} + +/// Async fetcher for one [`RepoHostId`]'s credential. +/// +/// Implementations are constructed with the host id they serve and +/// returned from [`CredentialProvider::host_id`] — the snapshot +/// driver uses that to key entries in the resulting store. +#[async_trait] +pub trait CredentialProvider: Send + Sync { + fn host_id(&self) -> &RepoHostId; + + /// Try to produce a credential. `Ok(None)` means "no credential + /// available from this provider"; the snapshot driver moves on + /// to the next provider for the same host (if any). + async fn fetch(&self) -> std::io::Result>; +} + +// ── Built-in providers ────────────────────────────────────────── + +/// Reads a token from a named environment variable. Empty values +/// are treated as missing. +pub struct EnvVarProvider { + host_id: RepoHostId, + var_name: String, +} + +impl EnvVarProvider { + pub fn new(host_id: RepoHostId, var_name: impl Into) -> Self { + Self { + host_id, + var_name: var_name.into(), + } + } + + pub fn var_name(&self) -> &str { + &self.var_name + } +} + +#[async_trait] +impl CredentialProvider for EnvVarProvider { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + + async fn fetch(&self) -> std::io::Result> { + match std::env::var(&self.var_name) { + Ok(v) if !v.is_empty() => Ok(Some(Credential::new( + self.host_id.clone(), + v, + TokenSource::EnvVar { + name: self.var_name.clone(), + }, + ))), + _ => Ok(None), + } + } +} + +/// Shells out to `gh auth token`. Only meaningful for github.com; +/// GHE installs need explicit env vars (or a future GHE-aware CLI +/// provider) since `gh` doesn't model multi-host credentials cleanly. +pub struct GhCliProvider { + host_id: RepoHostId, + timeout: Duration, +} + +impl GhCliProvider { + pub fn new(host_id: RepoHostId) -> Self { + Self { + host_id, + timeout: Duration::from_secs(5), + } + } + + /// Override the subprocess timeout. Defaults to 5 s. + pub fn with_timeout(mut self, t: Duration) -> Self { + self.timeout = t; + self + } +} + +#[async_trait] +impl CredentialProvider for GhCliProvider { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + + async fn fetch(&self) -> std::io::Result> { + let mut cmd = Command::new("gh"); + cmd.arg("auth").arg("token"); + cmd.stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()); + let output = match tokio::time::timeout(self.timeout, cmd.output()).await { + Ok(r) => r?, + Err(_) => return Ok(None), + }; + if !output.status.success() { + return Ok(None); + } + let token = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if token.is_empty() { + Ok(None) + } else { + Ok(Some(Credential::new( + self.host_id.clone(), + token, + TokenSource::GhCli, + ))) + } + } +} + +/// Shells out to `az account get-access-token --resource ` +/// to obtain an AAD bearer token usable against ADO REST endpoints. +/// +/// Note: ADO PATs are typically used for non-interactive automation +/// today, but `az`-issued AAD tokens are also accepted and have a +/// shorter blast radius. Operators preferring PATs should configure +/// an [`EnvVarProvider`] on `ADO_TOKEN` instead. +pub struct AzCliProvider { + host_id: RepoHostId, + timeout: Duration, +} + +impl AzCliProvider { + /// ADO's well-known AAD application id; used as `--resource`. + pub const ADO_RESOURCE_ID: &'static str = "499b84ac-1321-427f-aa17-267ca6975798"; + + pub fn new(host_id: RepoHostId) -> Self { + Self { + host_id, + timeout: Duration::from_secs(10), + } + } + + pub fn with_timeout(mut self, t: Duration) -> Self { + self.timeout = t; + self + } +} + +#[async_trait] +impl CredentialProvider for AzCliProvider { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + + async fn fetch(&self) -> std::io::Result> { + let mut cmd = Command::new("az"); + cmd.arg("account") + .arg("get-access-token") + .arg("--resource") + .arg(Self::ADO_RESOURCE_ID) + .arg("--query") + .arg("accessToken") + .arg("-o") + .arg("tsv"); + cmd.stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()); + let output = match tokio::time::timeout(self.timeout, cmd.output()).await { + Ok(r) => r?, + Err(_) => return Ok(None), + }; + if !output.status.success() { + return Ok(None); + } + let token = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if token.is_empty() { + Ok(None) + } else { + Ok(Some(Credential::new( + self.host_id.clone(), + token, + TokenSource::AzCli, + ))) + } + } +} + +// ── Frozen store ──────────────────────────────────────────────── + +/// Frozen, snapshot-once credential lookup table. +/// +/// Construction goes through [`CredentialStore::snapshot`] (production) +/// or [`CredentialStore::with_entries`] / [`CredentialStore::with_entry`] +/// (tests). After construction the inner table is `Arc`-wrapped and +/// shared by reference; there is no mutation API. +#[derive(Debug, Clone, Default)] +pub struct CredentialStore { + entries: Arc>, +} + +impl CredentialStore { + /// Empty store. Useful for tests and for `--no-credentials` boots. + pub fn empty() -> Self { + Self::default() + } + + /// Build a store directly from a list of credentials. Last entry + /// for a given host wins. Test-only sugar. + pub fn with_entries(creds: impl IntoIterator) -> Self { + let mut map = HashMap::new(); + for c in creds { + map.insert(c.host_id().clone(), c); + } + Self { + entries: Arc::new(map), + } + } + + /// Convenience: a one-entry store. Equivalent to + /// `with_entries([Credential::new(host, token, Injected)])`. + pub fn with_entry(host_id: RepoHostId, token: impl Into) -> Self { + Self::with_entries([Credential::new(host_id, token, TokenSource::Injected)]) + } + + /// Run all providers in declaration order, keeping the **first** + /// non-`None` result per [`RepoHostId`]. Errors from individual + /// providers are logged and treated as `None`, so a single broken + /// provider can't take down boot. + /// + /// Multiple providers for the same host id are allowed and form + /// a fallback chain. Distinct host ids are independent. + pub async fn snapshot(providers: Vec>) -> Self { + let mut map: HashMap = HashMap::new(); + for provider in providers { + let host = provider.host_id().clone(); + if map.contains_key(&host) { + continue; // earlier provider already won + } + match provider.fetch().await { + Ok(Some(cred)) => { + map.insert(host, cred); + } + Ok(None) => {} + Err(e) => { + tracing::warn!( + host = %host, + error = %e, + "credential provider failed; treating as no credential", + ); + } + } + } + Self { + entries: Arc::new(map), + } + } + + /// Resolve a credential by host id. Returns `None` if no provider + /// produced one at snapshot time. + pub fn get(&self, host_id: &RepoHostId) -> Option<&Credential> { + self.entries.get(host_id) + } + + /// Number of entries. Diagnostic only. + pub fn len(&self) -> usize { + self.entries.len() + } + + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } + + /// Iterate entries (host_id, credential). Order is unspecified. + pub fn iter(&self) -> impl Iterator { + self.entries.iter() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn gh() -> RepoHostId { + RepoHostId::github_com() + } + + fn ado() -> RepoHostId { + RepoHostId::azure_devops() + } + + // ── RedactedString ───────────────────────────────────────── + + #[test] + fn redacted_string_does_not_leak_via_debug_or_display() { + let r = RedactedString::new("ghp_super_secret"); + let d = format!("{r:?}"); + let s = format!("{r}"); + assert!(!d.contains("ghp_super_secret"), "debug leaked: {d}"); + assert!(!s.contains("ghp_super_secret"), "display leaked: {s}"); + assert_eq!(r.expose(), "ghp_super_secret"); + } + + // ── Snapshot lifecycle ───────────────────────────────────── + + struct StaticProvider { + host_id: RepoHostId, + token: Option<&'static str>, + } + + #[async_trait] + impl CredentialProvider for StaticProvider { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + async fn fetch(&self) -> std::io::Result> { + Ok(self + .token + .map(|t| Credential::new(self.host_id.clone(), t, TokenSource::Injected))) + } + } + + #[tokio::test] + async fn snapshot_records_token_for_each_host() { + let store = CredentialStore::snapshot(vec![ + Arc::new(StaticProvider { + host_id: gh(), + token: Some("gh-tok"), + }), + Arc::new(StaticProvider { + host_id: ado(), + token: Some("ado-tok"), + }), + ]) + .await; + + assert_eq!(store.len(), 2); + assert_eq!(store.get(&gh()).unwrap().token().expose(), "gh-tok"); + assert_eq!(store.get(&ado()).unwrap().token().expose(), "ado-tok"); + } + + #[tokio::test] + async fn snapshot_falls_through_on_none_within_chain() { + let store = CredentialStore::snapshot(vec![ + Arc::new(StaticProvider { + host_id: gh(), + token: None, + }), + Arc::new(StaticProvider { + host_id: gh(), + token: Some("fallback"), + }), + ]) + .await; + assert_eq!(store.get(&gh()).unwrap().token().expose(), "fallback"); + } + + #[tokio::test] + async fn snapshot_first_provider_wins_when_both_have_token() { + let store = CredentialStore::snapshot(vec![ + Arc::new(StaticProvider { + host_id: gh(), + token: Some("primary"), + }), + Arc::new(StaticProvider { + host_id: gh(), + token: Some("backup"), + }), + ]) + .await; + assert_eq!(store.get(&gh()).unwrap().token().expose(), "primary"); + } + + #[tokio::test] + async fn store_is_immutable_after_snapshot_env_mutation() { + // SAFETY: this test single-threadedly writes/reads the env + // var; no other test in this binary touches it. + let var = "DEVDEV_TEST_CRED_SNAPSHOT_TOKEN"; + unsafe { std::env::set_var(var, "before") }; + + let store = CredentialStore::snapshot(vec![Arc::new(EnvVarProvider::new(gh(), var))]).await; + assert_eq!(store.get(&gh()).unwrap().token().expose(), "before"); + + // Mutate the env after snapshot; the store must not change. + unsafe { std::env::set_var(var, "after") }; + assert_eq!(store.get(&gh()).unwrap().token().expose(), "before"); + + // Cleanup. + unsafe { std::env::remove_var(var) }; + } + + // ── EnvVarProvider ───────────────────────────────────────── + + #[tokio::test] + async fn env_var_provider_returns_none_for_unset_var() { + let var = "DEVDEV_TEST_CRED_UNSET_VAR_ABC"; + unsafe { std::env::remove_var(var) }; + let p = EnvVarProvider::new(gh(), var); + assert!(p.fetch().await.unwrap().is_none()); + } + + #[tokio::test] + async fn env_var_provider_returns_none_for_empty_var() { + let var = "DEVDEV_TEST_CRED_EMPTY_VAR"; + unsafe { std::env::set_var(var, "") }; + let p = EnvVarProvider::new(gh(), var); + assert!(p.fetch().await.unwrap().is_none()); + unsafe { std::env::remove_var(var) }; + } + + #[tokio::test] + async fn env_var_provider_records_source() { + let var = "DEVDEV_TEST_CRED_RECORD_SRC"; + unsafe { std::env::set_var(var, "ok") }; + let p = EnvVarProvider::new(gh(), var); + let cred = p.fetch().await.unwrap().unwrap(); + match cred.source() { + TokenSource::EnvVar { name } => assert_eq!(name, var), + other => panic!("wrong source: {other:?}"), + } + unsafe { std::env::remove_var(var) }; + } + + // ── with_entries / with_entry ────────────────────────────── + + #[test] + fn with_entry_round_trips() { + let store = CredentialStore::with_entry(gh(), "tok"); + assert_eq!(store.get(&gh()).unwrap().token().expose(), "tok"); + assert_eq!(store.get(&gh()).unwrap().source(), &TokenSource::Injected); + assert!(store.get(&ado()).is_none()); + } + + #[test] + fn empty_store_returns_none_and_is_clonable() { + let store = CredentialStore::empty(); + assert!(store.is_empty()); + let clone = store.clone(); + assert!(clone.get(&gh()).is_none()); + } + + // ── Hint timing ──────────────────────────────────────────── + + #[test] + fn expires_at_hint_is_one_hour_after_sample() { + let cred = Credential::new(gh(), "x", TokenSource::Injected); + let sampled = cred.sampled_at_unix().unwrap(); + let exp = cred.expires_at_hint().unwrap(); + assert_eq!(exp - sampled, 3600); + } +} diff --git a/crates/devdev-daemon/src/dispatch.rs b/crates/devdev-daemon/src/dispatch.rs index 0d43b33..f029ed8 100644 --- a/crates/devdev-daemon/src/dispatch.rs +++ b/crates/devdev-daemon/src/dispatch.rs @@ -7,7 +7,8 @@ use std::time::Duration; use serde_json::{Value, json}; use tokio::sync::{Mutex, watch}; -use devdev_integrations::GitHubAdapter; +use devdev_integrations::RepoHostAdapter; +use devdev_integrations::host::RepoHostId; use devdev_tasks::approval::{ ApprovalGate, ApprovalHandle, ApprovalPolicy, ApprovalResponse, approval_channel, }; @@ -18,16 +19,25 @@ use devdev_tasks::registry::TaskRegistry; use devdev_tasks::repo_watch::RepoWatchTask; use devdev_workspace::Fs; +use crate::credentials::CredentialStore; +use crate::host_registry::RepoHostRegistry; use crate::ipc::{IpcRequest, IpcResponse}; use crate::router::{SessionHandle, SessionRouter}; use crate::runner::RouterRunner; -use crate::secrets::AgentSecrets; /// Shared state for the dispatch layer. pub struct DispatchContext { pub router: Arc, pub tasks: Arc>, - pub github: Arc, + /// Default repo-host adapter used when the registry has no + /// entry for a given host id (typically GitHub.com). Kept as a + /// distinct field so legacy single-host code paths keep working + /// while multi-host preferences are still being plumbed. + pub github: Arc, + /// Multi-host adapter registry. `for_host(&host_id)` returns + /// the adapter for a specific host; falls through to `github` + /// when the host is unregistered. + pub host_registry: Arc, /// Sender side of the approval channel, used by `devdev_ask` to /// request user approval before the agent takes external action. pub approval_gate: Arc>, @@ -38,9 +48,10 @@ pub struct DispatchContext { pub ledger: Arc, pub approval_policy: ApprovalPolicy, pub approval_timeout: Duration, - /// Host-derived secrets (e.g. `gh auth token`) handed out only on - /// approved `devdev_ask` calls. - pub agent_secrets: Arc>, + /// Frozen credential snapshot, sampled once at `devdev up`. + /// Tokens are surfaced to the agent only via approved + /// `devdev_ask` calls. + pub credentials: Arc, pub shutdown_tx: watch::Sender, /// Workspace filesystem, shared with the MCP provider so `fs/read` /// IPC calls observe the same bytes the agent wrote via MCP tools. @@ -48,10 +59,10 @@ pub struct DispatchContext { interactive: Mutex>, /// Log entries per task (task_id → messages). task_logs: Mutex>>, - /// Active `RepoWatchTask`s keyed by `(owner, repo)`. - repo_watch_ids: Mutex>, - /// Active `MonitorPrTask`s keyed by `(owner, repo, number)`. - monitor_pr_ids: Mutex>, + /// Active `RepoWatchTask`s keyed by `(host_id, owner, repo)`. + repo_watch_ids: Mutex>, + /// Active `MonitorPrTask`s keyed by `(host_id, owner, repo, number)`. + monitor_pr_ids: Mutex>, } impl DispatchContext { @@ -59,13 +70,14 @@ impl DispatchContext { pub fn new( router: Arc, tasks: Arc>, - github: Arc, + github: Arc, + host_registry: Arc, approval_gate: Arc>, approval_handle: Arc>, event_bus: EventBus, ledger: Arc, approval_policy: ApprovalPolicy, - agent_secrets: Arc>, + credentials: Arc, shutdown_tx: watch::Sender, fs: Arc>, ) -> Self { @@ -73,13 +85,14 @@ impl DispatchContext { router, tasks, github, + host_registry, approval_gate, approval_handle, event_bus, ledger, approval_policy, approval_timeout: Duration::from_secs(300), - agent_secrets, + credentials, shutdown_tx, fs, interactive: Mutex::new(None), @@ -89,6 +102,17 @@ impl DispatchContext { } } + /// Resolve the adapter for a host id, falling back to the + /// default `github` adapter when the registry has no entry. + /// Used by handlers to honour multi-host configuration without + /// breaking the legacy single-host smoke flow. + fn adapter_for(&self, host_id: &RepoHostId) -> Arc { + self.host_registry + .for_host(host_id) + .cloned() + .unwrap_or_else(|| Arc::clone(&self.github)) + } + /// Set a custom approval timeout (useful for tests). Rebuilds the /// underlying channel so the new timeout is honored. pub fn with_approval_timeout(mut self, timeout: Duration) -> Self { @@ -220,10 +244,21 @@ impl DispatchContext { let runner = Arc::new(RouterRunner::new(Arc::clone(&self.router), task_id.clone())) as Arc; + // Pre-parse so we can route the adapter lookup by host_id + // before constructing the task. Mismatch between the + // text-extracted host and the registered set is a hard + // error — better to fail fast than to silently shadow a + // GHE PR with the github.com adapter. + let parsed = match devdev_tasks::pr_ref::PrRef::parse(&pr_ref_str) { + Ok(p) => p, + Err(e) => return IpcResponse::err(req.id, -32602, format!("invalid PR ref: {e}")), + }; + let adapter = self.adapter_for(&parsed.host_id); + match MonitorPrTask::new( task_id.clone(), &pr_ref_str, - Arc::clone(&self.github), + adapter, runner, &self.event_bus, ) { @@ -232,7 +267,7 @@ impl DispatchContext { registry.add(Box::new(task)); drop(registry); self.monitor_pr_ids.lock().await.insert( - (pr.owner.clone(), pr.repo.clone(), pr.number), + (pr.host_id.clone(), pr.owner.clone(), pr.repo.clone(), pr.number), task_id.clone(), ); IpcResponse::ok( @@ -246,10 +281,12 @@ impl DispatchContext { } } - /// "repo/watch" — start a `RepoWatchTask` for `(owner, repo)`. + /// "repo/watch" — start a `RepoWatchTask` for `(host, owner, repo)`. /// - /// Idempotent: subsequent calls for the same repo return the + /// Idempotent: subsequent calls for the same triple return the /// existing task id without spawning a duplicate watcher. + /// `params.host` is optional and defaults to `github.com`; when + /// supplied it must be classifiable by `RepoHostId::from_browse_host`. async fn handle_repo_watch(&self, req: IpcRequest) -> IpcResponse { let owner = match req.params["owner"].as_str() { Some(s) => s.to_string(), @@ -259,13 +296,26 @@ impl DispatchContext { Some(s) => s.to_string(), None => return IpcResponse::err(req.id, -32602, "missing params.repo"), }; + let host_id = match req.params.get("host").and_then(|v| v.as_str()) { + Some(h) => match RepoHostId::from_browse_host(h) { + Some(id) => id, + None => { + return IpcResponse::err( + req.id, + -32602, + format!("params.host {h:?} is not a recognised repo host"), + ); + } + }, + None => RepoHostId::github_com(), + }; let interval_secs = req .params .get("poll_interval_secs") .and_then(|v| v.as_u64()) .unwrap_or(60); - let key = (owner.clone(), repo.clone()); + let key = (host_id.clone(), owner.clone(), repo.clone()); { let watches = self.repo_watch_ids.lock().await; if let Some(id) = watches.get(&key) { @@ -277,9 +327,10 @@ impl DispatchContext { let task_id = registry.next_id(); let task = RepoWatchTask::new( task_id.clone(), + host_id.clone(), owner.clone(), repo.clone(), - Arc::clone(&self.github), + self.adapter_for(&host_id), Arc::clone(&self.ledger), self.event_bus.clone(), ) @@ -308,16 +359,29 @@ impl DispatchContext { Some(s) => s.to_string(), None => return IpcResponse::err(req.id, -32602, "missing params.repo"), }; + let host_id = match req.params.get("host").and_then(|v| v.as_str()) { + Some(h) => match RepoHostId::from_browse_host(h) { + Some(id) => id, + None => { + return IpcResponse::err( + req.id, + -32602, + format!("params.host {h:?} is not a recognised repo host"), + ); + } + }, + None => RepoHostId::github_com(), + }; let task_id = { let mut watches = self.repo_watch_ids.lock().await; - match watches.remove(&(owner.clone(), repo.clone())) { + match watches.remove(&(host_id.clone(), owner.clone(), repo.clone())) { Some(id) => id, None => { return IpcResponse::err( req.id, -32602, - format!("not watching {owner}/{repo}"), + format!("not watching {}:{owner}/{repo}", host_id.ledger_key()), ); } } @@ -330,18 +394,19 @@ impl DispatchContext { } } - /// Ensure a `MonitorPrTask` exists for `(owner, repo, number)`. + /// Ensure a `MonitorPrTask` exists for `(host_id, owner, repo, number)`. /// Used by the event coordinator on first observation of a PR. /// Returns `(task_id, newly_created)`. When `newly_created` is /// true the caller should replay the triggering event onto the /// bus so the freshly-subscribed task observes it. pub async fn ensure_monitor_pr_task( &self, + host_id: &RepoHostId, owner: &str, repo: &str, number: u64, ) -> Result<(String, bool), String> { - let key = (owner.to_string(), repo.to_string(), number); + let key = (host_id.clone(), owner.to_string(), repo.to_string(), number); { let map = self.monitor_pr_ids.lock().await; if let Some(id) = map.get(&key) { @@ -349,6 +414,12 @@ impl DispatchContext { } } + // Build a PrRef directly so we honour the host_id without + // round-tripping through a string parser. MonitorPrTask + // currently re-parses a string — keep it stable for now and + // pass the shorthand form (event coordinator only fires for + // GitHub today; ADO/GHE event sources will pass full URLs + // when Phase 5 wires the registry through this path). let pr_ref_str = format!("{owner}/{repo}#{number}"); let mut registry = self.tasks.lock().await; let task_id = registry.next_id(); @@ -358,7 +429,7 @@ impl DispatchContext { let task = MonitorPrTask::new( task_id.clone(), &pr_ref_str, - Arc::clone(&self.github), + self.adapter_for(host_id), runner, &self.event_bus, ) @@ -493,11 +564,12 @@ pub fn spawn_event_coordinator( Ok(e) => e, Err(_) => break, }; - if let Some((owner, repo, number)) = ev.pr_target() { + if let Some((host_id, owner, repo, number)) = ev.pr_target() { + let host_id = host_id.clone(); let owner = owner.to_string(); let repo = repo.to_string(); match ctx - .ensure_monitor_pr_task(&owner, &repo, number) + .ensure_monitor_pr_task(&host_id, &owner, &repo, number) .await { Ok((_, true)) => { @@ -508,7 +580,8 @@ pub fn spawn_event_coordinator( Ok((_, false)) => {} Err(e) => { tracing::warn!( - "event coordinator: ensure_monitor_pr_task failed for {owner}/{repo}#{number}: {e}" + "event coordinator: ensure_monitor_pr_task failed for {}:{owner}/{repo}#{number}: {e}", + host_id.ledger_key() ); } } diff --git a/crates/devdev-daemon/src/host_registry.rs b/crates/devdev-daemon/src/host_registry.rs new file mode 100644 index 0000000..cd997d6 --- /dev/null +++ b/crates/devdev-daemon/src/host_registry.rs @@ -0,0 +1,308 @@ +//! Routes `RepoHostAdapter` lookups by `RepoHostId`. +//! +//! The registry is the single source of truth for "given this PR +//! URL or host id, which adapter speaks its API?". It owns no +//! credential material; tokens flow through [`crate::credentials`] +//! and are correlated by the same [`RepoHostId`] keys. +//! +//! ## Identity model +//! +//! Adapters are keyed by a fully-resolved [`RepoHostId`] +//! (`{kind, api_base, host}`). Two registry entries with the same +//! `host` but different `kind`/`api_base` are not currently +//! supported (and would indicate a mis-configuration upstream). +//! +//! ## URL routing +//! +//! [`RepoHostRegistry::for_url`] strips the scheme + path and asks +//! [`RepoHostId::from_browse_host`] to classify the bare host. If +//! classification succeeds AND the registry has an entry for the +//! resulting host id, the adapter is returned. Unknown hosts and +//! unregistered-but-known hosts both yield `None` so callers can +//! tell "we don't know that host" apart from "we recognise it but +//! aren't watching it" by also consulting `RepoHostId::from_browse_host`. + +use std::collections::HashMap; +use std::sync::Arc; + +use devdev_integrations::RepoHostAdapter; +use devdev_integrations::host::RepoHostId; + +/// Read-only registry of [`RepoHostAdapter`]s keyed by [`RepoHostId`]. +/// +/// Built once at `devdev up` from preferences; immutable afterward +/// to mirror the [`crate::credentials::CredentialStore`] lifecycle +/// model. Mutation would invite the same race window we eliminated +/// in Phase 4 (a fetch operation observing a partially-installed +/// registry), so we don't expose any. +#[derive(Clone)] +pub struct RepoHostRegistry { + adapters: Arc>>, +} + +impl RepoHostRegistry { + /// Build a registry from a fully-prepared map. Callers use + /// [`RepoHostRegistryBuilder`] for incremental construction. + pub fn from_map(adapters: HashMap>) -> Self { + Self { + adapters: Arc::new(adapters), + } + } + + /// Empty registry — only useful as a placeholder in tests that + /// never hit the adapter lookup path. + pub fn empty() -> Self { + Self::from_map(HashMap::new()) + } + + /// Convenience constructor for the common single-host case. + pub fn single(host_id: RepoHostId, adapter: Arc) -> Self { + let mut m = HashMap::new(); + m.insert(host_id, adapter); + Self::from_map(m) + } + + /// Number of registered adapters. + pub fn len(&self) -> usize { + self.adapters.len() + } + + /// True when no adapters are registered. + pub fn is_empty(&self) -> bool { + self.adapters.is_empty() + } + + /// Look up an adapter by its host id. + pub fn for_host(&self, host_id: &RepoHostId) -> Option<&Arc> { + self.adapters.get(host_id) + } + + /// Look up an adapter for a browser-shaped URL or bare host + /// string. Returns `None` if the host can't be classified or + /// isn't registered. + pub fn for_url(&self, url_or_host: &str) -> Option<&Arc> { + let host = extract_host(url_or_host)?; + let host_id = RepoHostId::from_browse_host(host)?; + self.for_host(&host_id) + } + + /// Iterate over `(host_id, adapter)` pairs in arbitrary order. + pub fn iter(&self) -> impl Iterator)> { + self.adapters.iter() + } +} + +/// Strip scheme + path, return the bare host. Accepts both full +/// URLs (`https://github.com/o/r/pull/1`) and bare hosts +/// (`github.com`). Empty input returns `None`. +fn extract_host(input: &str) -> Option<&str> { + let s = input.trim(); + if s.is_empty() { + return None; + } + let after_scheme = s + .strip_prefix("https://") + .or_else(|| s.strip_prefix("http://")) + .unwrap_or(s); + let host = after_scheme.split('/').next()?; + if host.is_empty() { None } else { Some(host) } +} + +/// Incremental builder for [`RepoHostRegistry`]. Insertions overwrite +/// silently; if you need conflict detection in the future, layer it +/// on the calling side. +#[derive(Default)] +pub struct RepoHostRegistryBuilder { + adapters: HashMap>, +} + +impl RepoHostRegistryBuilder { + pub fn new() -> Self { + Self::default() + } + + pub fn with(mut self, host_id: RepoHostId, adapter: Arc) -> Self { + self.adapters.insert(host_id, adapter); + self + } + + pub fn insert(&mut self, host_id: RepoHostId, adapter: Arc) { + self.adapters.insert(host_id, adapter); + } + + pub fn build(self) -> RepoHostRegistry { + RepoHostRegistry::from_map(self.adapters) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use async_trait::async_trait; + use devdev_integrations::{Comment, PrStatus, PullRequest, RepoHostError, Review}; + + /// Adapter stub that records its identity for lookup assertions. + /// Only `host_id` is meaningful; everything else returns NotFound + /// so any accidental call panics loudly in tests. + struct StubAdapter { + host_id: RepoHostId, + } + + #[async_trait] + impl RepoHostAdapter for StubAdapter { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + async fn get_pr(&self, _o: &str, _r: &str, _n: u64) -> Result { + Err(RepoHostError::NotFound("stub".into())) + } + async fn get_pr_diff(&self, _o: &str, _r: &str, _n: u64) -> Result { + Err(RepoHostError::NotFound("stub".into())) + } + async fn list_pr_comments( + &self, + _o: &str, + _r: &str, + _n: u64, + ) -> Result, RepoHostError> { + Ok(vec![]) + } + async fn post_review( + &self, + _o: &str, + _r: &str, + _n: u64, + _review: Review, + ) -> Result<(), RepoHostError> { + Ok(()) + } + async fn post_comment( + &self, + _o: &str, + _r: &str, + _n: u64, + _body: &str, + ) -> Result<(), RepoHostError> { + Ok(()) + } + async fn get_pr_status( + &self, + _o: &str, + _r: &str, + _n: u64, + ) -> Result { + Err(RepoHostError::NotFound("stub".into())) + } + async fn get_pr_head_sha( + &self, + _o: &str, + _r: &str, + _n: u64, + ) -> Result { + Err(RepoHostError::NotFound("stub".into())) + } + async fn list_open_prs( + &self, + _o: &str, + _r: &str, + ) -> Result, RepoHostError> { + Ok(vec![]) + } + } + + fn stub(host_id: RepoHostId) -> Arc { + Arc::new(StubAdapter { host_id }) + } + + #[test] + fn empty_registry_returns_none() { + let r = RepoHostRegistry::empty(); + assert!(r.is_empty()); + assert_eq!(r.len(), 0); + assert!(r.for_host(&RepoHostId::github_com()).is_none()); + assert!(r.for_url("https://github.com/o/r").is_none()); + } + + #[test] + fn single_round_trips() { + let id = RepoHostId::github_com(); + let r = RepoHostRegistry::single(id.clone(), stub(id.clone())); + assert_eq!(r.len(), 1); + let got = r.for_host(&id).expect("present"); + assert_eq!(got.host_id(), &id); + } + + #[test] + fn for_url_routes_to_correct_adapter() { + let gh = RepoHostId::github_com(); + let ghe = RepoHostId::ghe("ghe.acme.io"); + let ado = RepoHostId::azure_devops(); + let r = RepoHostRegistryBuilder::new() + .with(gh.clone(), stub(gh.clone())) + .with(ghe.clone(), stub(ghe.clone())) + .with(ado.clone(), stub(ado.clone())) + .build(); + + assert_eq!( + r.for_url("https://github.com/o/r/pull/1").unwrap().host_id(), + &gh + ); + assert_eq!( + r.for_url("https://ghe.acme.io/o/r/pull/1").unwrap().host_id(), + &ghe + ); + assert_eq!( + r.for_url("https://dev.azure.com/org/proj/_git/repo/pullrequest/1") + .unwrap() + .host_id(), + &ado + ); + } + + #[test] + fn for_url_accepts_bare_host() { + let id = RepoHostId::github_com(); + let r = RepoHostRegistry::single(id.clone(), stub(id)); + assert!(r.for_url("github.com").is_some()); + assert!(r.for_url("www.github.com").is_some()); + } + + #[test] + fn for_url_returns_none_for_unknown_host() { + let id = RepoHostId::github_com(); + let r = RepoHostRegistry::single(id.clone(), stub(id)); + assert!(r.for_url("https://gitlab.com/o/r").is_none()); + assert!(r.for_url("not a url").is_none()); + assert!(r.for_url("").is_none()); + } + + #[test] + fn for_url_returns_none_for_unregistered_known_host() { + // Host classifies but we never registered it. + let id = RepoHostId::github_com(); + let r = RepoHostRegistry::single(id.clone(), stub(id)); + assert!(r.for_url("https://ghe.example.com/o/r").is_none()); + } + + #[test] + fn builder_overwrite_keeps_last() { + let id = RepoHostId::github_com(); + let other = RepoHostId::ghe("ghe.example.com"); + let r = RepoHostRegistryBuilder::new() + .with(id.clone(), stub(other.clone())) + .with(id.clone(), stub(id.clone())) + .build(); + assert_eq!(r.for_host(&id).unwrap().host_id(), &id); + } + + #[test] + fn extract_host_strips_scheme_and_path() { + assert_eq!(extract_host("https://github.com/o/r"), Some("github.com")); + assert_eq!(extract_host("http://github.com"), Some("github.com")); + assert_eq!(extract_host("github.com/o/r"), Some("github.com")); + assert_eq!(extract_host("github.com"), Some("github.com")); + assert_eq!(extract_host(" https://x.io/y "), Some("x.io")); + assert_eq!(extract_host(""), None); + assert_eq!(extract_host("/leading-slash"), None); + } +} diff --git a/crates/devdev-daemon/src/lib.rs b/crates/devdev-daemon/src/lib.rs index 12c802c..798cf22 100644 --- a/crates/devdev-daemon/src/lib.rs +++ b/crates/devdev-daemon/src/lib.rs @@ -5,14 +5,15 @@ //! to it over IPC. pub mod checkpoint; +pub mod credentials; pub mod dispatch; +pub mod host_registry; pub mod ipc; pub mod ledger; pub mod mcp; pub mod pid; pub mod router; pub mod runner; -pub mod secrets; pub mod server; use std::path::PathBuf; diff --git a/crates/devdev-daemon/src/mcp/provider.rs b/crates/devdev-daemon/src/mcp/provider.rs index 76b7205..14949d0 100644 --- a/crates/devdev-daemon/src/mcp/provider.rs +++ b/crates/devdev-daemon/src/mcp/provider.rs @@ -8,13 +8,14 @@ use std::sync::Arc; use async_trait::async_trait; +use devdev_integrations::host::RepoHostId; use devdev_tasks::approval::{ApprovalError, ApprovalGate}; use devdev_tasks::registry::TaskRegistry; use devdev_workspace::Fs; use tokio::sync::Mutex; +use crate::credentials::CredentialStore; use crate::mcp::{AskKind, AskRequest, AskResponse, McpProviderError, McpToolProvider, TaskInfo}; -use crate::secrets::AgentSecrets; /// Wraps the daemon's shared `Arc>` and /// `Arc>` so the MCP server can both surface task state and @@ -28,7 +29,7 @@ pub struct DaemonToolProvider { tasks: Arc>, fs: Arc>, approval_gate: Option>>, - agent_secrets: Option>>, + credentials: Option>, } impl DaemonToolProvider { @@ -37,20 +38,20 @@ impl DaemonToolProvider { tasks, fs, approval_gate: None, - agent_secrets: None, + credentials: None, } } - /// Wire the approval gate + secrets slot so `devdev_ask` is live. - /// Constructed without these, the tool returns a `not configured` - /// error — keeping pre-Phase-C tests unaffected. + /// Wire the approval gate + credential snapshot so `devdev_ask` + /// is live. Constructed without these, the tool returns a + /// `not configured` error — keeping pre-Phase-C tests unaffected. pub fn with_ask( mut self, gate: Arc>, - secrets: Arc>, + credentials: Arc, ) -> Self { self.approval_gate = Some(gate); - self.agent_secrets = Some(secrets); + self.credentials = Some(credentials); self } } @@ -97,10 +98,10 @@ impl McpToolProvider for DaemonToolProvider { .approval_gate .as_ref() .ok_or_else(|| McpProviderError::Other("ask: approval gate not configured".into()))?; - let secrets = self - .agent_secrets + let credentials = self + .credentials .as_ref() - .ok_or_else(|| McpProviderError::Other("ask: secrets slot not configured".into()))?; + .ok_or_else(|| McpProviderError::Other("ask: credential store not configured".into()))?; let action = match req.kind { AskKind::PostReview => "post_review", @@ -125,8 +126,29 @@ impl McpToolProvider for DaemonToolProvider { AskKind::PostReview | AskKind::PostComment | AskKind::RequestToken ); let (token, expires_at) = if needs_token { - let s = secrets.lock().await; - (s.gh_token.clone(), s.token_expires_at_hint()) + // Resolve the target host: explicit `host` field + // wins, otherwise default to github.com so old + // clients keep working. Unknown hosts surface as + // a hard rejection \u2014 silently swapping in the + // wrong token would be a security footgun. + let host_id = match req.host.as_deref() { + Some(h) => match RepoHostId::from_browse_host(h) { + Some(id) => id, + None => { + return Ok(AskResponse::Rejected { + reason: format!("unknown ask host: {h}"), + }); + } + }, + None => RepoHostId::github_com(), + }; + match credentials.get(&host_id) { + Some(c) => ( + Some(c.token().expose().to_string()), + c.expires_at_hint(), + ), + None => (None, None), + } } else { (None, None) }; @@ -246,20 +268,21 @@ mod tests { ) -> ( DaemonToolProvider, Arc>, - Arc>, + Arc, ) { let (gate, handle) = approval_channel(policy, timeout); let gate = Arc::new(Mutex::new(gate)); let handle = Arc::new(Mutex::new(handle)); - let mut secrets = AgentSecrets::default(); - secrets.set_gh_token(token.map(str::to_string)); - let secrets = Arc::new(Mutex::new(secrets)); + let store = Arc::new(match token { + Some(t) => CredentialStore::with_entry(RepoHostId::github_com(), t), + None => CredentialStore::empty(), + }); let provider = DaemonToolProvider::new( Arc::new(Mutex::new(TaskRegistry::new())), Arc::new(Mutex::new(devdev_workspace::Fs::new())), ) - .with_ask(gate, Arc::clone(&secrets)); - (provider, handle, secrets) + .with_ask(gate, Arc::clone(&store)); + (provider, handle, store) } #[tokio::test] @@ -274,6 +297,7 @@ mod tests { kind: AskKind::PostReview, summary: "post review on PR #42".into(), payload: serde_json::json!({ "comment": "looks good" }), + host: None, }) .await .expect("ask succeeds"); @@ -303,6 +327,7 @@ mod tests { kind: AskKind::Question, summary: "what color?".into(), payload: serde_json::json!({}), + host: None, }) .await .unwrap(); @@ -325,6 +350,7 @@ mod tests { kind: AskKind::PostReview, summary: "x".into(), payload: serde_json::json!({}), + host: None, }; let provider_clone = provider.clone(); let ask_task = tokio::spawn(async move { provider_clone.ask(req).await }); @@ -356,6 +382,7 @@ mod tests { kind: AskKind::Question, summary: "stalls".into(), payload: serde_json::json!({}), + host: None, }) .await .unwrap(); @@ -371,6 +398,7 @@ mod tests { kind: AskKind::PostComment, summary: "drop".into(), payload: serde_json::json!({"comment": "x"}), + host: None, }) .await .unwrap(); @@ -394,9 +422,75 @@ mod tests { kind: AskKind::Question, summary: "nope".into(), payload: serde_json::json!({}), + host: None, }) .await .expect_err("must error"); assert!(format!("{err}").contains("not configured")); } + + #[tokio::test] + async fn ask_routes_token_by_host_selector() { + // Two host entries in the credential store; the ask's + // `host` field picks which token comes back. + let (gate, _handle) = + approval_channel(ApprovalPolicy::AutoApprove, Duration::from_secs(1)); + let store = Arc::new(CredentialStore::with_entries([ + crate::credentials::Credential::new( + RepoHostId::github_com(), + "ghp_main", + crate::credentials::TokenSource::Injected, + ), + crate::credentials::Credential::new( + RepoHostId::ghe("ghe.acme.io"), + "ghe_secret", + crate::credentials::TokenSource::Injected, + ), + ])); + let provider = DaemonToolProvider::new( + Arc::new(Mutex::new(TaskRegistry::new())), + Arc::new(Mutex::new(devdev_workspace::Fs::new())), + ) + .with_ask(Arc::new(Mutex::new(gate)), store); + + let resp = provider + .ask(AskRequest { + kind: AskKind::PostReview, + summary: "ghe review".into(), + payload: serde_json::json!({}), + host: Some("ghe.acme.io".into()), + }) + .await + .unwrap(); + match resp { + AskResponse::Approved { token, .. } => { + assert_eq!(token.as_deref(), Some("ghe_secret")); + } + other => panic!("expected approved, got {other:?}"), + } + } + + #[tokio::test] + async fn ask_unknown_host_is_rejected() { + let (provider, _h, _s) = build_provider_with_ask( + ApprovalPolicy::AutoApprove, + Duration::from_secs(1), + Some("ghp_main"), + ); + let resp = provider + .ask(AskRequest { + kind: AskKind::PostReview, + summary: "bogus".into(), + payload: serde_json::json!({}), + host: Some("gitlab.example.com".into()), + }) + .await + .unwrap(); + match resp { + AskResponse::Rejected { reason } => { + assert!(reason.contains("unknown ask host"), "reason was {reason}"); + } + other => panic!("expected rejected, got {other:?}"), + } + } } diff --git a/crates/devdev-daemon/src/mcp/tools.rs b/crates/devdev-daemon/src/mcp/tools.rs index ca3136f..c6981ac 100644 --- a/crates/devdev-daemon/src/mcp/tools.rs +++ b/crates/devdev-daemon/src/mcp/tools.rs @@ -74,6 +74,13 @@ pub struct AskRequest { /// for `post_review`). Echoed back in the response. #[serde(default)] pub payload: serde_json::Value, + /// Repo host the ask targets (e.g. `"github.com"`, + /// `"ghe.acme.io"`, `"dev.azure.com"`). Optional for back-compat + /// with single-host clients; when absent the provider defaults + /// to `github.com`. The provider uses this to pick which + /// credential entry to surface in the response. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub host: Option, } /// Outcome the agent receives. diff --git a/crates/devdev-daemon/src/secrets.rs b/crates/devdev-daemon/src/secrets.rs deleted file mode 100644 index 63ac4c2..0000000 --- a/crates/devdev-daemon/src/secrets.rs +++ /dev/null @@ -1,98 +0,0 @@ -//! Host-derived secrets handed to the agent only via approved -//! `devdev_ask` calls. -//! -//! The slot is populated at `devdev up` time (best-effort `gh auth -//! token`) and may be cleared / rotated at runtime. It is *never* -//! injected into the agent's process environment — the agent must -//! call `devdev_ask` and receive the token in the tool response. - -use std::process::Stdio; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; - -use tokio::process::Command; - -/// Slot of secrets that may be surfaced through `devdev_ask`. -#[derive(Debug, Default, Clone)] -pub struct AgentSecrets { - /// Result of `gh auth token`. `None` if `gh` is missing or unauth. - pub gh_token: Option, - /// Wall-clock seconds since epoch when `gh_token` was sampled. - pub gh_token_sampled_at: Option, -} - -impl AgentSecrets { - /// Set the GitHub token and stamp the sample time. Used both by - /// the boot path and by tests (which can inject deterministically). - pub fn set_gh_token(&mut self, token: Option) { - self.gh_token = token; - self.gh_token_sampled_at = SystemTime::now() - .duration_since(UNIX_EPOCH) - .ok() - .map(|d| d.as_secs()); - } - - /// `expires_at` hint for token consumers. We do not enforce - /// revocation; this is informational only. - pub fn token_expires_at_hint(&self) -> Option { - // Hand out a one-hour validity window. GitHub user tokens live - // far longer, but bounded hints train downstream consumers - // not to cache. - const ONE_HOUR_SECS: u64 = 3600; - self.gh_token_sampled_at.map(|t| t + ONE_HOUR_SECS) - } -} - -/// Best-effort `gh auth token` invocation. Returns `Ok(None)` when -/// `gh` is not on PATH or returns a non-zero status; the caller treats -/// that as "no token available" rather than a hard failure. -pub async fn try_read_gh_token() -> std::io::Result> { - let mut cmd = Command::new("gh"); - cmd.arg("auth").arg("token"); - cmd.stdout(Stdio::piped()).stderr(Stdio::null()); - let output = match tokio::time::timeout(Duration::from_secs(5), cmd.output()).await { - Ok(r) => r?, - Err(_) => return Ok(None), - }; - if !output.status.success() { - return Ok(None); - } - let token = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if token.is_empty() { - Ok(None) - } else { - Ok(Some(token)) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn set_token_stamps_time() { - let mut s = AgentSecrets::default(); - assert!(s.gh_token.is_none()); - assert!(s.token_expires_at_hint().is_none()); - s.set_gh_token(Some("ghp_test".to_string())); - assert_eq!(s.gh_token.as_deref(), Some("ghp_test")); - let exp = s.token_expires_at_hint().expect("hint set"); - let now = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_secs(); - // Expires roughly an hour from now (bounded slack for slow tests). - assert!(exp >= now + 3500 && exp <= now + 3700); - } - - #[test] - fn clear_removes_token_and_keeps_hint_none() { - let mut s = AgentSecrets::default(); - s.set_gh_token(Some("x".into())); - s.set_gh_token(None); - assert!(s.gh_token.is_none()); - // Hint stays Some because we stamped time on the second call; - // hint is a function of "when did we last sample", not whether - // we got a value. - assert!(s.token_expires_at_hint().is_some()); - } -} diff --git a/crates/devdev-daemon/tests/e2e_pr_shepherding.rs b/crates/devdev-daemon/tests/e2e_pr_shepherding.rs index 7d9991a..1875f1c 100644 --- a/crates/devdev-daemon/tests/e2e_pr_shepherding.rs +++ b/crates/devdev-daemon/tests/e2e_pr_shepherding.rs @@ -13,7 +13,7 @@ use devdev_daemon::router::{ AgentResponse, ResponseChunk, RouterError, SessionBackend, SessionRouter, }; use devdev_daemon::{Daemon, DaemonConfig}; -use devdev_integrations::{MockGitHubAdapter, PrState, PrStatus, PullRequest}; +use devdev_integrations::{MockAdapter, PrState, PrStatus, PullRequest}; use devdev_tasks::approval::{self, ApprovalPolicy}; use devdev_tasks::events::{DaemonEvent, EventBus}; use devdev_tasks::ledger::IdempotencyLedger; @@ -97,8 +97,8 @@ fn test_pr(sha: &str) -> PullRequest { } } -fn test_github(sha: &str) -> MockGitHubAdapter { - MockGitHubAdapter::new() +fn test_github(sha: &str) -> MockAdapter { + MockAdapter::new() .with_pr("test-org", "test-repo", test_pr(sha)) .with_diff( "test-org", @@ -143,8 +143,12 @@ impl E2EHarness { let daemon = Daemon::start(config, false).await.unwrap(); let gh = Arc::new(test_github("sha-initial-001")); - let github: Arc = - Arc::clone(&gh) as Arc; + let github: Arc = + Arc::clone(&gh) as Arc; + let host_registry = Arc::new(devdev_daemon::host_registry::RepoHostRegistry::single( + devdev_integrations::host::RepoHostId::github_com(), + Arc::clone(&github), + )); let backend = Arc::new(FakeAgentBackend::new()); let backend_dyn: Arc = backend.clone(); @@ -156,7 +160,7 @@ impl E2EHarness { let (gate, handle) = approval::approval_channel(policy, Duration::from_secs(30)); let approval_gate = Arc::new(Mutex::new(gate)); let approval_handle = Arc::new(Mutex::new(handle)); - let agent_secrets = Arc::new(Mutex::new(devdev_daemon::secrets::AgentSecrets::default())); + let credentials = Arc::new(devdev_daemon::credentials::CredentialStore::empty()); let bus = EventBus::new(); let ledger: Arc = @@ -169,12 +173,13 @@ impl E2EHarness { Arc::clone(&router), Arc::clone(®istry), github, + host_registry, approval_gate, approval_handle, bus.clone(), ledger, policy, - agent_secrets, + credentials, shutdown_tx.clone(), Arc::new(Mutex::new(devdev_workspace::Fs::new())), ) @@ -255,6 +260,7 @@ async fn pr_opened_event_drives_agent_prompt() { .await; harness.bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "test-org".into(), repo: "test-repo".into(), number: 1, @@ -288,6 +294,7 @@ async fn pr_closed_event_completes_task() { .await; harness.bus.publish(DaemonEvent::PrClosed { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "test-org".into(), repo: "test-repo".into(), number: 1, @@ -318,6 +325,7 @@ async fn pr_updated_event_reprompts_agent() { .await; harness.bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "test-org".into(), repo: "test-repo".into(), number: 1, @@ -326,6 +334,7 @@ async fn pr_updated_event_reprompts_agent() { harness.advance_polls(1).await; harness.bus.publish(DaemonEvent::PrUpdated { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "test-org".into(), repo: "test-repo".into(), number: 1, @@ -353,6 +362,7 @@ async fn unrelated_pr_event_is_ignored() { // Different number. harness.bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "test-org".into(), repo: "test-repo".into(), number: 2, diff --git a/crates/devdev-integrations/Cargo.toml b/crates/devdev-integrations/Cargo.toml index 72956a3..3fecc97 100644 --- a/crates/devdev-integrations/Cargo.toml +++ b/crates/devdev-integrations/Cargo.toml @@ -10,6 +10,7 @@ publish.workspace = true [dependencies] async-trait = { workspace = true } +base64 = { workspace = true } reqwest = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/crates/devdev-integrations/src/azure_devops.rs b/crates/devdev-integrations/src/azure_devops.rs new file mode 100644 index 0000000..2275a11 --- /dev/null +++ b/crates/devdev-integrations/src/azure_devops.rs @@ -0,0 +1,537 @@ +//! Azure DevOps Services adapter (REST API 7.0). +//! +//! # URL layout +//! +//! ADO scopes pull requests by `organization / project / repository` +//! rather than GitHub's `owner / repo` pair. To fit the +//! [`crate::RepoHostAdapter`] surface without a breaking signature +//! change, this adapter encodes the triple as: +//! +//! * `owner = "/"` (slash-joined) +//! * `repo = ""` +//! +//! For example `https://dev.azure.com/contoso/Acme/_git/widgets` is +//! addressed as `owner = "contoso/Acme"`, `repo = "widgets"`. +//! +//! # Authentication +//! +//! ADO uses HTTP Basic auth with an empty username and the PAT as +//! the password (`Authorization: Basic base64(":")`). PATs are +//! organization-scoped; obtain one from +//! `https://dev.azure.com//_usersSettings/tokens`. +//! +//! # Type mapping (lossy points) +//! +//! | DevDev type | ADO source | Notes | +//! |-----------------------|--------------------------------------|-------| +//! | `PrState::Open` | `status = "active"` | | +//! | `PrState::Closed` | `status = "abandoned"` | | +//! | `PrState::Merged` | `status = "completed"` | | +//! | `ReviewEvent::Approve`| vote `10` (or `5` *approved with suggestions*) | `5` flattened to approve | +//! | `ReviewEvent::RequestChanges` | vote `-10` or `-5` | `-5` *waiting for author* flattened | +//! | `ReviewEvent::Comment`| vote `0` | | +//! | `CheckRun.status` | PR Status `state` | `pending`/`succeeded`/`failed`/`error`/`notApplicable` mapped to GH-shaped `queued`/`in_progress`/`completed` | +//! | `CheckRun.conclusion` | derived from PR Status `state` | `succeeded`→`success`, `failed`→`failure`, `error`→`failure`, `notApplicable`→`neutral` | +//! +//! # Status note +//! +//! This is the initial cut. Pagination uses ADO's `continuationToken` +//! header for `list_pr_comments` / `list_open_prs`; not all error +//! shapes have been exercised against live tenants. Live testing is +//! gated on `DEVDEV_E2E_ADO=1` + `ADO_TOKEN` + `ADO_ORG_URL`. + +use async_trait::async_trait; +use base64::Engine as _; +use base64::engine::general_purpose::STANDARD as B64; +use reqwest::header::{ACCEPT, AUTHORIZATION, USER_AGENT}; + +use crate::RepoHostAdapter; +use crate::host::RepoHostId; +use crate::types::*; + +const API_VERSION: &str = "7.0"; + +pub struct AzureDevOpsAdapter { + host_id: RepoHostId, + client: reqwest::Client, + auth_header: String, +} + +impl AzureDevOpsAdapter { + /// Build an ADO Services adapter using `dev.azure.com` and the + /// supplied PAT. + pub fn new(pat: String) -> Self { + Self::with_host(RepoHostId::azure_devops(), pat) + } + + /// Build an adapter against a specific host id (e.g. a legacy + /// `*.visualstudio.com` instance). + pub fn with_host(host_id: RepoHostId, pat: String) -> Self { + let auth = B64.encode(format!(":{pat}")); + Self { + host_id, + client: reqwest::Client::new(), + auth_header: format!("Basic {auth}"), + } + } + + fn split_owner(owner: &str) -> Result<(&str, &str), RepoHostError> { + owner.split_once('/').ok_or_else(|| { + RepoHostError::Unsupported(format!( + "ADO requires owner=\"/\"; got {owner:?}" + )) + }) + } + + fn pr_base(&self, owner: &str, repo: &str, number: u64) -> Result { + let (org, project) = Self::split_owner(owner)?; + Ok(format!( + "{}/{org}/{project}/_apis/git/repositories/{repo}/pullrequests/{number}", + self.host_id.api_base + )) + } + + fn list_base(&self, owner: &str, repo: &str) -> Result { + let (org, project) = Self::split_owner(owner)?; + Ok(format!( + "{}/{org}/{project}/_apis/git/repositories/{repo}/pullrequests", + self.host_id.api_base + )) + } + + async fn get_json( + &self, + url: &str, + ) -> Result { + let resp = self + .client + .get(url) + .header(AUTHORIZATION, &self.auth_header) + .header(ACCEPT, "application/json") + .header(USER_AGENT, "devdev/0.1") + .send() + .await?; + check_status(&resp)?; + let text = resp.text().await?; + serde_json::from_str(&text) + .map_err(|e| RepoHostError::Deserialize(format!("{e}: {text}"))) + } + + async fn post_json( + &self, + url: &str, + body: &serde_json::Value, + ) -> Result { + let resp = self + .client + .post(url) + .header(AUTHORIZATION, &self.auth_header) + .header(ACCEPT, "application/json") + .header(USER_AGENT, "devdev/0.1") + .json(body) + .send() + .await?; + check_status(&resp)?; + let text = resp.text().await?; + if text.trim().is_empty() { + return Ok(serde_json::Value::Null); + } + serde_json::from_str(&text) + .map_err(|e| RepoHostError::Deserialize(format!("{e}: {text}"))) + } + + async fn patch_json( + &self, + url: &str, + body: &serde_json::Value, + ) -> Result<(), RepoHostError> { + let resp = self + .client + .patch(url) + .header(AUTHORIZATION, &self.auth_header) + .header(ACCEPT, "application/json") + .header(USER_AGENT, "devdev/0.1") + .json(body) + .send() + .await?; + check_status(&resp)?; + Ok(()) + } +} + +fn check_status(resp: &reqwest::Response) -> Result<(), RepoHostError> { + let status = resp.status().as_u16(); + match status { + 200..=299 => Ok(()), + 401 | 403 => Err(RepoHostError::Unauthorized), + 404 => Err(RepoHostError::NotFound(resp.url().to_string())), + 429 => { + let retry_after = resp + .headers() + .get("retry-after") + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.parse().ok()) + .unwrap_or(60); + Err(RepoHostError::RateLimited { retry_after }) + } + _ => Err(RepoHostError::ServerError { + status, + body: String::new(), + }), + } +} + +fn parse_pr(value: &serde_json::Value) -> PullRequest { + let status = value["status"].as_str().unwrap_or("active"); + let state = match status { + "completed" => PrState::Merged, + "abandoned" => PrState::Closed, + _ => PrState::Open, + }; + PullRequest { + number: value["pullRequestId"].as_u64().unwrap_or(0), + title: value["title"].as_str().unwrap_or("").to_string(), + author: value["createdBy"]["uniqueName"] + .as_str() + .or_else(|| value["createdBy"]["displayName"].as_str()) + .unwrap_or("") + .to_string(), + state, + head_sha: value["lastMergeSourceCommit"]["commitId"] + .as_str() + .unwrap_or("") + .to_string(), + base_sha: value["lastMergeTargetCommit"]["commitId"] + .as_str() + .unwrap_or("") + .to_string(), + head_ref: strip_refs(value["sourceRefName"].as_str().unwrap_or("")), + base_ref: strip_refs(value["targetRefName"].as_str().unwrap_or("")), + body: value["description"].as_str().map(String::from), + created_at: value["creationDate"].as_str().unwrap_or("").to_string(), + // ADO doesn't expose a top-level updated_at; fall back to creation. + updated_at: value["closedDate"] + .as_str() + .or_else(|| value["creationDate"].as_str()) + .unwrap_or("") + .to_string(), + } +} + +fn strip_refs(r: &str) -> String { + r.strip_prefix("refs/heads/").unwrap_or(r).to_string() +} + +fn map_status_state(state: &str) -> (String, Option) { + // ADO PR status `state` → (GH-shaped status, conclusion) + match state { + "succeeded" => ("completed".into(), Some("success".into())), + "failed" => ("completed".into(), Some("failure".into())), + "error" => ("completed".into(), Some("failure".into())), + "notApplicable" => ("completed".into(), Some("neutral".into())), + "pending" => ("in_progress".into(), None), + "notSet" | "" => ("queued".into(), None), + other => (other.to_string(), None), + } +} + +#[async_trait] +impl RepoHostAdapter for AzureDevOpsAdapter { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + + async fn get_pr( + &self, + owner: &str, + repo: &str, + number: u64, + ) -> Result { + let url = format!( + "{}?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let value: serde_json::Value = self.get_json(&url).await?; + Ok(parse_pr(&value)) + } + + async fn get_pr_diff( + &self, + owner: &str, + repo: &str, + number: u64, + ) -> Result { + // ADO doesn't return a unified diff in one call. Synthesize + // by fetching the iteration's changes endpoint and rendering + // a placeholder; production callers should prefer + // `get_pr_status` + diff against the head SHA via local git. + // For now we surface a clear `Unsupported` so callers can + // route around it. + let _ = (owner, repo, number); + Err(RepoHostError::Unsupported( + "Azure DevOps unified-diff endpoint is not implemented; \ + diff via head SHA against base instead" + .into(), + )) + } + + async fn list_pr_comments( + &self, + owner: &str, + repo: &str, + number: u64, + ) -> Result, RepoHostError> { + let url = format!( + "{}/threads?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let value: serde_json::Value = self.get_json(&url).await?; + let mut out = Vec::new(); + let threads = value["value"].as_array().cloned().unwrap_or_default(); + for thread in threads { + // System-generated threads (e.g. status changes) have a + // synthetic author; skip if they have no comments. + let Some(comments) = thread["comments"].as_array() else { + continue; + }; + for c in comments { + out.push(Comment { + id: c["id"].as_u64().unwrap_or(0), + author: c["author"]["uniqueName"] + .as_str() + .or_else(|| c["author"]["displayName"].as_str()) + .unwrap_or("") + .to_string(), + body: c["content"].as_str().unwrap_or("").to_string(), + path: thread["threadContext"]["filePath"] + .as_str() + .map(String::from), + line: thread["threadContext"]["rightFileStart"]["line"].as_u64(), + created_at: c["publishedDate"].as_str().unwrap_or("").to_string(), + }); + } + } + Ok(out) + } + + async fn post_review( + &self, + owner: &str, + repo: &str, + number: u64, + review: Review, + ) -> Result<(), RepoHostError> { + // 1. Post the summary comment as a new thread. + if !review.body.is_empty() { + let thread_url = format!( + "{}/threads?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let thread_body = serde_json::json!({ + "comments": [{ + "parentCommentId": 0, + "content": review.body, + "commentType": 1, + }], + "status": 1, + }); + self.post_json(&thread_url, &thread_body).await?; + } + + // 2. Post each line comment as its own thread with file context. + for c in &review.comments { + let thread_url = format!( + "{}/threads?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let thread_body = serde_json::json!({ + "comments": [{ + "parentCommentId": 0, + "content": c.body, + "commentType": 1, + }], + "status": 1, + "threadContext": { + "filePath": c.path, + "rightFileStart": { "line": c.line, "offset": 1 }, + "rightFileEnd": { "line": c.line, "offset": 1 }, + }, + }); + self.post_json(&thread_url, &thread_body).await?; + } + + // 3. Cast the vote (reviewer self). + let vote = match review.event { + ReviewEvent::Approve => 10, + ReviewEvent::RequestChanges => -10, + ReviewEvent::Comment => 0, + }; + if vote != 0 { + // The reviewer id `me` resolves to the PAT's identity. + let vote_url = format!( + "{}/reviewers/me?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let body = serde_json::json!({ "vote": vote }); + self.patch_json(&vote_url, &body).await?; + } + Ok(()) + } + + async fn post_comment( + &self, + owner: &str, + repo: &str, + number: u64, + body: &str, + ) -> Result<(), RepoHostError> { + let url = format!( + "{}/threads?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let payload = serde_json::json!({ + "comments": [{ + "parentCommentId": 0, + "content": body, + "commentType": 1, + }], + "status": 1, + }); + self.post_json(&url, &payload).await?; + Ok(()) + } + + async fn get_pr_status( + &self, + owner: &str, + repo: &str, + number: u64, + ) -> Result { + // PR mergeStatus. + let pr_url = format!( + "{}?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let pr_value: serde_json::Value = self.get_json(&pr_url).await?; + let mergeable = pr_value["mergeStatus"] + .as_str() + .map(|s| matches!(s, "succeeded" | "queued")); + + // PR statuses (genre/name/state). + let st_url = format!( + "{}/statuses?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let st_value: serde_json::Value = self.get_json(&st_url).await?; + let checks = st_value["value"] + .as_array() + .cloned() + .unwrap_or_default() + .into_iter() + .map(|c| { + let state = c["state"].as_str().unwrap_or("").to_string(); + let (status, conclusion) = map_status_state(&state); + let name = c["context"]["name"] + .as_str() + .unwrap_or_else(|| c["description"].as_str().unwrap_or("")) + .to_string(); + CheckRun { + name, + status, + conclusion, + } + }) + .collect(); + + Ok(PrStatus { mergeable, checks }) + } + + async fn get_pr_head_sha( + &self, + owner: &str, + repo: &str, + number: u64, + ) -> Result { + let url = format!( + "{}?api-version={API_VERSION}", + self.pr_base(owner, repo, number)? + ); + let value: serde_json::Value = self.get_json(&url).await?; + Ok(value["lastMergeSourceCommit"]["commitId"] + .as_str() + .unwrap_or("") + .to_string()) + } + + async fn list_open_prs( + &self, + owner: &str, + repo: &str, + ) -> Result, RepoHostError> { + let url = format!( + "{}?searchCriteria.status=active&api-version={API_VERSION}", + self.list_base(owner, repo)? + ); + let value: serde_json::Value = self.get_json(&url).await?; + Ok(value["value"] + .as_array() + .cloned() + .unwrap_or_default() + .iter() + .map(parse_pr) + .collect()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn split_owner_requires_org_project() { + assert!(AzureDevOpsAdapter::split_owner("contoso").is_err()); + let ok = AzureDevOpsAdapter::split_owner("contoso/Acme").unwrap(); + assert_eq!(ok, ("contoso", "Acme")); + } + + #[test] + fn pr_url_layout() { + let a = AzureDevOpsAdapter::new("pat".into()); + let url = a.pr_base("contoso/Acme", "widgets", 42).unwrap(); + assert_eq!( + url, + "https://dev.azure.com/contoso/Acme/_apis/git/repositories/widgets/pullrequests/42" + ); + } + + #[test] + fn map_status_states() { + assert_eq!( + map_status_state("succeeded"), + ("completed".into(), Some("success".into())) + ); + assert_eq!(map_status_state("pending"), ("in_progress".into(), None)); + assert_eq!(map_status_state("notSet"), ("queued".into(), None)); + } + + #[test] + fn parse_pr_maps_status_to_state() { + let raw = serde_json::json!({ + "pullRequestId": 7, + "title": "Add ADO support", + "createdBy": { "uniqueName": "alice@example.com" }, + "status": "completed", + "lastMergeSourceCommit": { "commitId": "deadbeef" }, + "lastMergeTargetCommit": { "commitId": "cafef00d" }, + "sourceRefName": "refs/heads/feature/x", + "targetRefName": "refs/heads/main", + "creationDate": "2026-05-03T00:00:00Z", + }); + let pr = parse_pr(&raw); + assert_eq!(pr.number, 7); + assert_eq!(pr.state, PrState::Merged); + assert_eq!(pr.head_ref, "feature/x"); + assert_eq!(pr.base_ref, "main"); + assert_eq!(pr.head_sha, "deadbeef"); + } +} diff --git a/crates/devdev-integrations/src/github.rs b/crates/devdev-integrations/src/github.rs index 84900db..ada5adb 100644 --- a/crates/devdev-integrations/src/github.rs +++ b/crates/devdev-integrations/src/github.rs @@ -1,36 +1,59 @@ -//! Live GitHub adapter using reqwest. +//! Live GitHub REST adapter. +//! +//! Covers both **GitHub.com** (`https://api.github.com`) and any +//! **GitHub Enterprise Server** instance (`https:///api/v3`). +//! The two speak the same wire protocol; only the API base URL +//! differs. Construct via [`GitHubAdapter::new`] with an explicit +//! [`RepoHostId`], or via [`GitHubAdapter::github_com`] for the +//! common github.com case. use async_trait::async_trait; use reqwest::header::{ACCEPT, AUTHORIZATION, USER_AGENT}; use std::env; -use crate::GitHubAdapter; +use crate::RepoHostAdapter; +use crate::host::RepoHostId; use crate::rate_limit::RateLimitTracker; use crate::types::*; -const API_BASE: &str = "https://api.github.com"; - -/// Real GitHub API client. -pub struct LiveGitHubAdapter { +/// Real GitHub REST API client. +/// +/// Holds an [`RepoHostId`] (so the daemon registry can key on it), +/// the API base URL, the bearer token, and a rate-limit tracker. +pub struct GitHubAdapter { + host_id: RepoHostId, client: reqwest::Client, token: String, rate_limit: RateLimitTracker, } -impl LiveGitHubAdapter { - /// Create a new adapter, reading the token from `GH_TOKEN`. - pub fn from_env() -> Result { - let token = env::var("GH_TOKEN").map_err(|_| GitHubError::TokenNotSet)?; - Ok(Self { - client: reqwest::Client::new(), - token, - rate_limit: RateLimitTracker::new(), - }) +impl GitHubAdapter { + /// Build a github.com adapter, reading the token from `GH_TOKEN`. + /// + /// Equivalent to `Self::new(RepoHostId::github_com(), token)` with + /// the env-var lookup folded in. Provided for migration ease; + /// production daemons should resolve credentials through the + /// `devdev-daemon` `CredentialStore` and call [`Self::new`]. + pub fn from_env() -> Result { + let token = env::var("GH_TOKEN").map_err(|_| RepoHostError::TokenNotSet)?; + Ok(Self::new(RepoHostId::github_com(), token)) + } + + /// Build a github.com adapter with an explicit token. + pub fn github_com(token: String) -> Self { + Self::new(RepoHostId::github_com(), token) + } + + /// Build an adapter for a GitHub Enterprise Server install at + /// `host` (e.g. `ghe.example.com`). + pub fn ghe(host: impl Into, token: String) -> Self { + Self::new(RepoHostId::ghe(host), token) } - /// Create a new adapter with an explicit token. - pub fn with_token(token: String) -> Self { + /// Construct directly from a host id and token. + pub fn new(host_id: RepoHostId, token: String) -> Self { Self { + host_id, client: reqwest::Client::new(), token, rate_limit: RateLimitTracker::new(), @@ -42,8 +65,15 @@ impl LiveGitHubAdapter { &self.rate_limit } + fn api_base(&self) -> &str { + &self.host_id.api_base + } + /// Send a GET request and handle common errors. - async fn get_json(&self, url: &str) -> Result { + async fn get_json( + &self, + url: &str, + ) -> Result { let resp = self .client .get(url) @@ -57,11 +87,12 @@ impl LiveGitHubAdapter { self.check_status(&resp)?; let text = resp.text().await?; - serde_json::from_str(&text).map_err(|e| GitHubError::Deserialize(format!("{e}: {text}"))) + serde_json::from_str(&text) + .map_err(|e| RepoHostError::Deserialize(format!("{e}: {text}"))) } /// Send a GET and return raw text (for diffs). - async fn get_text(&self, url: &str, accept: &str) -> Result { + async fn get_text(&self, url: &str, accept: &str) -> Result { let resp = self .client .get(url) @@ -78,7 +109,11 @@ impl LiveGitHubAdapter { } /// Send a POST with JSON body. - async fn post_json(&self, url: &str, body: &serde_json::Value) -> Result<(), GitHubError> { + async fn post_json( + &self, + url: &str, + body: &serde_json::Value, + ) -> Result<(), RepoHostError> { let resp = self .client .post(url) @@ -111,12 +146,12 @@ impl LiveGitHubAdapter { self.rate_limit.update(remaining, reset); } - fn check_status(&self, resp: &reqwest::Response) -> Result<(), GitHubError> { + fn check_status(&self, resp: &reqwest::Response) -> Result<(), RepoHostError> { let status = resp.status().as_u16(); match status { 200..=299 => Ok(()), - 401 => Err(GitHubError::Unauthorized), - 404 => Err(GitHubError::NotFound(resp.url().to_string())), + 401 => Err(RepoHostError::Unauthorized), + 404 => Err(RepoHostError::NotFound(resp.url().to_string())), 429 => { let retry_after = resp .headers() @@ -124,9 +159,9 @@ impl LiveGitHubAdapter { .and_then(|v| v.to_str().ok()) .and_then(|v| v.parse().ok()) .unwrap_or(60); - Err(GitHubError::RateLimited { retry_after }) + Err(RepoHostError::RateLimited { retry_after }) } - _ => Err(GitHubError::ServerError { + _ => Err(RepoHostError::ServerError { status, body: String::new(), }), @@ -134,8 +169,8 @@ impl LiveGitHubAdapter { } } -/// Parse the GitHub API PR JSON response into our `PullRequest` type. -fn parse_pr(value: serde_json::Value) -> Result { +/// Parse a PR JSON response into [`PullRequest`]. +fn parse_pr(value: serde_json::Value) -> Result { let merged = value .get("merged") .and_then(|v| v.as_bool()) @@ -179,14 +214,18 @@ fn parse_comment(value: &serde_json::Value) -> Comment { } #[async_trait] -impl GitHubAdapter for LiveGitHubAdapter { +impl RepoHostAdapter for GitHubAdapter { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + async fn get_pr( &self, owner: &str, repo: &str, number: u64, - ) -> Result { - let url = format!("{API_BASE}/repos/{owner}/{repo}/pulls/{number}"); + ) -> Result { + let url = format!("{}/repos/{owner}/{repo}/pulls/{number}", self.api_base()); let value: serde_json::Value = self.get_json(&url).await?; parse_pr(value) } @@ -196,8 +235,8 @@ impl GitHubAdapter for LiveGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { - let url = format!("{API_BASE}/repos/{owner}/{repo}/pulls/{number}"); + ) -> Result { + let url = format!("{}/repos/{owner}/{repo}/pulls/{number}", self.api_base()); self.get_text(&url, "application/vnd.github.v3.diff").await } @@ -206,20 +245,21 @@ impl GitHubAdapter for LiveGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result, GitHubError> { + ) -> Result, RepoHostError> { let mut all_comments = Vec::new(); let mut page = 1u32; let max_pages = 10; loop { let url = format!( - "{API_BASE}/repos/{owner}/{repo}/pulls/{number}/comments?per_page=100&page={page}" + "{}/repos/{owner}/{repo}/pulls/{number}/comments?per_page=100&page={page}", + self.api_base() ); let value: serde_json::Value = self.get_json(&url).await?; let arr = value .as_array() - .ok_or_else(|| GitHubError::Deserialize("expected array".into()))?; + .ok_or_else(|| RepoHostError::Deserialize("expected array".into()))?; if arr.is_empty() { break; @@ -244,8 +284,11 @@ impl GitHubAdapter for LiveGitHubAdapter { repo: &str, number: u64, review: Review, - ) -> Result<(), GitHubError> { - let url = format!("{API_BASE}/repos/{owner}/{repo}/pulls/{number}/reviews"); + ) -> Result<(), RepoHostError> { + let url = format!( + "{}/repos/{owner}/{repo}/pulls/{number}/reviews", + self.api_base() + ); let event = match review.event { ReviewEvent::Approve => "APPROVE", @@ -280,8 +323,11 @@ impl GitHubAdapter for LiveGitHubAdapter { repo: &str, number: u64, body: &str, - ) -> Result<(), GitHubError> { - let url = format!("{API_BASE}/repos/{owner}/{repo}/issues/{number}/comments"); + ) -> Result<(), RepoHostError> { + let url = format!( + "{}/repos/{owner}/{repo}/issues/{number}/comments", + self.api_base() + ); let payload = serde_json::json!({ "body": body }); self.post_json(&url, &payload).await } @@ -291,15 +337,18 @@ impl GitHubAdapter for LiveGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { + ) -> Result { // Get PR for mergeable status - let pr_url = format!("{API_BASE}/repos/{owner}/{repo}/pulls/{number}"); + let pr_url = format!("{}/repos/{owner}/{repo}/pulls/{number}", self.api_base()); let pr_value: serde_json::Value = self.get_json(&pr_url).await?; let mergeable = pr_value["mergeable"].as_bool(); // Get check runs for the head SHA let head_sha = pr_value["head"]["sha"].as_str().unwrap_or(""); - let checks_url = format!("{API_BASE}/repos/{owner}/{repo}/commits/{head_sha}/check-runs"); + let checks_url = format!( + "{}/repos/{owner}/{repo}/commits/{head_sha}/check-runs", + self.api_base() + ); let checks_value: serde_json::Value = self.get_json(&checks_url).await?; let checks = checks_value["check_runs"] @@ -321,8 +370,8 @@ impl GitHubAdapter for LiveGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { - let url = format!("{API_BASE}/repos/{owner}/{repo}/pulls/{number}"); + ) -> Result { + let url = format!("{}/repos/{owner}/{repo}/pulls/{number}", self.api_base()); let value: serde_json::Value = self.get_json(&url).await?; Ok(value["head"]["sha"].as_str().unwrap_or("").to_string()) } @@ -331,18 +380,19 @@ impl GitHubAdapter for LiveGitHubAdapter { &self, owner: &str, repo: &str, - ) -> Result, GitHubError> { + ) -> Result, RepoHostError> { let mut all = Vec::new(); let mut page = 1u32; let max_pages = 10u32; loop { let url = format!( - "{API_BASE}/repos/{owner}/{repo}/pulls?state=open&per_page=100&page={page}" + "{}/repos/{owner}/{repo}/pulls?state=open&per_page=100&page={page}", + self.api_base() ); let value: serde_json::Value = self.get_json(&url).await?; let arr = value .as_array() - .ok_or_else(|| GitHubError::Deserialize("expected array".into()))?; + .ok_or_else(|| RepoHostError::Deserialize("expected array".into()))?; if arr.is_empty() { break; } @@ -357,3 +407,22 @@ impl GitHubAdapter for LiveGitHubAdapter { Ok(all) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn github_com_constructor_uses_dotcom_base() { + let a = GitHubAdapter::github_com("tok".into()); + assert_eq!(a.host_id().host, "github.com"); + assert_eq!(a.api_base(), "https://api.github.com"); + } + + #[test] + fn ghe_constructor_uses_v3_path() { + let a = GitHubAdapter::ghe("ghe.example.com", "tok".into()); + assert_eq!(a.host_id().host, "ghe.example.com"); + assert_eq!(a.api_base(), "https://ghe.example.com/api/v3"); + } +} diff --git a/crates/devdev-integrations/src/host.rs b/crates/devdev-integrations/src/host.rs new file mode 100644 index 0000000..cee08ed --- /dev/null +++ b/crates/devdev-integrations/src/host.rs @@ -0,0 +1,222 @@ +//! Host identification for repository forges. +//! +//! A `RepoHostId` pairs a forge family ([`RepoHostKind`]) with the +//! base URL of a specific instance. It is the routing key used by the +//! daemon's host registry to dispatch agent tool calls and watch-repo +//! event polling to the correct adapter implementation. +//! +//! Classification rules: +//! * `github.com` and any `*.ghe.com` host → [`RepoHostKind::GitHub`] +//! with API base `https:///api/v3` (GHE) or +//! `https://api.github.com` (github.com). +//! * `dev.azure.com` and `*.visualstudio.com` → +//! [`RepoHostKind::AzureDevOps`]. +//! * Anything else → unclassified; callers must supply the kind +//! explicitly via configuration. + +use serde::{Deserialize, Serialize}; + +/// Family of repository forge. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum RepoHostKind { + /// GitHub.com or a GitHub Enterprise Server instance. Both speak + /// the same REST surface; only the API base URL differs. + GitHub, + /// Azure DevOps Services (`dev.azure.com`) or a legacy + /// Visual Studio Team Services host. + AzureDevOps, +} + +/// Stable identifier for a forge instance: a kind + the canonical +/// API base URL. Used as a `HashMap` key in the daemon registry and +/// embedded in ledger entries / `PrRef` values. +/// +/// Constructed via [`RepoHostId::github_com`], [`RepoHostId::ghe`], or +/// [`RepoHostId::azure_devops`]; or via [`RepoHostId::from_browse_url`] +/// for URL-driven dispatch. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub struct RepoHostId { + pub kind: RepoHostKind, + /// API base URL **without** trailing slash, e.g. + /// `https://api.github.com` or `https://ghe.example.com/api/v3` or + /// `https://dev.azure.com`. + pub api_base: String, + /// Browse-URL host, e.g. `github.com`, `ghe.example.com`, + /// `dev.azure.com`. Used for ledger keys and human display. + pub host: String, +} + +impl RepoHostId { + /// `https://api.github.com` against `github.com`. + pub fn github_com() -> Self { + Self { + kind: RepoHostKind::GitHub, + api_base: "https://api.github.com".to_string(), + host: "github.com".to_string(), + } + } + + /// GitHub Enterprise Server instance hosted at `host` (e.g. + /// `ghe.example.com`). API base is `https:///api/v3`. + pub fn ghe(host: impl Into) -> Self { + let host = host.into(); + let api_base = format!("https://{host}/api/v3"); + Self { + kind: RepoHostKind::GitHub, + api_base, + host, + } + } + + /// Azure DevOps Services. The API base is the same for every org + /// (`https://dev.azure.com`); per-org routing happens in the URL + /// path, not the host. + pub fn azure_devops() -> Self { + Self { + kind: RepoHostKind::AzureDevOps, + api_base: "https://dev.azure.com".to_string(), + host: "dev.azure.com".to_string(), + } + } + + /// Stable string key suitable for use in the idempotency ledger + /// or any other deduplication store. Format: `:`. + pub fn ledger_key(&self) -> String { + let kind = match self.kind { + RepoHostKind::GitHub => "github", + RepoHostKind::AzureDevOps => "ado", + }; + format!("{kind}:{}", self.host) + } + + /// Best-effort classification of a *browse* URL host (the `host` + /// portion of a URL like `https://ghe.example.com/owner/repo`). + /// + /// Returns `None` when the host doesn't match any known forge. + /// Callers should fall through to explicit configuration in that + /// case rather than guessing. + pub fn classify_host(host: &str) -> Option { + let host = host.to_ascii_lowercase(); + if host == "github.com" || host == "www.github.com" { + return Some(RepoHostKind::GitHub); + } + if host == "dev.azure.com" || host.ends_with(".visualstudio.com") { + return Some(RepoHostKind::AzureDevOps); + } + // Heuristic: `ghe.*` or `github.*` (GitHub Enterprise Server + // installs commonly use these prefixes). Conservative — only + // hits when the segment is the literal "ghe" or "github". + if host.starts_with("ghe.") || host.starts_with("github.") { + return Some(RepoHostKind::GitHub); + } + None + } + + /// Build a [`RepoHostId`] from a browse-URL host string. Returns + /// `None` when classification fails. + pub fn from_browse_host(host: &str) -> Option { + match Self::classify_host(host)? { + RepoHostKind::GitHub => { + if host.eq_ignore_ascii_case("github.com") + || host.eq_ignore_ascii_case("www.github.com") + { + Some(Self::github_com()) + } else { + Some(Self::ghe(host)) + } + } + RepoHostKind::AzureDevOps => Some(Self::azure_devops()), + } + } +} + +impl std::fmt::Display for RepoHostId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.ledger_key()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn classify_github_com() { + assert_eq!( + RepoHostId::classify_host("github.com"), + Some(RepoHostKind::GitHub) + ); + assert_eq!( + RepoHostId::classify_host("GitHub.com"), + Some(RepoHostKind::GitHub) + ); + } + + #[test] + fn classify_ghe() { + assert_eq!( + RepoHostId::classify_host("ghe.example.com"), + Some(RepoHostKind::GitHub) + ); + assert_eq!( + RepoHostId::classify_host("github.example.com"), + Some(RepoHostKind::GitHub) + ); + } + + #[test] + fn classify_ado() { + assert_eq!( + RepoHostId::classify_host("dev.azure.com"), + Some(RepoHostKind::AzureDevOps) + ); + assert_eq!( + RepoHostId::classify_host("contoso.visualstudio.com"), + Some(RepoHostKind::AzureDevOps) + ); + } + + #[test] + fn classify_unknown() { + assert_eq!(RepoHostId::classify_host("gitlab.com"), None); + assert_eq!(RepoHostId::classify_host("bitbucket.org"), None); + } + + #[test] + fn ghe_api_base_path() { + let id = RepoHostId::ghe("ghe.example.com"); + assert_eq!(id.api_base, "https://ghe.example.com/api/v3"); + assert_eq!(id.host, "ghe.example.com"); + } + + #[test] + fn ledger_key_format() { + assert_eq!(RepoHostId::github_com().ledger_key(), "github:github.com"); + assert_eq!( + RepoHostId::ghe("ghe.acme.io").ledger_key(), + "github:ghe.acme.io" + ); + assert_eq!( + RepoHostId::azure_devops().ledger_key(), + "ado:dev.azure.com" + ); + } + + #[test] + fn from_browse_host_routes_correctly() { + assert_eq!( + RepoHostId::from_browse_host("github.com"), + Some(RepoHostId::github_com()) + ); + assert_eq!( + RepoHostId::from_browse_host("ghe.example.com"), + Some(RepoHostId::ghe("ghe.example.com")) + ); + assert_eq!( + RepoHostId::from_browse_host("dev.azure.com"), + Some(RepoHostId::azure_devops()) + ); + assert_eq!(RepoHostId::from_browse_host("gitlab.com"), None); + } +} diff --git a/crates/devdev-integrations/src/lib.rs b/crates/devdev-integrations/src/lib.rs index 793c083..7ca3fcf 100644 --- a/crates/devdev-integrations/src/lib.rs +++ b/crates/devdev-integrations/src/lib.rs @@ -1,29 +1,60 @@ -//! GitHub integration adapter for DevDev. +//! Repository-host integration adapters for DevDev. //! -//! Provides the `GitHubAdapter` trait and a `MockGitHubAdapter` for testing. -//! The `LiveGitHubAdapter` performs real HTTP calls to the GitHub REST API. +//! This crate exposes a host-agnostic [`RepoHostAdapter`] trait +//! covering the pull-request operations DevDev's tasks need: +//! fetching metadata, listing comments, posting reviews, reading +//! merge state, and discovering open PRs. +//! +//! Concrete implementations: +//! * [`GitHubAdapter`] — covers github.com **and** GitHub Enterprise +//! Server (the wire protocol is identical; only the API base URL +//! differs). +//! * [`AzureDevOpsAdapter`] — Azure DevOps Services REST 7.0. +//! * [`MockAdapter`] — in-memory test double, host-agnostic. +//! +//! Host routing is keyed by [`RepoHostId`] (see [`host`]). Callers +//! that already know the host construct an adapter directly; callers +//! that only have a URL (e.g. an MCP tool invocation from the agent) +//! classify the host first via [`RepoHostId::from_browse_host`] and +//! then look up the adapter in the daemon-side registry. +#![allow(clippy::result_large_err)] + +pub mod azure_devops; pub mod github; -pub mod github_mock; +pub mod host; +pub mod mock; pub mod rate_limit; pub mod types; -pub use github::LiveGitHubAdapter; -pub use github_mock::MockGitHubAdapter; +pub use azure_devops::AzureDevOpsAdapter; +pub use github::GitHubAdapter; +pub use host::{RepoHostId, RepoHostKind}; +pub use mock::MockAdapter; pub use types::*; use async_trait::async_trait; -/// Abstract interface for GitHub API operations. +/// Abstract pull-request operations against any supported forge. +/// +/// Methods take `(owner, repo, number)` for backwards compatibility +/// with the original GitHub-only surface. ADO's `org/project/repo` +/// triple is encoded as `owner = "/"`, `repo = ""` +/// (see [`AzureDevOpsAdapter`]). A future revision may introduce a +/// structured `RepoCoord` type. #[async_trait] -pub trait GitHubAdapter: Send + Sync { +pub trait RepoHostAdapter: Send + Sync { + /// Identifier of the forge instance this adapter talks to. Used + /// by the daemon registry as a routing/dedup key. + fn host_id(&self) -> &RepoHostId; + /// Fetch PR metadata. async fn get_pr( &self, owner: &str, repo: &str, number: u64, - ) -> Result; + ) -> Result; /// Fetch the unified diff for a PR. async fn get_pr_diff( @@ -31,7 +62,7 @@ pub trait GitHubAdapter: Send + Sync { owner: &str, repo: &str, number: u64, - ) -> Result; + ) -> Result; /// List all review comments on a PR. async fn list_pr_comments( @@ -39,7 +70,7 @@ pub trait GitHubAdapter: Send + Sync { owner: &str, repo: &str, number: u64, - ) -> Result, GitHubError>; + ) -> Result, RepoHostError>; /// Post a full review (approve, request changes, or comment). async fn post_review( @@ -48,7 +79,7 @@ pub trait GitHubAdapter: Send + Sync { repo: &str, number: u64, review: Review, - ) -> Result<(), GitHubError>; + ) -> Result<(), RepoHostError>; /// Post a single comment on a PR. async fn post_comment( @@ -57,7 +88,7 @@ pub trait GitHubAdapter: Send + Sync { repo: &str, number: u64, body: &str, - ) -> Result<(), GitHubError>; + ) -> Result<(), RepoHostError>; /// Get PR merge status and CI check runs. async fn get_pr_status( @@ -65,7 +96,7 @@ pub trait GitHubAdapter: Send + Sync { owner: &str, repo: &str, number: u64, - ) -> Result; + ) -> Result; /// Get the head SHA of the PR (for detecting new pushes). async fn get_pr_head_sha( @@ -73,13 +104,16 @@ pub trait GitHubAdapter: Send + Sync { owner: &str, repo: &str, number: u64, - ) -> Result; + ) -> Result; /// List open PRs in a repo. Adapters paginate internally; callers /// receive the flat union. Used by `RepoWatchTask` to discover /// new PRs without webhooks. - async fn list_open_prs(&self, owner: &str, repo: &str) - -> Result, GitHubError>; + async fn list_open_prs( + &self, + owner: &str, + repo: &str, + ) -> Result, RepoHostError>; } /// Stable fingerprint of a PR's reviewable state. Used as a ledger diff --git a/crates/devdev-integrations/src/github_mock.rs b/crates/devdev-integrations/src/mock.rs similarity index 78% rename from crates/devdev-integrations/src/github_mock.rs rename to crates/devdev-integrations/src/mock.rs index 92e96a5..71c484c 100644 --- a/crates/devdev-integrations/src/github_mock.rs +++ b/crates/devdev-integrations/src/mock.rs @@ -1,19 +1,26 @@ -//! Mock GitHub adapter for testing. +//! In-memory test double for [`crate::RepoHostAdapter`]. +//! +//! Host-agnostic: the same mock serves GitHub and Azure DevOps tests. +//! Construct with [`MockAdapter::new`] for a default github.com host +//! id, or [`MockAdapter::with_host`] to simulate any forge instance. use std::collections::HashMap; use std::sync::{Arc, Mutex}; use async_trait::async_trait; -use crate::GitHubAdapter; +use crate::RepoHostAdapter; +use crate::host::RepoHostId; use crate::types::*; type PrKey = (String, String, u64); type PostedReview = (String, String, u64, Review); type PostedComment = (String, String, u64, String); -/// Test double that returns canned responses and records outgoing calls. -pub struct MockGitHubAdapter { +/// In-memory double that returns canned responses and records +/// outgoing calls. Default host id is [`RepoHostId::github_com`]. +pub struct MockAdapter { + host_id: RepoHostId, prs: HashMap, diffs: HashMap, comments: HashMap>, @@ -24,9 +31,14 @@ pub struct MockGitHubAdapter { sha_overrides: Arc>>, } -impl MockGitHubAdapter { +impl MockAdapter { pub fn new() -> Self { + Self::with_host(RepoHostId::github_com()) + } + + pub fn with_host(host_id: RepoHostId) -> Self { Self { + host_id, prs: HashMap::new(), diffs: HashMap::new(), comments: HashMap::new(), @@ -90,7 +102,7 @@ impl MockGitHubAdapter { } } -impl Default for MockGitHubAdapter { +impl Default for MockAdapter { fn default() -> Self { Self::new() } @@ -101,20 +113,23 @@ fn key(owner: &str, repo: &str, number: u64) -> PrKey { } #[async_trait] -impl GitHubAdapter for MockGitHubAdapter { +impl RepoHostAdapter for MockAdapter { + fn host_id(&self) -> &RepoHostId { + &self.host_id + } + async fn get_pr( &self, owner: &str, repo: &str, number: u64, - ) -> Result { + ) -> Result { let mut pr = self .prs .get(&key(owner, repo, number)) .cloned() - .ok_or_else(|| GitHubError::NotFound(format!("{owner}/{repo}#{number}")))?; + .ok_or_else(|| RepoHostError::NotFound(format!("{owner}/{repo}#{number}")))?; - // Apply SHA override if present. if let Some(sha) = self .sha_overrides .lock() @@ -132,11 +147,11 @@ impl GitHubAdapter for MockGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { + ) -> Result { self.diffs .get(&key(owner, repo, number)) .cloned() - .ok_or_else(|| GitHubError::NotFound(format!("{owner}/{repo}#{number}"))) + .ok_or_else(|| RepoHostError::NotFound(format!("{owner}/{repo}#{number}"))) } async fn list_pr_comments( @@ -144,7 +159,7 @@ impl GitHubAdapter for MockGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result, GitHubError> { + ) -> Result, RepoHostError> { Ok(self .comments .get(&key(owner, repo, number)) @@ -158,7 +173,7 @@ impl GitHubAdapter for MockGitHubAdapter { repo: &str, number: u64, review: Review, - ) -> Result<(), GitHubError> { + ) -> Result<(), RepoHostError> { self.posted_reviews .lock() .unwrap() @@ -172,7 +187,7 @@ impl GitHubAdapter for MockGitHubAdapter { repo: &str, number: u64, body: &str, - ) -> Result<(), GitHubError> { + ) -> Result<(), RepoHostError> { self.posted_comments .lock() .unwrap() @@ -185,11 +200,11 @@ impl GitHubAdapter for MockGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { + ) -> Result { self.statuses .get(&key(owner, repo, number)) .cloned() - .ok_or_else(|| GitHubError::NotFound(format!("{owner}/{repo}#{number}"))) + .ok_or_else(|| RepoHostError::NotFound(format!("{owner}/{repo}#{number}"))) } async fn get_pr_head_sha( @@ -197,8 +212,7 @@ impl GitHubAdapter for MockGitHubAdapter { owner: &str, repo: &str, number: u64, - ) -> Result { - // Check override first. + ) -> Result { if let Some(sha) = self .sha_overrides .lock() @@ -210,14 +224,14 @@ impl GitHubAdapter for MockGitHubAdapter { self.prs .get(&key(owner, repo, number)) .map(|pr| pr.head_sha.clone()) - .ok_or_else(|| GitHubError::NotFound(format!("{owner}/{repo}#{number}"))) + .ok_or_else(|| RepoHostError::NotFound(format!("{owner}/{repo}#{number}"))) } async fn list_open_prs( &self, owner: &str, repo: &str, - ) -> Result, GitHubError> { + ) -> Result, RepoHostError> { let overrides = self.sha_overrides.lock().unwrap().clone(); let mut out = Vec::new(); for ((o, r, n), pr) in &self.prs { @@ -229,7 +243,6 @@ impl GitHubAdapter for MockGitHubAdapter { out.push(pr); } } - out.sort_by_key(|p| p.number); Ok(out) } } diff --git a/crates/devdev-integrations/src/types.rs b/crates/devdev-integrations/src/types.rs index 44ad2a7..31a103a 100644 --- a/crates/devdev-integrations/src/types.rs +++ b/crates/devdev-integrations/src/types.rs @@ -1,4 +1,10 @@ -//! Data types for GitHub API interactions. +//! Host-agnostic data types for repository forge interactions. +//! +//! These types intentionally avoid GitHub-specific vocabulary +//! (`check_runs`, `merge_commit_sha`, etc.) so the same shapes can +//! describe pull requests on GitHub.com, GitHub Enterprise, and +//! Azure DevOps. Adapter implementations are responsible for the +//! lossy mappings. use serde::{Deserialize, Serialize}; @@ -61,6 +67,14 @@ pub struct PrStatus { pub checks: Vec, } +/// A unifying status-check record. +/// +/// On GitHub this maps to a Checks API entry (`status`, +/// `conclusion`). On Azure DevOps it maps to a PR status policy +/// entry (`state`, `genre/name`). The `status` field follows the +/// GitHub vocabulary (`queued`, `in_progress`, `completed`) for +/// historical compatibility; ADO mappings are documented in the +/// `azure_devops` adapter module. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CheckRun { pub name: String, @@ -68,10 +82,14 @@ pub struct CheckRun { pub conclusion: Option, } -/// Errors from GitHub API interactions. +/// Errors from any [`crate::RepoHostAdapter`] implementation. +/// +/// Adapter-specific status codes are mapped to the closest abstract +/// variant; the `body` field on [`RepoHostError::ServerError`] +/// preserves the wire-level detail for diagnostics. #[derive(thiserror::Error, Debug)] -pub enum GitHubError { - #[error("authentication failed: check GH_TOKEN")] +pub enum RepoHostError { + #[error("authentication failed: token missing or invalid")] Unauthorized, #[error("not found: {0}")] @@ -86,15 +104,18 @@ pub enum GitHubError { #[error("network error: {0}")] Network(String), - #[error("token not set: GH_TOKEN environment variable is required")] + #[error("token not set: a credential is required for this host")] TokenNotSet, #[error("deserialization error: {0}")] Deserialize(String), + + #[error("unsupported operation: {0}")] + Unsupported(String), } -impl From for GitHubError { +impl From for RepoHostError { fn from(e: reqwest::Error) -> Self { - GitHubError::Network(e.to_string()) + RepoHostError::Network(e.to_string()) } } diff --git a/crates/devdev-integrations/tests/acceptance.rs b/crates/devdev-integrations/tests/acceptance.rs index 7b36c94..e9d4e8a 100644 --- a/crates/devdev-integrations/tests/acceptance.rs +++ b/crates/devdev-integrations/tests/acceptance.rs @@ -1,10 +1,10 @@ //! Acceptance tests for P2-05 — GitHub Integration Adapter. //! -//! These tests use the MockGitHubAdapter. Live API tests are gated +//! These tests use the MockAdapter. Live API tests are gated //! behind DEVDEV_E2E (not run in CI). use devdev_integrations::{ - CheckRun, Comment, GitHubAdapter, GitHubError, MockGitHubAdapter, PrState, PrStatus, + CheckRun, Comment, RepoHostAdapter, RepoHostError, MockAdapter, PrState, PrStatus, PullRequest, Review, ReviewComment, ReviewEvent, }; @@ -28,7 +28,7 @@ fn sample_pr() -> PullRequest { #[tokio::test] async fn mock_returns_configured_pr() { - let adapter = MockGitHubAdapter::new().with_pr("org", "repo", sample_pr()); + let adapter = MockAdapter::new().with_pr("org", "repo", sample_pr()); let pr = adapter.get_pr("org", "repo", 42).await.unwrap(); assert_eq!(pr.number, 42); @@ -42,9 +42,9 @@ async fn mock_returns_configured_pr() { #[tokio::test] async fn mock_get_pr_not_found() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); let err = adapter.get_pr("org", "repo", 999).await.err().unwrap(); - assert!(matches!(err, GitHubError::NotFound(_))); + assert!(matches!(err, RepoHostError::NotFound(_))); } // ── Mock: get_pr_diff ────────────────────────────────────────── @@ -53,7 +53,7 @@ async fn mock_get_pr_not_found() { async fn mock_get_pr_diff_returns_diff() { let diff = "diff --git a/file.rs b/file.rs\n--- a/file.rs\n+++ b/file.rs\n@@ -1 +1 @@\n-old\n+new\n"; - let adapter = MockGitHubAdapter::new().with_diff("org", "repo", 42, diff); + let adapter = MockAdapter::new().with_diff("org", "repo", 42, diff); let result = adapter.get_pr_diff("org", "repo", 42).await.unwrap(); assert_eq!(result, diff); @@ -81,7 +81,7 @@ async fn mock_list_comments() { created_at: "2025-01-01T01:00:00Z".into(), }, ]; - let adapter = MockGitHubAdapter::new().with_comments("org", "repo", 42, comments); + let adapter = MockAdapter::new().with_comments("org", "repo", 42, comments); let result = adapter.list_pr_comments("org", "repo", 42).await.unwrap(); assert_eq!(result.len(), 2); @@ -93,7 +93,7 @@ async fn mock_list_comments() { #[tokio::test] async fn mock_list_comments_empty_for_unknown_pr() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); let result = adapter.list_pr_comments("org", "repo", 99).await.unwrap(); assert!(result.is_empty()); } @@ -102,7 +102,7 @@ async fn mock_list_comments_empty_for_unknown_pr() { #[tokio::test] async fn mock_records_posted_reviews() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); let review = Review { event: ReviewEvent::Comment, @@ -133,7 +133,7 @@ async fn mock_records_posted_reviews() { #[tokio::test] async fn mock_records_posted_comments() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); adapter .post_comment("org", "repo", 42, "Nice work!") @@ -154,7 +154,7 @@ async fn mock_records_posted_comments() { #[tokio::test] async fn mock_get_pr_head_sha() { - let adapter = MockGitHubAdapter::new().with_pr("org", "repo", sample_pr()); + let adapter = MockAdapter::new().with_pr("org", "repo", sample_pr()); let sha = adapter.get_pr_head_sha("org", "repo", 42).await.unwrap(); assert_eq!(sha, "abc123"); @@ -180,7 +180,7 @@ async fn mock_get_pr_status() { ], }; - let adapter = MockGitHubAdapter::new().with_status("org", "repo", 42, status); + let adapter = MockAdapter::new().with_status("org", "repo", 42, status); let result = adapter.get_pr_status("org", "repo", 42).await.unwrap(); assert_eq!(result.mergeable, Some(true)); @@ -193,22 +193,22 @@ async fn mock_get_pr_status() { #[tokio::test] async fn mock_diff_not_found() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); let err = adapter.get_pr_diff("org", "repo", 99).await.err().unwrap(); - assert!(matches!(err, GitHubError::NotFound(_))); + assert!(matches!(err, RepoHostError::NotFound(_))); } // ── Mock: status not found ───────────────────────────────────── #[tokio::test] async fn mock_status_not_found() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); let err = adapter .get_pr_status("org", "repo", 99) .await .err() .unwrap(); - assert!(matches!(err, GitHubError::NotFound(_))); + assert!(matches!(err, RepoHostError::NotFound(_))); } // ── Live: token_not_set_errors ───────────────────────────────── @@ -218,10 +218,10 @@ fn token_not_set_errors() { // Ensure GH_TOKEN is not set for this test // SAFETY: No other threads are reading GH_TOKEN concurrently in this test. unsafe { std::env::remove_var("GH_TOKEN") }; - let result = devdev_integrations::LiveGitHubAdapter::from_env(); + let result = devdev_integrations::GitHubAdapter::from_env(); assert!(result.is_err()); match result.err().unwrap() { - GitHubError::TokenNotSet => {} + RepoHostError::TokenNotSet => {} e => panic!("expected TokenNotSet, got: {e}"), } } @@ -230,7 +230,7 @@ fn token_not_set_errors() { #[tokio::test] async fn mock_post_review_preserves_event_type() { - let adapter = MockGitHubAdapter::new(); + let adapter = MockAdapter::new(); // Post an approval let review = Review { @@ -265,7 +265,7 @@ async fn mock_pr_state_variants() { merged_pr.number = 20; merged_pr.state = PrState::Merged; - let adapter = MockGitHubAdapter::new() + let adapter = MockAdapter::new() .with_pr("org", "repo", sample_pr()) // Open, #42 .with_pr("org", "repo", closed_pr) .with_pr("org", "repo", merged_pr); diff --git a/crates/devdev-scenarios/catalog/S08-multi-host-registry.md b/crates/devdev-scenarios/catalog/S08-multi-host-registry.md new file mode 100644 index 0000000..47786b5 --- /dev/null +++ b/crates/devdev-scenarios/catalog/S08-multi-host-registry.md @@ -0,0 +1,69 @@ +--- +id: S08 +title: Multi-host adapter registry routes by host +status: ready +blocked-on: [] +--- + +# S08 — Multi-host adapter registry routes by host + +**User story.** A team mirrors the same PR identity (`platform/api#42`) +on `github.com` and on their internal `ghe.acme.io`. They run +`devdev up`, then watch both repos and add a monitor task for each. +The daemon must keep the two PRs separate at every layer — task +keys, ledger keys, event identities — so a comment posted to one +never lands on the other, and a credential intended for one host is +never surfaced to the other. + +This scenario validates the [`RepoHostRegistry`](../../devdev-daemon/src/host_registry.rs) +seam end-to-end. The actual ADO/GHE HTTP traffic is covered by +adapter-level tests in `devdev-integrations`; S08's job is to prove +the **dispatch layer** never collides identities across hosts. + +## Steps + +1. Run `devdev up --data-dir --github mock` and wait for the + daemon's port file to appear. +2. IPC `repo/watch` with `{owner: "platform", repo: "api"}` + (default host: `github.com`). +3. IPC `repo/watch` with `{owner: "platform", repo: "api", host: + "ghe.acme.io"}`. +4. IPC `status`. Capture both task ids. +5. IPC `repo/watch` again for `{owner: "platform", repo: "api", + host: "ghe.acme.io"}` (idempotent re-registration). +6. IPC `repo/unwatch` for the github.com pair. +7. IPC `status`. +8. IPC `repo/unwatch` for the ghe.acme.io pair. +9. `devdev down`. + +## Assertions + +* Step 2 and step 3 both return `already_watching: false` with + **distinct** `task_id`s — same `(owner, repo)` on different + hosts must not collide. +* Step 4 reports two `repo-watch` tasks. +* Step 5 returns `already_watching: true` and the same `task_id` + as step 3. +* After step 6, only the ghe.acme.io watch remains. +* Step 7 reports exactly one `repo-watch` task (the GHE one). +* `repo/watch` with `host: "gitlab.example.com"` returns a + `-32602` error ("not a recognised repo host") — unknown hosts + are hard rejections, not silent github.com fallbacks. + +## Guards against + +* A regression where `repo_watch_ids` keys lose their `RepoHostId` + prefix and conflate hosts. +* A change to `RepoHostId::from_browse_host` that accidentally + classifies an unrelated forge as github/ghe/ado. +* A future credential-routing bug that surfaces a github.com token + in response to a `host: "ghe.acme.io"` ask. + +## Notes + +This scenario does **not** exercise `MonitorPrTask` review +posting (the `gh`/`az` token surface is covered by the ask +provider unit tests in +[crates/devdev-daemon/src/mcp/provider.rs](../../devdev-daemon/src/mcp/provider.rs)). +S08's job is to prove the host id propagates through the user- +visible IPC surface so that the registry can do its job. diff --git a/crates/devdev-scenarios/tests/scenarios.rs b/crates/devdev-scenarios/tests/scenarios.rs index efa9389..f5a6889 100644 --- a/crates/devdev-scenarios/tests/scenarios.rs +++ b/crates/devdev-scenarios/tests/scenarios.rs @@ -298,3 +298,146 @@ async fn s07_repo_watch_task_lifecycle() { let after = DirSnapshot::capture(scratch.outer()).expect("snapshot after"); assert_confined(scratch.outer(), &scratch.data_dir, &before, &after); } + + +// __ S08 _________________________________________________________ + +/// S08 \u2014 Multi-host adapter registry routes by host. +/// See: crates/devdev-scenarios/catalog/S08-multi-host-registry.md +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn s08_multi_host_registry_routes_by_host() { + let scratch = Scratch::new(); + let before = DirSnapshot::capture(scratch.outer()).expect("snapshot before"); + + let mut daemon = DaemonProcess::spawn(&scratch.data_dir, false).expect("devdev up"); + + let task_count = |v: &serde_json::Value| -> u64 { + v.get("tasks") + .and_then(|t| t.as_u64()) + .expect("status.tasks is u64") + }; + + let baseline = task_count(&daemon.status().await.expect("status baseline")); + + // Step 2: watch on default host (github.com). + let mut client = daemon.connect().await.expect("connect github"); + let resp = client + .request( + "repo/watch", + serde_json::json!({ "owner": "platform", "repo": "api" }), + ) + .await + .expect("repo/watch github"); + let r_gh = resp.result.expect("github result"); + assert_eq!(r_gh["already_watching"], serde_json::json!(false)); + let gh_task = r_gh["task_id"].as_str().expect("github task_id").to_string(); + + // Step 3: watch the same (owner, repo) on a GHE host. Must NOT + // collapse onto the github.com entry \u2014 distinct task_id. + let mut client = daemon.connect().await.expect("connect ghe"); + let resp = client + .request( + "repo/watch", + serde_json::json!({ + "owner": "platform", + "repo": "api", + "host": "ghe.acme.io", + }), + ) + .await + .expect("repo/watch ghe"); + let r_ghe = resp.result.expect("ghe result"); + assert_eq!(r_ghe["already_watching"], serde_json::json!(false)); + let ghe_task = r_ghe["task_id"].as_str().expect("ghe task_id").to_string(); + + assert_ne!( + gh_task, ghe_task, + "same (owner, repo) on different hosts must produce distinct task_ids" + ); + + // Step 4: both tasks visible. + let after_two = task_count(&daemon.status().await.expect("status after two")); + assert_eq!( + after_two, + baseline + 2, + "two distinct hosts must yield two tasks" + ); + + // Step 5: idempotent re-registration on the GHE side. + let mut client = daemon.connect().await.expect("connect ghe again"); + let resp = client + .request( + "repo/watch", + serde_json::json!({ + "owner": "platform", + "repo": "api", + "host": "ghe.acme.io", + }), + ) + .await + .expect("repo/watch ghe idempotent"); + let r_ghe2 = resp.result.expect("ghe2 result"); + assert_eq!(r_ghe2["already_watching"], serde_json::json!(true)); + assert_eq!(r_ghe2["task_id"].as_str(), Some(ghe_task.as_str())); + + // Unknown hosts are hard rejections (not silent github fallback). + let mut client = daemon.connect().await.expect("connect bogus"); + let resp = client + .request( + "repo/watch", + serde_json::json!({ + "owner": "platform", + "repo": "api", + "host": "gitlab.example.com", + }), + ) + .await + .expect("repo/watch bogus"); + assert!(resp.result.is_none(), "unknown host must error, not succeed"); + let err = resp.error.expect("error payload"); + assert_eq!(err.code, -32602, "expected invalid-params code"); + + // Step 6: unwatch the github.com entry. The GHE one must persist. + let mut client = daemon.connect().await.expect("connect unwatch gh"); + let resp = client + .request( + "repo/unwatch", + serde_json::json!({ "owner": "platform", "repo": "api" }), + ) + .await + .expect("repo/unwatch github"); + let r_uw_gh = resp.result.expect("unwatch gh result"); + assert_eq!(r_uw_gh["task_id"].as_str(), Some(gh_task.as_str())); + + // Step 7: cancellation must not bring task count above the + // pre-unwatch high-water-mark. The registry may keep cancelled + // entries around in a tombstoned state (matches S07's contract); + // the proof of separation is that step 6 returned `gh_task` and + // not `ghe_task`, asserted above. + let after_one = task_count(&daemon.status().await.expect("status after gh unwatch")); + assert!( + after_one <= after_two, + "github unwatch must not increase task count: was {after_two}, now {after_one}" + ); + + // Step 8: unwatch GHE. + let mut client = daemon.connect().await.expect("connect unwatch ghe"); + let resp = client + .request( + "repo/unwatch", + serde_json::json!({ + "owner": "platform", + "repo": "api", + "host": "ghe.acme.io", + }), + ) + .await + .expect("repo/unwatch ghe"); + let r_uw_ghe = resp.result.expect("unwatch ghe result"); + assert_eq!(r_uw_ghe["task_id"].as_str(), Some(ghe_task.as_str())); + + daemon.shutdown().expect("devdev down"); + + let after = DirSnapshot::capture(scratch.outer()).expect("snapshot after"); + assert_confined(scratch.outer(), &scratch.data_dir, &before, &after); +} \ No newline at end of file diff --git a/crates/devdev-tasks/src/events.rs b/crates/devdev-tasks/src/events.rs index 03786a9..4c373dd 100644 --- a/crates/devdev-tasks/src/events.rs +++ b/crates/devdev-tasks/src/events.rs @@ -5,6 +5,7 @@ //! without taking a daemon dependency. The bus is a thin //! `tokio::sync::broadcast` wrapper — see [`EventBus::publish`]. +use devdev_integrations::host::RepoHostId; use serde::{Deserialize, Serialize}; use tokio::sync::broadcast; @@ -14,18 +15,21 @@ const CHANNEL_CAPACITY: usize = 1024; #[serde(tag = "kind", rename_all = "snake_case")] pub enum DaemonEvent { PrOpened { + host_id: RepoHostId, owner: String, repo: String, number: u64, head_sha: String, }, PrUpdated { + host_id: RepoHostId, owner: String, repo: String, number: u64, head_sha: String, }, PrClosed { + host_id: RepoHostId, owner: String, repo: String, number: u64, @@ -34,28 +38,34 @@ pub enum DaemonEvent { } impl DaemonEvent { - /// `(owner, repo, number)` — subscribers filter the broadcast on - /// this tuple to scope to a single PR. - pub fn pr_target(&self) -> Option<(&str, &str, u64)> { + /// `(host_id, owner, repo, number)` — subscribers filter the + /// broadcast on this tuple to scope to a single PR. Identical + /// `(owner, repo, number)` triples on different hosts (e.g. a + /// fork on github.com and a mirror on a GHE install) MUST not + /// collide; the host_id is the disambiguator. + pub fn pr_target(&self) -> Option<(&RepoHostId, &str, &str, u64)> { match self { DaemonEvent::PrOpened { + host_id, owner, repo, number, .. } | DaemonEvent::PrUpdated { + host_id, owner, repo, number, .. } | DaemonEvent::PrClosed { + host_id, owner, repo, number, .. - } => Some((owner.as_str(), repo.as_str(), *number)), + } => Some((host_id, owner.as_str(), repo.as_str(), *number)), } } } @@ -98,6 +108,7 @@ mod tests { fn opened(n: u64) -> DaemonEvent { DaemonEvent::PrOpened { + host_id: RepoHostId::github_com(), owner: "o".into(), repo: "r".into(), number: n, @@ -132,14 +143,38 @@ mod tests { #[tokio::test] async fn pr_target_extracts_tuple() { + let host = RepoHostId::github_com(); let e = opened(42); - assert_eq!(e.pr_target(), Some(("o", "r", 42))); + assert_eq!(e.pr_target(), Some((&host, "o", "r", 42))); let c = DaemonEvent::PrClosed { + host_id: host.clone(), owner: "o".into(), repo: "r".into(), number: 42, merged: true, }; - assert_eq!(c.pr_target(), Some(("o", "r", 42))); + assert_eq!(c.pr_target(), Some((&host, "o", "r", 42))); + } + + #[tokio::test] + async fn pr_target_disambiguates_by_host() { + // Same (owner, repo, number) on github.com vs a GHE install + // must produce distinct event identities. + let gh = DaemonEvent::PrOpened { + host_id: RepoHostId::github_com(), + owner: "o".into(), + repo: "r".into(), + number: 1, + head_sha: "a".into(), + }; + let ghe = DaemonEvent::PrOpened { + host_id: RepoHostId::ghe("ghe.example.com"), + owner: "o".into(), + repo: "r".into(), + number: 1, + head_sha: "a".into(), + }; + assert_ne!(gh, ghe); + assert_ne!(gh.pr_target().unwrap().0, ghe.pr_target().unwrap().0); } } diff --git a/crates/devdev-tasks/src/monitor_pr.rs b/crates/devdev-tasks/src/monitor_pr.rs index 18037ad..45bd64c 100644 --- a/crates/devdev-tasks/src/monitor_pr.rs +++ b/crates/devdev-tasks/src/monitor_pr.rs @@ -15,7 +15,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; -use devdev_integrations::GitHubAdapter; +use devdev_integrations::RepoHostAdapter; use tokio::sync::broadcast::{Receiver, error::TryRecvError}; use crate::agent::AgentRunner; @@ -31,7 +31,7 @@ pub struct MonitorPrTask { last_sha: Option, observations: Vec, poll_interval: Duration, - github: Arc, + github: Arc, runner: Arc, rx: Receiver, /// Paths to `.devdev/*.md` preference files (Vibe Check, Phase D). @@ -43,7 +43,7 @@ impl MonitorPrTask { pub fn new( id: String, pr_ref_str: &str, - github: Arc, + github: Arc, runner: Arc, bus: &EventBus, ) -> Result { @@ -83,8 +83,11 @@ impl MonitorPrTask { /// Whether an event targets this task's PR. fn matches(&self, ev: &DaemonEvent) -> bool { match ev.pr_target() { - Some((o, r, n)) => { - o == self.pr_ref.owner && r == self.pr_ref.repo && n == self.pr_ref.number + Some((host, o, r, n)) => { + host == &self.pr_ref.host_id + && o == self.pr_ref.owner + && r == self.pr_ref.repo + && n == self.pr_ref.number } None => false, } diff --git a/crates/devdev-tasks/src/pr_ref.rs b/crates/devdev-tasks/src/pr_ref.rs index 4286455..e46404e 100644 --- a/crates/devdev-tasks/src/pr_ref.rs +++ b/crates/devdev-tasks/src/pr_ref.rs @@ -1,31 +1,56 @@ //! Parse PR references from strings. //! -//! Supports: "owner/repo#123", "https://github.com/owner/repo/pull/123" +//! # Supported syntaxes +//! +//! * Shorthand: `owner/repo#123` (assumed `github.com`). +//! * GitHub.com: `https://github.com/owner/repo/pull/123` +//! * GitHub Enterprise: `https:///owner/repo/pull/123` +//! (any host classified as GitHub by [`RepoHostId::classify_host`] +//! that isn't `github.com`). +//! * Azure DevOps Services: +//! `https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` +//! * Legacy Azure DevOps: +//! `https://{org}.visualstudio.com/{project}/_git/{repo}/pullrequest/{id}` +//! +//! For ADO, `(org, project, repo)` is collapsed into the trait's +//! `(owner, repo)` slot as `owner = "{org}/{project}"`, `repo = "{repo}"`. +//! This matches the encoding used by `AzureDevOpsAdapter` (see the +//! mappings table in that module). +//! +//! Every `PrRef` carries a [`RepoHostId`] so downstream callers can +//! route to the correct adapter and form ledger keys. + +use devdev_integrations::host::{RepoHostId, RepoHostKind}; use crate::task::TaskError; -/// Parsed PR reference. +/// Parsed PR reference. `host_id` identifies the forge instance; +/// `owner` and `repo` are interpreted in that forge's idiom (see the +/// module rustdoc for the ADO encoding). #[derive(Debug, Clone, PartialEq, Eq)] pub struct PrRef { + pub host_id: RepoHostId, pub owner: String, pub repo: String, pub number: u64, } impl PrRef { - /// Parse a PR reference from shorthand ("owner/repo#123") or URL. + /// Parse a PR reference. See the module rustdoc for accepted + /// syntaxes. pub fn parse(input: &str) -> Result { let input = input.trim(); - // Try URL: https://github.com/owner/repo/pull/123 if input.starts_with("https://") || input.starts_with("http://") { return Self::parse_url(input); } - // Try shorthand: owner/repo#123 Self::parse_shorthand(input) } + /// Shorthand always means github.com — there's no concise host- + /// disambiguating syntax for GHE/ADO, and the agent should be + /// passing full URLs anyway in those cases. fn parse_shorthand(input: &str) -> Result { let Some((repo_part, number_str)) = input.split_once('#') else { return Err(TaskError::PollFailed(format!( @@ -50,6 +75,7 @@ impl PrRef { } Ok(Self { + host_id: RepoHostId::github_com(), owner: owner.to_string(), repo: repo.to_string(), number, @@ -57,40 +83,266 @@ impl PrRef { } fn parse_url(input: &str) -> Result { - // Remove scheme. - let path = input - .strip_prefix("https://github.com/") - .or_else(|| input.strip_prefix("http://github.com/")) - .ok_or_else(|| TaskError::PollFailed(format!("unsupported URL host: {input}")))?; + let after_scheme = input + .strip_prefix("https://") + .or_else(|| input.strip_prefix("http://")) + .expect("caller checked scheme"); + + let (host, path) = match after_scheme.split_once('/') { + Some((h, p)) => (h, p), + None => { + return Err(TaskError::PollFailed(format!( + "invalid PR URL: {input} (missing path)" + ))); + } + }; + + let host_id = RepoHostId::from_browse_host(host).ok_or_else(|| { + TaskError::PollFailed(format!("unsupported PR URL host: {host} (in {input})")) + })?; + + match host_id.kind { + RepoHostKind::GitHub => Self::parse_github_path(host_id, path, input), + RepoHostKind::AzureDevOps => Self::parse_ado_path(host_id, host, path, input), + } + } - // Expected: owner/repo/pull/123 + /// `owner/repo/pull/{number}` (trailing path segments allowed). + fn parse_github_path( + host_id: RepoHostId, + path: &str, + input: &str, + ) -> Result { let parts: Vec<&str> = path.split('/').collect(); if parts.len() < 4 || parts[2] != "pull" { return Err(TaskError::PollFailed(format!( - "invalid PR URL: {input} (expected .../owner/repo/pull/number)" + "invalid GitHub PR URL: {input} (expected .../owner/repo/pull/number)" ))); } - let owner = parts[0]; let repo = parts[1]; let number: u64 = parts[3].parse().map_err(|_| { TaskError::PollFailed(format!("invalid PR number in URL: {}", parts[3])) })?; - if owner.is_empty() || repo.is_empty() || number == 0 { return Err(TaskError::PollFailed(format!("invalid PR URL: {input}"))); } - Ok(Self { + host_id, owner: owner.to_string(), repo: repo.to_string(), number, }) } + + /// Two ADO URL shapes are accepted: + /// + /// * `dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` + /// * `{org}.visualstudio.com/{project}/_git/{repo}/pullrequest/{id}` + /// (legacy host; org is the leftmost host label). + fn parse_ado_path( + host_id: RepoHostId, + host: &str, + path: &str, + input: &str, + ) -> Result { + let parts: Vec<&str> = path.split('/').collect(); + let lower_host = host.to_ascii_lowercase(); + + let (org, project, repo, id_str) = if lower_host == "dev.azure.com" { + if parts.len() < 6 || parts[2] != "_git" || parts[4] != "pullrequest" { + return Err(TaskError::PollFailed(format!( + "invalid ADO PR URL: {input} \ + (expected ///_git//pullrequest/)" + ))); + } + (parts[0], parts[1], parts[3], parts[5]) + } else if lower_host.ends_with(".visualstudio.com") { + let org = lower_host + .strip_suffix(".visualstudio.com") + .expect("just checked suffix"); + if parts.len() < 5 || parts[1] != "_git" || parts[3] != "pullrequest" { + return Err(TaskError::PollFailed(format!( + "invalid ADO PR URL: {input} \ + (expected //_git//pullrequest/ on visualstudio.com)" + ))); + } + // Detach from the borrow of `lower_host` by allocating now. + let org_owned: String = org.to_string(); + return Self::ado_finalise(host_id, &org_owned, parts[0], parts[2], parts[4], input); + } else { + return Err(TaskError::PollFailed(format!( + "unrecognised ADO host: {host}" + ))); + }; + + Self::ado_finalise(host_id, org, project, repo, id_str, input) + } + + fn ado_finalise( + host_id: RepoHostId, + org: &str, + project: &str, + repo: &str, + id_str: &str, + input: &str, + ) -> Result { + let number: u64 = id_str + .parse() + .map_err(|_| TaskError::PollFailed(format!("invalid ADO PR id: {id_str}")))?; + if org.is_empty() || project.is_empty() || repo.is_empty() || number == 0 { + return Err(TaskError::PollFailed(format!("invalid ADO PR URL: {input}"))); + } + Ok(Self { + host_id, + owner: format!("{org}/{project}"), + repo: repo.to_string(), + number, + }) + } } impl std::fmt::Display for PrRef { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Display intentionally omits host so existing log lines and + // resource ids stay shape-stable. Use `host_id.ledger_key()` + // when host is needed. write!(f, "{}/{}#{}", self.owner, self.repo, self.number) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn gh() -> RepoHostId { + RepoHostId::github_com() + } + + // ── Shorthand ─────────────────────────────────────────────── + + #[test] + fn shorthand_defaults_to_github_com() { + let r = PrRef::parse("owner/repo#123").unwrap(); + assert_eq!(r.host_id, gh()); + assert_eq!(r.owner, "owner"); + assert_eq!(r.repo, "repo"); + assert_eq!(r.number, 123); + } + + #[test] + fn shorthand_rejects_missing_hash() { + assert!(PrRef::parse("owner/repo").is_err()); + } + + #[test] + fn shorthand_rejects_zero_number() { + assert!(PrRef::parse("o/r#0").is_err()); + } + + #[test] + fn shorthand_rejects_empty_segments() { + assert!(PrRef::parse("/r#1").is_err()); + assert!(PrRef::parse("o/#1").is_err()); + assert!(PrRef::parse("#1").is_err()); + } + + // ── GitHub.com ────────────────────────────────────────────── + + #[test] + fn github_com_url_round_trips() { + let r = PrRef::parse("https://github.com/o/r/pull/42").unwrap(); + assert_eq!(r.host_id, RepoHostId::github_com()); + assert_eq!(r.owner, "o"); + assert_eq!(r.repo, "r"); + assert_eq!(r.number, 42); + } + + #[test] + fn github_com_url_with_trailing_path_extracts_pr() { + let r = PrRef::parse("https://github.com/o/r/pull/42/files").unwrap(); + assert_eq!(r.host_id, RepoHostId::github_com()); + assert_eq!(r.number, 42); + } + + #[test] + fn github_com_url_rejects_non_pull_path() { + assert!(PrRef::parse("https://github.com/o/r/issues/42").is_err()); + assert!(PrRef::parse("https://github.com/o/r/tree/main").is_err()); + } + + // ── GHE ───────────────────────────────────────────────────── + + #[test] + fn ghe_url_resolves_to_ghe_host_id() { + let r = PrRef::parse("https://ghe.example.com/team/proj/pull/7").unwrap(); + assert_eq!(r.host_id, RepoHostId::ghe("ghe.example.com")); + assert_eq!(r.host_id.api_base, "https://ghe.example.com/api/v3"); + assert_eq!(r.owner, "team"); + assert_eq!(r.repo, "proj"); + assert_eq!(r.number, 7); + } + + #[test] + fn ghe_url_with_github_prefix_classifies_as_github() { + // `github.acme.io` heuristic — see `RepoHostId::classify_host`. + let r = PrRef::parse("https://github.acme.io/o/r/pull/1").unwrap(); + assert_eq!(r.host_id.kind, RepoHostKind::GitHub); + assert_eq!(r.host_id.host, "github.acme.io"); + } + + // ── Azure DevOps ─────────────────────────────────────────── + + #[test] + fn ado_modern_url_collapses_org_project_into_owner() { + let r = + PrRef::parse("https://dev.azure.com/contoso/widgets/_git/api/pullrequest/99").unwrap(); + assert_eq!(r.host_id, RepoHostId::azure_devops()); + assert_eq!(r.owner, "contoso/widgets"); + assert_eq!(r.repo, "api"); + assert_eq!(r.number, 99); + } + + #[test] + fn ado_modern_url_rejects_wrong_segments() { + // Missing `_git`. + assert!(PrRef::parse("https://dev.azure.com/c/w/api/pullrequest/1").is_err()); + // Wrong literal in pullrequest slot. + assert!(PrRef::parse("https://dev.azure.com/c/w/_git/api/pulls/1").is_err()); + } + + #[test] + fn ado_legacy_visualstudio_url_pulls_org_from_host() { + let r = PrRef::parse( + "https://contoso.visualstudio.com/widgets/_git/api/pullrequest/77", + ) + .unwrap(); + assert_eq!(r.host_id, RepoHostId::azure_devops()); + assert_eq!(r.owner, "contoso/widgets"); + assert_eq!(r.repo, "api"); + assert_eq!(r.number, 77); + } + + #[test] + fn ado_legacy_url_rejects_missing_git_segment() { + assert!( + PrRef::parse("https://contoso.visualstudio.com/widgets/api/pullrequest/1").is_err() + ); + } + + // ── Unknown hosts ─────────────────────────────────────────── + + #[test] + fn unknown_host_is_rejected() { + assert!(PrRef::parse("https://gitlab.com/o/r/-/merge_requests/1").is_err()); + assert!(PrRef::parse("https://bitbucket.org/o/r/pull-requests/1").is_err()); + } + + // ── Display ──────────────────────────────────────────────── + + #[test] + fn display_omits_host_for_log_stability() { + let r = PrRef::parse("https://ghe.example.com/o/r/pull/1").unwrap(); + assert_eq!(format!("{r}"), "o/r#1"); + } +} diff --git a/crates/devdev-tasks/src/repo_watch.rs b/crates/devdev-tasks/src/repo_watch.rs index e15bb44..679cde7 100644 --- a/crates/devdev-tasks/src/repo_watch.rs +++ b/crates/devdev-tasks/src/repo_watch.rs @@ -2,7 +2,7 @@ //! //! The polling counterpart to a webhook receiver. Each tick: //! -//! 1. List open PRs via the [`GitHubAdapter`]. +//! 1. List open PRs via the [`RepoHostAdapter`]. //! 2. For each PR, compute a state hash (head_sha + updated_at) and //! consult the [`IdempotencyLedger`]. If we've seen this exact //! state, skip — we already published the corresponding event. @@ -20,18 +20,19 @@ use std::collections::HashMap; use std::sync::Arc; use std::time::{Duration, Instant}; -use devdev_integrations::{GitHubAdapter, pr_state_hash}; +use devdev_integrations::host::RepoHostId; +use devdev_integrations::{RepoHostAdapter, pr_state_hash}; use crate::events::{DaemonEvent, EventBus}; use crate::ledger::{IdempotencyLedger, LedgerKey}; use crate::task::{Task, TaskError, TaskMessage, TaskStatus}; -const ADAPTER: &str = "github"; const RESOURCE_TYPE: &str = "pr_state"; /// A task that polls a single repo's open PRs and emits events. pub struct RepoWatchTask { id: String, + host_id: RepoHostId, owner: String, repo: String, /// `pr_number → state_hash` for the most recent poll. @@ -40,7 +41,7 @@ pub struct RepoWatchTask { /// When `poll()` last actually ran. `None` until the first run. last_polled: Option, status: TaskStatus, - github: Arc, + github: Arc, ledger: Arc, bus: EventBus, } @@ -48,14 +49,16 @@ pub struct RepoWatchTask { impl RepoWatchTask { pub fn new( id: String, + host_id: RepoHostId, owner: impl Into, repo: impl Into, - github: Arc, + github: Arc, ledger: Arc, bus: EventBus, ) -> Self { Self { id, + host_id, owner: owner.into(), repo: repo.into(), last_seen: HashMap::new(), @@ -73,6 +76,10 @@ impl RepoWatchTask { self } + pub fn host_id(&self) -> &RepoHostId { + &self.host_id + } + pub fn owner(&self) -> &str { &self.owner } @@ -99,7 +106,12 @@ impl RepoWatchTask { let hash = pr_state_hash(pr); current.insert(pr.number, hash.clone()); - let key = LedgerKey::new(ADAPTER, RESOURCE_TYPE, self.resource_id(pr.number), &hash); + let key = LedgerKey::new( + self.host_id.ledger_key(), + RESOURCE_TYPE, + self.resource_id(pr.number), + &hash, + ); // Two independent questions: // 1. Have we already published this exact state hash? @@ -125,6 +137,7 @@ impl RepoWatchTask { let event = if first_in_session { DaemonEvent::PrOpened { + host_id: self.host_id.clone(), owner: self.owner.clone(), repo: self.repo.clone(), number: pr.number, @@ -132,6 +145,7 @@ impl RepoWatchTask { } } else { DaemonEvent::PrUpdated { + host_id: self.host_id.clone(), owner: self.owner.clone(), repo: self.repo.clone(), number: pr.number, @@ -171,6 +185,7 @@ impl RepoWatchTask { // (mergeable=false) and let MonitorPrTask resolve via // its own get_pr_status if it cares. let event = DaemonEvent::PrClosed { + host_id: self.host_id.clone(), owner: self.owner.clone(), repo: self.repo.clone(), number: *number, @@ -197,7 +212,12 @@ impl Task for RepoWatchTask { } fn describe(&self) -> String { - format!("Watching {}/{} for PR events", self.owner, self.repo) + format!( + "Watching {}/{} for PR events ({})", + self.owner, + self.repo, + self.host_id.ledger_key() + ) } fn status(&self) -> &TaskStatus { @@ -226,6 +246,7 @@ impl Task for RepoWatchTask { fn serialize(&self) -> Result { Ok(serde_json::json!({ "id": self.id, + "host": self.host_id.ledger_key(), "owner": self.owner, "repo": self.repo, "last_seen": self.last_seen, @@ -245,7 +266,7 @@ impl Task for RepoWatchTask { #[cfg(test)] mod tests { use super::*; - use devdev_integrations::{MockGitHubAdapter, PrState, PullRequest}; + use devdev_integrations::{MockAdapter, PrState, PullRequest}; /// In-memory test ledger. #[derive(Default)] @@ -287,18 +308,19 @@ mod tests { fn task() -> ( RepoWatchTask, - Arc, + Arc, Arc, EventBus, ) { - let gh = Arc::new(MockGitHubAdapter::new()); + let gh = Arc::new(MockAdapter::new()); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let t = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh.clone() as Arc, + gh.clone() as Arc, ledger.clone() as Arc, bus.clone(), ) @@ -319,15 +341,16 @@ mod tests { #[tokio::test] async fn first_pr_emits_opened() { - let gh = Arc::new(MockGitHubAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); + let gh = Arc::new(MockAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let mut rx = bus.subscribe(); let mut t = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh as Arc, + gh as Arc, ledger as Arc, bus, ) @@ -339,15 +362,16 @@ mod tests { #[tokio::test] async fn second_poll_no_change_emits_nothing() { - let gh = Arc::new(MockGitHubAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); + let gh = Arc::new(MockAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let mut rx = bus.subscribe(); let mut t = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh as Arc, + gh as Arc, ledger as Arc, bus, ) @@ -361,15 +385,16 @@ mod tests { #[tokio::test] async fn updated_pr_emits_pr_updated() { - let gh = Arc::new(MockGitHubAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); + let gh = Arc::new(MockAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let mut rx = bus.subscribe(); let mut t = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh.clone() as Arc, + gh.clone() as Arc, ledger as Arc, bus, ) @@ -386,15 +411,16 @@ mod tests { #[tokio::test] async fn closed_pr_emits_pr_closed() { // Two adapters: one with PR open, one without. - let gh_open = Arc::new(MockGitHubAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); + let gh_open = Arc::new(MockAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let mut rx = bus.subscribe(); let mut t = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh_open as Arc, + gh_open as Arc, ledger.clone() as Arc, bus.clone(), ) @@ -403,7 +429,7 @@ mod tests { let _ = rx.recv().await.unwrap(); // Replace adapter with empty one (PR disappeared). - let gh_empty = Arc::new(MockGitHubAdapter::new()); + let gh_empty = Arc::new(MockAdapter::new()); t.github = gh_empty; t.poll().await.unwrap(); let evt = rx.recv().await.unwrap(); @@ -412,7 +438,7 @@ mod tests { #[tokio::test] async fn ledger_dedups_across_restart() { - let gh = Arc::new(MockGitHubAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); + let gh = Arc::new(MockAdapter::new().with_pr("o", "r", pr(1, "sha1", "t1"))); let ledger = Arc::new(MemLedger::default()); let bus = EventBus::new(); let mut rx = bus.subscribe(); @@ -420,9 +446,10 @@ mod tests { // First task instance: emits PrOpened, records ledger. let mut t1 = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh.clone() as Arc, + gh.clone() as Arc, ledger.clone() as Arc, bus.clone(), ) @@ -437,9 +464,10 @@ mod tests { // even though the ledger already has the state hash. let mut t2 = RepoWatchTask::new( "t-1".into(), + RepoHostId::github_com(), "o", "r", - gh.clone() as Arc, + gh.clone() as Arc, ledger.clone() as Arc, bus, ) diff --git a/crates/devdev-tasks/tests/acceptance_monitor_pr.rs b/crates/devdev-tasks/tests/acceptance_monitor_pr.rs index 4acc1be..8f41651 100644 --- a/crates/devdev-tasks/tests/acceptance_monitor_pr.rs +++ b/crates/devdev-tasks/tests/acceptance_monitor_pr.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use async_trait::async_trait; -use devdev_integrations::{MockGitHubAdapter, PrState, PrStatus, PullRequest}; +use devdev_integrations::{MockAdapter, PrState, PrStatus, PullRequest}; use devdev_tasks::agent::AgentRunner; use devdev_tasks::events::{DaemonEvent, EventBus}; use devdev_tasks::monitor_pr::MonitorPrTask; @@ -27,8 +27,8 @@ fn mock_pr(number: u64, sha: &str) -> PullRequest { } } -fn mock_github(sha: &str) -> MockGitHubAdapter { - MockGitHubAdapter::new() +fn mock_github(sha: &str) -> MockAdapter { + MockAdapter::new() .with_pr("org", "repo", mock_pr(247, sha)) .with_diff( "org", @@ -102,7 +102,7 @@ fn parse_structured_review() { #[tokio::test] async fn idle_with_no_events_is_quiet() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner: Arc = Arc::new(FakeRunner::default()); let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner, &bus).unwrap(); @@ -113,13 +113,14 @@ async fn idle_with_no_events_is_quiet() { #[tokio::test] async fn pr_opened_event_triggers_agent_prompt() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner = Arc::new(FakeRunner::default()); let runner_dyn: Arc = runner.clone(); let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner_dyn, &bus).unwrap(); bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -136,13 +137,14 @@ async fn pr_opened_event_triggers_agent_prompt() { #[tokio::test] async fn pr_updated_event_triggers_agent_prompt() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner = Arc::new(FakeRunner::default()); let runner_dyn: Arc = runner.clone(); let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner_dyn, &bus).unwrap(); bus.publish(DaemonEvent::PrUpdated { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -157,12 +159,13 @@ async fn pr_updated_event_triggers_agent_prompt() { #[tokio::test] async fn pr_closed_event_completes_task() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner: Arc = Arc::new(FakeRunner::default()); let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner, &bus).unwrap(); bus.publish(DaemonEvent::PrClosed { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -176,7 +179,7 @@ async fn pr_closed_event_completes_task() { #[tokio::test] async fn non_matching_event_is_ignored() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner = Arc::new(FakeRunner::default()); let runner_dyn: Arc = runner.clone(); @@ -184,6 +187,7 @@ async fn non_matching_event_is_ignored() { // Different PR number. bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 999, @@ -197,12 +201,13 @@ async fn non_matching_event_is_ignored() { #[tokio::test] async fn observations_accumulate_across_events() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner: Arc = Arc::new(FakeRunner::default()); let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner, &bus).unwrap(); bus.publish(DaemonEvent::PrOpened { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -211,6 +216,7 @@ async fn observations_accumulate_across_events() { task.poll().await.unwrap(); bus.publish(DaemonEvent::PrUpdated { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -225,8 +231,8 @@ async fn observations_accumulate_across_events() { async fn merged_pr_short_circuits_to_completed() { let mut pr = mock_pr(247, "abc123"); pr.state = PrState::Merged; - let gh: Arc = Arc::new( - MockGitHubAdapter::new() + let gh: Arc = Arc::new( + MockAdapter::new() .with_pr("org", "repo", pr) .with_diff("org", "repo", 247, ""), ); @@ -235,6 +241,7 @@ async fn merged_pr_short_circuits_to_completed() { let mut task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner, &bus).unwrap(); bus.publish(DaemonEvent::PrUpdated { + host_id: devdev_integrations::host::RepoHostId::github_com(), owner: "org".into(), repo: "repo".into(), number: 247, @@ -246,7 +253,7 @@ async fn merged_pr_short_circuits_to_completed() { #[tokio::test] async fn serialize_includes_pr_state() { - let gh: Arc = Arc::new(mock_github("abc123")); + let gh: Arc = Arc::new(mock_github("abc123")); let bus = EventBus::new(); let runner: Arc = Arc::new(FakeRunner::default()); let task = MonitorPrTask::new("t-1".into(), "org/repo#247", gh, runner, &bus).unwrap(); diff --git a/crates/devdev-test-env/Cargo.toml b/crates/devdev-test-env/Cargo.toml new file mode 100644 index 0000000..5609ceb --- /dev/null +++ b/crates/devdev-test-env/Cargo.toml @@ -0,0 +1,38 @@ +[package] +name = "devdev-test-env" +version.workspace = true +edition.workspace = true +license.workspace = true +repository.workspace = true +authors.workspace = true +publish = false + +description = """ +Provisions and resets the live-test environment fixtures (a github.com +org/repo/canonical PR and an Azure DevOps org/project/repo/canonical PR). +The `devdev-test-env` binary is intended to run from the live-tests CI +workflow with admin credentials it stages for the lower-privilege test +binaries to consume via env vars. +""" + +[lib] +path = "src/lib.rs" + +[[bin]] +name = "devdev-test-env" +path = "src/main.rs" + +[dependencies] +anyhow.workspace = true +base64.workspace = true +clap = { workspace = true, features = ["env"] } +reqwest = { workspace = true, features = ["rustls-tls"] } +serde.workspace = true +serde_json.workspace = true +thiserror.workspace = true +tokio.workspace = true +tracing.workspace = true +tracing-subscriber.workspace = true + +[dev-dependencies] +tempfile.workspace = true diff --git a/crates/devdev-test-env/src/ado.rs b/crates/devdev-test-env/src/ado.rs new file mode 100644 index 0000000..646eb09 --- /dev/null +++ b/crates/devdev-test-env/src/ado.rs @@ -0,0 +1,366 @@ +//! Azure DevOps fixture provisioner. +//! +//! Surface mirrors `github.rs`: idempotent `apply` / `verify` and a +//! `list_pr_threads` / `delete_thread_comment` pair for +//! `reset-comments`. ADO uses HTTP Basic auth with `base64(":")` +//! exactly the way our `RepoHostAdapter` does. +//! +//! ADO REST is more verbose than GitHub's: branches don't exist as +//! a first-class resource (you push commits to refs); PR comments +//! live inside threads. Both quirks are abstracted away here so the +//! main binary sees a uniform interface. + +use std::time::Duration; + +use base64::Engine as _; +use base64::engine::general_purpose::STANDARD as B64; +use reqwest::Client; +use reqwest::header::{ACCEPT, AUTHORIZATION, CONTENT_TYPE, USER_AGENT}; +use serde::Deserialize; +use serde_json::{Value, json}; + +use crate::manifest::{AdoFixture, AdoLock, CanonicalPr}; +use crate::secret::Token; + +const API_VERSION: &str = "7.1"; +const UA: &str = "devdev-test-env/0.1"; + +/// Authenticated client for the Azure DevOps REST API. +/// +/// SECURITY: deliberately does **not** derive `Debug`. The pre-built +/// `Authorization: Basic ...` header is wrapped in [`Token`] so that +/// even if someone adds Debug later, the value won't leak through +/// `{:?}`. +pub struct AdoClient { + http: Client, + auth_header: Token, +} + +impl AdoClient { + pub fn new(pat: &str) -> anyhow::Result { + let basic = format!("Basic {}", B64.encode(format!(":{pat}").as_bytes())); + let http = Client::builder() + .user_agent(UA) + .timeout(Duration::from_secs(30)) + .build()?; + Ok(Self { + http, + auth_header: Token::new(basic), + }) + } + + fn auth(&self, req: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + req.header(AUTHORIZATION, self.auth_header.expose()) + .header(ACCEPT, "application/json") + .header(USER_AGENT, UA) + } + + fn org_root(&self, fixture: &AdoFixture) -> String { + format!( + "https://dev.azure.com/{}/{}/_apis", + fixture.org, fixture.project + ) + } + + /// Apply the manifest's ADO side. Project + org are asserted to + /// already exist; repo is created if missing; canonical PR is + /// opened if not already open. + pub async fn apply(&self, fixture: &AdoFixture) -> anyhow::Result { + let repo_id = self.ensure_repo(fixture).await?; + // Seeding the fixture branch via REST `pushes` is significantly + // hairier than github's `contents` API: ADO requires building + // a tree object and pushing a commit. For the first cut we + // require the fixture branch to exist (manual one-time push) + // and limit `apply` to PR-level idempotency. Verified by + // attempting `branch_head_sha`; if missing, `apply` fails with + // a directive pointing at the bootstrap doc. + let head_sha = self + .branch_head_sha(fixture, &repo_id, &fixture.fixture_branch) + .await?; + let _ = self + .branch_head_sha(fixture, &repo_id, &fixture.default_branch) + .await?; + let pr_id = self + .ensure_canonical_pr(fixture, &repo_id, &head_sha) + .await?; + Ok(AdoLock { + repo_id, + canonical_pr_id: pr_id, + }) + } + + pub async fn verify( + &self, + fixture: &AdoFixture, + lock: &AdoLock, + ) -> anyhow::Result<()> { + let url = format!( + "{}/git/repositories/{}/pullrequests/{}?api-version={API_VERSION}", + self.org_root(fixture), + lock.repo_id, + lock.canonical_pr_id + ); + let pr: PrResponse = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + if pr.title != fixture.canonical_pr.title { + anyhow::bail!( + "ado canonical PR title drifted: manifest={:?}, live={:?}", + fixture.canonical_pr.title, + pr.title + ); + } + if pr.description.as_deref().unwrap_or("") != fixture.canonical_pr.body { + anyhow::bail!("ado canonical PR description drifted from manifest"); + } + if pr.status != "active" { + anyhow::bail!("ado canonical PR is not active: status={}", pr.status); + } + Ok(()) + } + + async fn ensure_repo(&self, fixture: &AdoFixture) -> anyhow::Result { + let list_url = format!( + "{}/git/repositories?api-version={API_VERSION}", + self.org_root(fixture) + ); + let v: ListRepos = self + .auth(self.http.get(&list_url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + if let Some(r) = v.value.iter().find(|r| r.name == fixture.repo) { + return Ok(r.id.clone()); + } + let create: RepoResponse = self + .auth(self.http.post(&list_url)) + .header(CONTENT_TYPE, "application/json") + .json(&json!({ "name": fixture.repo })) + .send() + .await? + .error_for_status()? + .json() + .await?; + Ok(create.id) + } + + async fn branch_head_sha( + &self, + fixture: &AdoFixture, + repo_id: &str, + branch: &str, + ) -> anyhow::Result { + let url = format!( + "{}/git/repositories/{}/refs?filter=heads/{}&api-version={API_VERSION}", + self.org_root(fixture), + repo_id, + branch + ); + let v: ListRefs = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + v.value + .into_iter() + .find(|r| r.name == format!("refs/heads/{branch}")) + .map(|r| r.object_id) + .ok_or_else(|| { + anyhow::anyhow!( + "ado branch heads/{branch} not found in repo {repo_id}; \ + bootstrap by pushing the fixture branch manually \ + (see docs/internals/live-test-fixtures.md)" + ) + }) + } + + async fn ensure_canonical_pr( + &self, + fixture: &AdoFixture, + repo_id: &str, + _head_sha: &str, + ) -> anyhow::Result { + let list_url = format!( + "{}/git/repositories/{}/pullrequests?searchCriteria.status=active&searchCriteria.sourceRefName=refs/heads/{}&api-version={API_VERSION}", + self.org_root(fixture), + repo_id, + fixture.fixture_branch, + ); + let list: ListPrs = self + .auth(self.http.get(&list_url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + if let Some(pr) = list + .value + .into_iter() + .find(|p| p.title == fixture.canonical_pr.title) + { + // Description drift fixed here. + if pr.description.as_deref().unwrap_or("") != fixture.canonical_pr.body { + self.auth( + self.http.patch(format!( + "{}/git/repositories/{}/pullrequests/{}?api-version={API_VERSION}", + self.org_root(fixture), + repo_id, + pr.pull_request_id, + )), + ) + .header(CONTENT_TYPE, "application/json") + .json(&json!({ "description": fixture.canonical_pr.body })) + .send() + .await? + .error_for_status()?; + } + return Ok(pr.pull_request_id); + } + let body = pr_create_body(fixture, &fixture.canonical_pr); + let create_url = format!( + "{}/git/repositories/{}/pullrequests?api-version={API_VERSION}", + self.org_root(fixture), + repo_id + ); + let pr: PrResponse = self + .auth(self.http.post(&create_url)) + .header(CONTENT_TYPE, "application/json") + .json(&body) + .send() + .await? + .error_for_status()? + .json() + .await?; + Ok(pr.pull_request_id) + } + + pub async fn list_pr_threads( + &self, + fixture: &AdoFixture, + repo_id: &str, + pr_id: u64, + ) -> anyhow::Result> { + let url = format!( + "{}/git/repositories/{}/pullrequests/{}/threads?api-version={API_VERSION}", + self.org_root(fixture), + repo_id, + pr_id + ); + let v: ListThreads = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + Ok(v.value) + } + + pub async fn delete_thread_comment( + &self, + fixture: &AdoFixture, + repo_id: &str, + pr_id: u64, + thread_id: u64, + comment_id: u64, + ) -> anyhow::Result<()> { + let url = format!( + "{}/git/repositories/{}/pullrequests/{}/threads/{}/comments/{}?api-version={API_VERSION}", + self.org_root(fixture), + repo_id, + pr_id, + thread_id, + comment_id, + ); + self.auth(self.http.delete(&url)) + .send() + .await? + .error_for_status()?; + Ok(()) + } +} + +fn pr_create_body(fixture: &AdoFixture, canonical: &CanonicalPr) -> Value { + json!({ + "sourceRefName": format!("refs/heads/{}", fixture.fixture_branch), + "targetRefName": format!("refs/heads/{}", canonical.base), + "title": canonical.title, + "description": canonical.body, + }) +} + +#[derive(Debug, Deserialize)] +struct ListRepos { + value: Vec, +} + +#[derive(Debug, Deserialize)] +struct RepoResponse { + id: String, + name: String, +} + +#[derive(Debug, Deserialize)] +struct ListRefs { + value: Vec, +} + +#[derive(Debug, Deserialize)] +struct RefResponse { + name: String, + #[serde(rename = "objectId")] + object_id: String, +} + +#[derive(Debug, Deserialize)] +struct ListPrs { + value: Vec, +} + +#[derive(Debug, Deserialize)] +struct PrResponse { + #[serde(rename = "pullRequestId")] + pull_request_id: u64, + title: String, + status: String, + #[serde(default)] + description: Option, +} + +#[derive(Debug, Deserialize)] +struct ListThreads { + value: Vec, +} + +#[derive(Debug, Deserialize)] +pub struct PrThread { + pub id: u64, + #[serde(default)] + pub comments: Vec, + /// `active`, `fixed`, `closed`, etc. We delete comments inside + /// active threads only — closed threads are usually historical. + #[serde(default)] + pub status: Option, +} + +#[derive(Debug, Deserialize)] +pub struct ThreadComment { + pub id: u64, + #[serde(default)] + pub content: String, + pub author: ThreadCommentAuthor, +} + +#[derive(Debug, Deserialize)] +pub struct ThreadCommentAuthor { + #[serde(default, rename = "uniqueName")] + pub unique_name: String, +} diff --git a/crates/devdev-test-env/src/github.rs b/crates/devdev-test-env/src/github.rs new file mode 100644 index 0000000..12c9f51 --- /dev/null +++ b/crates/devdev-test-env/src/github.rs @@ -0,0 +1,369 @@ +//! GitHub fixture provisioner. +//! +//! Implements the idempotent `apply` / `verify` / `reset-comments` +//! ops for the GitHub side of the manifest. Talks to the github.com +//! REST API directly (the `octocrab` crate would pull a non-trivial +//! dep tree for not much benefit at this scope). +//! +//! Authentication: a GitHub fine-grained PAT with `Contents: write`, +//! `Pull requests: write`, `Issues: write`, and `Administration: +//! write` (last only needed if the repo doesn't yet exist) on the +//! fixture org. + +use std::time::Duration; + +use base64::Engine as _; +use base64::engine::general_purpose::STANDARD as B64; +use reqwest::Client; +use reqwest::header::{ACCEPT, AUTHORIZATION, USER_AGENT}; +use serde::{Deserialize, Serialize}; +use serde_json::{Value, json}; + +use crate::manifest::{CanonicalPr, GithubFixture, GithubLock}; +use crate::secret::Token; + +const API_BASE: &str = "https://api.github.com"; +const UA: &str = "devdev-test-env/0.1"; + +/// Authenticated client for the GitHub REST API. Each method is +/// idempotent: it reads first, only writes if state diverges. +/// +/// SECURITY: deliberately does **not** derive `Debug`. The token is +/// further wrapped in [`Token`] so even if someone adds Debug later, +/// the value won't leak through `{:?}`. +pub struct GithubClient { + http: Client, + token: Token, +} + +impl GithubClient { + pub fn new(token: String) -> anyhow::Result { + let http = Client::builder() + .user_agent(UA) + .timeout(Duration::from_secs(30)) + .build()?; + Ok(Self { + http, + token: Token::new(token), + }) + } + + fn auth(&self, req: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + req.header(AUTHORIZATION, format!("Bearer {}", self.token.expose())) + .header(ACCEPT, "application/vnd.github+json") + .header(USER_AGENT, UA) + .header("X-GitHub-Api-Version", "2022-11-28") + } + + /// Apply the manifest's GitHub side. Returns the resolved + /// [`GithubLock`] (PR number etc.). + pub async fn apply(&self, fixture: &GithubFixture) -> anyhow::Result { + let repo = self.ensure_repo(&fixture.org, &fixture.repo).await?; + let default_sha = self + .branch_head_sha(&fixture.org, &fixture.repo, &fixture.default_branch) + .await?; + self.ensure_fixture_branch( + &fixture.org, + &fixture.repo, + &fixture.fixture_branch, + &default_sha, + ) + .await?; + self.ensure_fixture_files( + &fixture.org, + &fixture.repo, + &fixture.fixture_branch, + &fixture.canonical_pr, + ) + .await?; + let (pr_number, pr_node_id) = self + .ensure_canonical_pr( + &fixture.org, + &fixture.repo, + &fixture.fixture_branch, + &fixture.canonical_pr, + ) + .await?; + + Ok(GithubLock { + repo_id: repo.id, + canonical_pr_number: pr_number, + canonical_pr_node_id: pr_node_id, + }) + } + + /// Read-only: returns Ok if the manifest matches reality byte- + /// for-byte. Non-zero exit otherwise. + pub async fn verify( + &self, + fixture: &GithubFixture, + lock: &GithubLock, + ) -> anyhow::Result<()> { + let pr: PrResponse = self + .auth(self.http.get(format!( + "{API_BASE}/repos/{}/{}/pulls/{}", + fixture.org, fixture.repo, lock.canonical_pr_number + ))) + .send() + .await? + .error_for_status()? + .json() + .await?; + if pr.title != fixture.canonical_pr.title { + anyhow::bail!( + "github canonical PR title drifted: manifest={:?}, live={:?}", + fixture.canonical_pr.title, + pr.title + ); + } + if pr.body.as_deref().unwrap_or("") != fixture.canonical_pr.body { + anyhow::bail!("github canonical PR body drifted from manifest"); + } + if pr.state != "open" { + anyhow::bail!("github canonical PR is not open: state={}", pr.state); + } + Ok(()) + } + + async fn ensure_repo(&self, org: &str, repo: &str) -> anyhow::Result { + let url = format!("{API_BASE}/repos/{org}/{repo}"); + let resp = self.auth(self.http.get(&url)).send().await?; + if resp.status().is_success() { + return Ok(resp.json().await?); + } + if resp.status().as_u16() != 404 { + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + anyhow::bail!("GET {url} returned {status}: {body}"); + } + // Create. + let create = self + .auth(self.http.post(format!("{API_BASE}/orgs/{org}/repos"))) + .json(&json!({ + "name": repo, + "private": false, + "auto_init": true, + "description": "DevDev live-test fixture; managed by devdev-test-env", + })) + .send() + .await? + .error_for_status()? + .json::() + .await?; + Ok(create) + } + + async fn branch_head_sha( + &self, + org: &str, + repo: &str, + branch: &str, + ) -> anyhow::Result { + let url = format!("{API_BASE}/repos/{org}/{repo}/git/ref/heads/{branch}"); + let v: Value = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + v["object"]["sha"] + .as_str() + .map(|s| s.to_string()) + .ok_or_else(|| anyhow::anyhow!("missing object.sha in {url}")) + } + + async fn ensure_fixture_branch( + &self, + org: &str, + repo: &str, + branch: &str, + from_sha: &str, + ) -> anyhow::Result<()> { + let url = format!("{API_BASE}/repos/{org}/{repo}/git/ref/heads/{branch}"); + let resp = self.auth(self.http.get(&url)).send().await?; + if resp.status().is_success() { + return Ok(()); + } + if resp.status().as_u16() != 404 { + anyhow::bail!("GET {url} returned {}", resp.status()); + } + self.auth(self.http.post(format!("{API_BASE}/repos/{org}/{repo}/git/refs"))) + .json(&json!({ "ref": format!("refs/heads/{branch}"), "sha": from_sha })) + .send() + .await? + .error_for_status()?; + Ok(()) + } + + async fn ensure_fixture_files( + &self, + org: &str, + repo: &str, + branch: &str, + canonical: &CanonicalPr, + ) -> anyhow::Result<()> { + for file in &canonical.fixture_files { + let path = &file.path; + let url = format!("{API_BASE}/repos/{org}/{repo}/contents/{path}?ref={branch}"); + let head = self.auth(self.http.get(&url)).send().await?; + let (existing_sha, existing_b64) = if head.status().is_success() { + let v: ContentsResponse = head.json().await?; + (Some(v.sha), v.content) + } else if head.status().as_u16() == 404 { + (None, String::new()) + } else { + anyhow::bail!("GET {url} returned {}", head.status()); + }; + + let want_b64 = B64.encode(file.contents.as_bytes()); + // GitHub returns content with newlines every 60 chars; normalise before compare. + if existing_b64.replace('\n', "") == want_b64 && existing_sha.is_some() { + continue; + } + + let mut body = json!({ + "message": format!("devdev-test-env: ensure fixture file {path}"), + "content": want_b64, + "branch": branch, + }); + if let Some(sha) = existing_sha { + body["sha"] = Value::String(sha); + } + self.auth( + self.http + .put(format!("{API_BASE}/repos/{org}/{repo}/contents/{path}")), + ) + .json(&body) + .send() + .await? + .error_for_status()?; + } + Ok(()) + } + + async fn ensure_canonical_pr( + &self, + org: &str, + repo: &str, + head: &str, + canonical: &CanonicalPr, + ) -> anyhow::Result<(u64, String)> { + // List open PRs with matching head ref. + let url = format!( + "{API_BASE}/repos/{org}/{repo}/pulls?state=open&head={org}:{head}&per_page=10" + ); + let prs: Vec = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + + if let Some(pr) = prs.into_iter().find(|p| p.title == canonical.title) { + // Body drift is fixed up here, not a verify failure. + if pr.body.as_deref().unwrap_or("") != canonical.body { + self.auth(self.http.patch(format!( + "{API_BASE}/repos/{org}/{repo}/pulls/{}", + pr.number + ))) + .json(&json!({ "body": canonical.body })) + .send() + .await? + .error_for_status()?; + } + return Ok((pr.number, pr.node_id)); + } + + // Create. + let pr: PrResponse = self + .auth( + self.http + .post(format!("{API_BASE}/repos/{org}/{repo}/pulls")), + ) + .json(&json!({ + "title": canonical.title, + "body": canonical.body, + "head": head, + "base": canonical.base, + })) + .send() + .await? + .error_for_status()? + .json() + .await?; + Ok((pr.number, pr.node_id)) + } + + /// List comment ids on the canonical PR. Used by `reset-comments`. + pub async fn list_pr_comments( + &self, + org: &str, + repo: &str, + pr_number: u64, + ) -> anyhow::Result> { + let url = format!("{API_BASE}/repos/{org}/{repo}/issues/{pr_number}/comments?per_page=100"); + let v: Vec = self + .auth(self.http.get(&url)) + .send() + .await? + .error_for_status()? + .json() + .await?; + Ok(v) + } + + pub async fn delete_issue_comment( + &self, + org: &str, + repo: &str, + comment_id: u64, + ) -> anyhow::Result<()> { + self.auth(self.http.delete(format!( + "{API_BASE}/repos/{org}/{repo}/issues/comments/{comment_id}" + ))) + .send() + .await? + .error_for_status()?; + Ok(()) + } +} + +#[derive(Debug, Deserialize, Serialize)] +pub struct RepoResponse { + pub id: u64, + #[serde(default)] + pub name: String, +} + +#[derive(Debug, Deserialize, Serialize)] +struct PrResponse { + number: u64, + node_id: String, + title: String, + state: String, + #[serde(default)] + body: Option, +} + +#[derive(Debug, Deserialize)] +struct ContentsResponse { + sha: String, + #[serde(default)] + content: String, +} + +/// Issue comment shape. The id and author login are all `reset` needs. +#[derive(Debug, Deserialize)] +pub struct IssueComment { + pub id: u64, + #[serde(default)] + pub body: String, + pub user: CommentUser, +} + +#[derive(Debug, Deserialize)] +pub struct CommentUser { + pub login: String, +} diff --git a/crates/devdev-test-env/src/lib.rs b/crates/devdev-test-env/src/lib.rs new file mode 100644 index 0000000..6dc026b --- /dev/null +++ b/crates/devdev-test-env/src/lib.rs @@ -0,0 +1,31 @@ +//! `devdev-test-env` — provisioner for the live-test fixture environment. +//! +//! The crate exposes a small library and a binary with five +//! subcommands (`apply`, `verify`, `reset-comments`, `destroy`, +//! `print-env`). The library is the place real provisioning logic +//! lives so it can be unit-tested without an HTTP round-trip — the +//! GitHub and ADO REST clients are pluggable via the +//! [`HostClient`] traits below. +//! +//! ## Design choices +//! +//! - **JSON manifest, not Terraform.** The manifest is a plain +//! serde struct; `manifest.lock.json` carries server-assigned +//! ids (PR numbers) backfilled after the first `apply`. +//! - **Hand-rolled REST.** Avoids dragging in `octocrab` / Azure +//! SDK crates and the dependency-update friction they bring. +//! - **Idempotent by construction.** Every `ensure_*` operation +//! reads first, then writes only if state diverges. A second +//! `apply` on a clean fixture is a no-op (asserted in tests). +//! - **Admin credentials never reach test code.** The binary uses +//! them; tests consume the env vars `print-env` emits, which +//! reference *separate* lower-privilege tokens. + +pub mod ado; +pub mod github; +pub mod manifest; +pub mod reset; +pub mod secret; + +pub use manifest::{AdoFixture, GithubFixture, Manifest, ManifestLock}; +pub use secret::Token; diff --git a/crates/devdev-test-env/src/main.rs b/crates/devdev-test-env/src/main.rs new file mode 100644 index 0000000..b5e9794 --- /dev/null +++ b/crates/devdev-test-env/src/main.rs @@ -0,0 +1,249 @@ +//! `devdev-test-env` binary entry-point. +//! +//! Five subcommands, all driven from a single committed manifest + +//! lock file pair under `test-env/`. Admin tokens come from the +//! environment; we never write them anywhere. +//! +//! ```text +//! devdev-test-env apply +//! devdev-test-env verify +//! devdev-test-env reset-comments --admin-github-login=... +//! --admin-ado-name=... +//! devdev-test-env destroy --yes-really +//! devdev-test-env print-env +//! ``` + +use std::path::{Path, PathBuf}; + +use clap::{Parser, Subcommand}; +use devdev_test_env::manifest::{Manifest, ManifestLock}; +use devdev_test_env::{ado, github, reset}; + +#[derive(Parser, Debug, Clone)] +#[command(name = "devdev-test-env", about = "Provision DevDev live-test fixtures")] +struct Cli { + /// Manifest path. Defaults to `test-env/manifest.json` relative to cwd. + #[arg(long, global = true, default_value = "test-env/manifest.json")] + manifest: PathBuf, + /// Lock file path. Defaults to `test-env/manifest.lock.json`. + #[arg(long, global = true, default_value = "test-env/manifest.lock.json")] + lock: PathBuf, + /// Skip the GitHub side (only act on ADO). + #[arg(long, global = true)] + skip_github: bool, + /// Skip the ADO side (only act on GitHub). + #[arg(long, global = true)] + skip_ado: bool, + #[command(subcommand)] + cmd: Command, +} + +#[derive(Subcommand, Debug, Clone)] +enum Command { + /// Provision (or reconcile) fixtures to match the manifest. + Apply, + /// Read-only check that fixtures match the manifest. + Verify, + /// Sweep stray comments off the canonical PRs. + ResetComments { + /// GitHub login of the admin identity (used to decide which + /// comments are admin-pinned vs test-issued). + #[arg(long, env = "DEVDEV_TEST_ENV_GITHUB_ADMIN_LOGIN")] + admin_github_login: String, + /// ADO `uniqueName` (usually an email) of the admin identity. + #[arg(long, env = "DEVDEV_TEST_ENV_ADO_ADMIN_NAME")] + admin_ado_name: String, + }, + /// Tear down fixtures. Disabled unless `--yes-really` is set. + Destroy { + #[arg(long)] + yes_really: bool, + }, + /// Emit the env-var block downstream test runners should consume. + PrintEnv, +} + +#[tokio::main] +async fn main() -> anyhow::Result<()> { + tracing_subscriber::fmt() + .with_env_filter( + tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")), + ) + .with_target(false) + .init(); + + let cli = Cli::parse(); + match cli.cmd.clone() { + Command::Apply => apply(&cli).await, + Command::Verify => verify(&cli).await, + Command::ResetComments { + admin_github_login, + admin_ado_name, + } => reset_comments(&cli, &admin_github_login, &admin_ado_name).await, + Command::Destroy { yes_really } => destroy(&cli, yes_really).await, + Command::PrintEnv => print_env(&cli).await, + } +} + +fn read_manifest(path: &Path) -> anyhow::Result { + Manifest::read(path) +} + +fn read_lock(path: &Path) -> anyhow::Result { + ManifestLock::read_or_default(path) +} + +async fn apply(cli: &Cli) -> anyhow::Result<()> { + let manifest = read_manifest(&cli.manifest)?; + let mut lock = read_lock(&cli.lock)?; + + if !cli.skip_github { + let token = std::env::var("GITHUB_TOKEN_ADMIN").map_err(|_| { + anyhow::anyhow!("GITHUB_TOKEN_ADMIN must be set for `apply --skip-github=false`") + })?; + let client = github::GithubClient::new(token)?; + let resolved = client.apply(&manifest.github).await?; + tracing::info!( + org = %manifest.github.org, + repo = %manifest.github.repo, + pr = resolved.canonical_pr_number, + "github fixture applied", + ); + lock.github = Some(resolved); + } + + if !cli.skip_ado { + let pat = std::env::var("ADO_PAT_ADMIN").map_err(|_| { + anyhow::anyhow!("ADO_PAT_ADMIN must be set for `apply --skip-ado=false`") + })?; + let client = ado::AdoClient::new(&pat)?; + let resolved = client.apply(&manifest.azure_devops).await?; + tracing::info!( + org = %manifest.azure_devops.org, + project = %manifest.azure_devops.project, + repo = %manifest.azure_devops.repo, + pr = resolved.canonical_pr_id, + "ado fixture applied", + ); + lock.azure_devops = Some(resolved); + } + + lock.write(&cli.lock)?; + println!("OK: lock written to {}", cli.lock.display()); + Ok(()) +} + +async fn verify(cli: &Cli) -> anyhow::Result<()> { + let manifest = read_manifest(&cli.manifest)?; + let lock = read_lock(&cli.lock)?; + + if !cli.skip_github { + let lock_gh = lock + .github + .as_ref() + .ok_or_else(|| anyhow::anyhow!("no github lock entry; run `apply` first"))?; + let token = std::env::var("GITHUB_TOKEN_ADMIN") + .or_else(|_| std::env::var("GITHUB_TOKEN")) + .map_err(|_| { + anyhow::anyhow!("GITHUB_TOKEN_ADMIN or GITHUB_TOKEN must be set for verify") + })?; + let client = github::GithubClient::new(token)?; + client.verify(&manifest.github, lock_gh).await?; + println!("OK: github fixture matches manifest"); + } + + if !cli.skip_ado { + let lock_ado = lock + .azure_devops + .as_ref() + .ok_or_else(|| anyhow::anyhow!("no ado lock entry; run `apply` first"))?; + let pat = std::env::var("ADO_PAT_ADMIN") + .or_else(|_| std::env::var("ADO_PAT")) + .map_err(|_| anyhow::anyhow!("ADO_PAT_ADMIN or ADO_PAT must be set for verify"))?; + let client = ado::AdoClient::new(&pat)?; + client.verify(&manifest.azure_devops, lock_ado).await?; + println!("OK: ado fixture matches manifest"); + } + + Ok(()) +} + +async fn reset_comments( + cli: &Cli, + admin_gh: &str, + admin_ado: &str, +) -> anyhow::Result<()> { + let manifest = read_manifest(&cli.manifest)?; + let lock = read_lock(&cli.lock)?; + + if !cli.skip_github { + let lock_gh = lock + .github + .as_ref() + .ok_or_else(|| anyhow::anyhow!("no github lock entry; run `apply` first"))?; + let token = std::env::var("GITHUB_TOKEN_ADMIN") + .map_err(|_| anyhow::anyhow!("GITHUB_TOKEN_ADMIN required for reset-comments"))?; + let client = github::GithubClient::new(token)?; + let n = reset::reset_github_comments(&client, &manifest.github, lock_gh, admin_gh).await?; + println!("OK: github reset-comments deleted {n}"); + } + + if !cli.skip_ado { + let lock_ado = lock + .azure_devops + .as_ref() + .ok_or_else(|| anyhow::anyhow!("no ado lock entry; run `apply` first"))?; + let pat = std::env::var("ADO_PAT_ADMIN") + .map_err(|_| anyhow::anyhow!("ADO_PAT_ADMIN required for reset-comments"))?; + let client = ado::AdoClient::new(&pat)?; + let n = + reset::reset_ado_comments(&client, &manifest.azure_devops, lock_ado, admin_ado).await?; + println!("OK: ado reset-comments deleted {n}"); + } + + Ok(()) +} + +async fn destroy(_cli: &Cli, yes_really: bool) -> anyhow::Result<()> { + if !yes_really { + anyhow::bail!("destroy requires `--yes-really`; aborting"); + } + // First-cut: not implemented. Manual teardown via the host UI is + // safer for now (we don't want a misconfigured cron eating the + // fixture org). Surface this clearly rather than silently + // pretending to succeed. + anyhow::bail!( + "destroy is not implemented in the first cut; tear down fixtures manually \ + (see docs/internals/live-test-fixtures.md)" + ) +} + +async fn print_env(cli: &Cli) -> anyhow::Result<()> { + let manifest = read_manifest(&cli.manifest)?; + let lock = read_lock(&cli.lock)?; + + if let Some(gh) = &lock.github { + println!("DEVDEV_LIVE_GITHUB=1"); + println!( + "DEVDEV_GH_PR_URL=https://github.com/{}/{}/pull/{}", + manifest.github.org, manifest.github.repo, gh.canonical_pr_number + ); + println!("DEVDEV_GH_FIXTURE_ORG={}", manifest.github.org); + println!("DEVDEV_GH_FIXTURE_REPO={}", manifest.github.repo); + } + if let Some(ado) = &lock.azure_devops { + println!("DEVDEV_LIVE_ADO=1"); + println!( + "DEVDEV_ADO_PR_URL=https://dev.azure.com/{}/{}/_git/{}/pullrequest/{}", + manifest.azure_devops.org, + manifest.azure_devops.project, + manifest.azure_devops.repo, + ado.canonical_pr_id, + ); + println!("DEVDEV_ADO_FIXTURE_ORG={}", manifest.azure_devops.org); + println!("DEVDEV_ADO_FIXTURE_PROJECT={}", manifest.azure_devops.project); + println!("DEVDEV_ADO_FIXTURE_REPO={}", manifest.azure_devops.repo); + } + Ok(()) +} diff --git a/crates/devdev-test-env/src/manifest.rs b/crates/devdev-test-env/src/manifest.rs new file mode 100644 index 0000000..e1b3fcc --- /dev/null +++ b/crates/devdev-test-env/src/manifest.rs @@ -0,0 +1,293 @@ +//! Manifest types and on-disk format. +//! +//! `manifest.json` is the *declarative* source of truth: org/project +//! names, fixture branch + PR shape, the canonical commit message. +//! `manifest.lock.json` is the *materialised* state: PR numbers and +//! other server-assigned ids the provisioner backfills after first +//! apply. Lock-file pattern matches `Cargo.lock` semantics — committed, +//! regenerable, but stable across runs once written. + +use std::path::Path; + +use serde::{Deserialize, Serialize}; + +/// Top-level manifest committed to `test-env/manifest.json`. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct Manifest { + pub github: GithubFixture, + pub azure_devops: AdoFixture, +} + +/// GitHub fixture description. +/// +/// The org is *asserted* (manual provisioning), every other resource +/// is ensured by `apply`. `comment_tag_prefix` is the literal string +/// every test-issued comment must start with so `reset-comments` can +/// distinguish them from the canonical PR description / admin-pinned +/// notes that must survive cleanup. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct GithubFixture { + pub org: String, + pub repo: String, + pub default_branch: String, + pub fixture_branch: String, + pub canonical_pr: CanonicalPr, + pub comment_tag_prefix: String, +} + +/// Azure DevOps fixture description. +/// +/// ADO addresses repos as `{org}/{project}/_git/{repo}`. Our adapter +/// encodes this as `owner = "{org}/{project}"`, `repo = "{repo}"`; +/// the manifest keeps them split because that's how ADO's REST API +/// wants them too. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct AdoFixture { + pub org: String, + pub project: String, + pub repo: String, + pub default_branch: String, + pub fixture_branch: String, + pub canonical_pr: CanonicalPr, + pub comment_tag_prefix: String, +} + +/// Canonical PR shape — the same struct on both hosts. The PR +/// number itself lives in [`ManifestLock`], not here. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct CanonicalPr { + /// Title — used as the PR's title and as part of the + /// `reset-comments` allow-list match. + pub title: String, + /// Body — admin-authored description. Tests must NOT modify + /// this. `verify` asserts it byte-for-byte. + pub body: String, + /// Branch the PR targets (defaults to `default_branch` if not + /// specified, but explicit avoids the merge-conflict footgun). + pub base: String, + /// Files committed on `fixture_branch` to seed the diff. Empty + /// vec is allowed — produces a no-change PR (still valid). + #[serde(default)] + pub fixture_files: Vec, +} + +/// Single file checked into the fixture branch. Contents are +/// rewritten on every `apply` if they drift, so this is the only +/// place to edit fixture content. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct FixtureFile { + pub path: String, + pub contents: String, +} + +/// Lock file written to `test-env/manifest.lock.json` after first +/// `apply`. Carries server-assigned ids that didn't exist at +/// manifest-edit time. Committed alongside the manifest. +#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct ManifestLock { + /// Per-host resolved state. `None` means the corresponding + /// `apply` hasn't been run yet (fresh checkout). + #[serde(default)] + pub github: Option, + #[serde(default)] + pub azure_devops: Option, +} + +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct GithubLock { + pub repo_id: u64, + pub canonical_pr_number: u64, + pub canonical_pr_node_id: String, +} + +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] +pub struct AdoLock { + pub repo_id: String, + pub canonical_pr_id: u64, +} + +impl Manifest { + pub fn read(path: &Path) -> anyhow::Result { + let bytes = std::fs::read(path) + .map_err(|e| anyhow::anyhow!("failed to read manifest at {}: {e}", path.display()))?; + let manifest: Manifest = serde_json::from_slice(&bytes) + .map_err(|e| anyhow::anyhow!("failed to parse manifest at {}: {e}", path.display()))?; + manifest.validate()?; + Ok(manifest) + } + + /// Cheap structural checks. Things the REST API would reject + /// later anyway, surfaced earlier with a useful diagnostic. + pub fn validate(&self) -> anyhow::Result<()> { + if self.github.org.is_empty() || self.github.repo.is_empty() { + anyhow::bail!("github.org and github.repo must be non-empty"); + } + if self.azure_devops.org.is_empty() + || self.azure_devops.project.is_empty() + || self.azure_devops.repo.is_empty() + { + anyhow::bail!("azure_devops.{{org,project,repo}} must all be non-empty"); + } + if self.github.canonical_pr.base != self.github.default_branch { + anyhow::bail!( + "github canonical_pr.base ({}) must currently equal default_branch ({}); \ + cross-branch fixtures aren't supported in the first cut", + self.github.canonical_pr.base, + self.github.default_branch, + ); + } + if !self.github.comment_tag_prefix.starts_with('[') + || !self.github.comment_tag_prefix.ends_with(']') + { + anyhow::bail!( + "github.comment_tag_prefix must be of the form `[devdev-live-test...]`" + ); + } + if !self.azure_devops.comment_tag_prefix.starts_with('[') + || !self.azure_devops.comment_tag_prefix.ends_with(']') + { + anyhow::bail!("azure_devops.comment_tag_prefix must be `[...]`-bracketed"); + } + Ok(()) + } +} + +impl ManifestLock { + pub fn read_or_default(path: &Path) -> anyhow::Result { + if !path.exists() { + return Ok(ManifestLock::default()); + } + let bytes = std::fs::read(path)?; + Ok(serde_json::from_slice(&bytes)?) + } + + pub fn write(&self, path: &Path) -> anyhow::Result<()> { + let pretty = serde_json::to_string_pretty(self)?; + std::fs::write(path, format!("{pretty}\n"))?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sample() -> Manifest { + Manifest { + github: GithubFixture { + org: "goldenwitch".into(), + repo: "devdev-test-environment".into(), + default_branch: "main".into(), + fixture_branch: "fixture/canonical".into(), + canonical_pr: CanonicalPr { + title: "Canonical fixture PR — DO NOT MERGE".into(), + body: "This PR is provisioned by devdev-test-env.".into(), + base: "main".into(), + fixture_files: vec![FixtureFile { + path: "FIXTURE.md".into(), + contents: "fixture\n".into(), + }], + }, + comment_tag_prefix: "[devdev-live-test]".into(), + }, + azure_devops: AdoFixture { + org: "devdev-fixtures".into(), + project: "DevDev-Live".into(), + repo: "devdev-test-environment".into(), + default_branch: "main".into(), + fixture_branch: "fixture/canonical".into(), + canonical_pr: CanonicalPr { + title: "Canonical fixture PR — DO NOT MERGE".into(), + body: "Provisioned by devdev-test-env.".into(), + base: "main".into(), + fixture_files: vec![], + }, + comment_tag_prefix: "[devdev-live-test]".into(), + }, + } + } + + #[test] + fn validate_accepts_sample() { + sample().validate().unwrap(); + } + + #[test] + fn validate_rejects_empty_org() { + let mut m = sample(); + m.github.org = String::new(); + assert!(m.validate().is_err()); + } + + #[test] + fn validate_rejects_unbracketed_tag_prefix() { + let mut m = sample(); + m.github.comment_tag_prefix = "devdev-live-test".into(); + let err = m.validate().unwrap_err().to_string(); + assert!(err.contains("comment_tag_prefix"), "diagnostic was: {err}"); + } + + #[test] + fn validate_rejects_cross_branch_pr() { + let mut m = sample(); + m.github.canonical_pr.base = "develop".into(); + let err = m.validate().unwrap_err().to_string(); + assert!(err.contains("default_branch"), "diagnostic was: {err}"); + } + + #[test] + fn manifest_round_trips_through_json() { + let json = serde_json::to_string_pretty(&sample()).unwrap(); + let back: Manifest = serde_json::from_str(&json).unwrap(); + assert_eq!(sample(), back); + } + + #[test] + fn lock_default_has_no_resolved_state() { + let lock = ManifestLock::default(); + assert!(lock.github.is_none()); + assert!(lock.azure_devops.is_none()); + } + + #[test] + fn lock_round_trips_through_json() { + let lock = ManifestLock { + github: Some(GithubLock { + repo_id: 12345, + canonical_pr_number: 7, + canonical_pr_node_id: "PR_kwDO".into(), + }), + azure_devops: Some(AdoLock { + repo_id: "abcd-ef".into(), + canonical_pr_id: 42, + }), + }; + let json = serde_json::to_string_pretty(&lock).unwrap(); + let back: ManifestLock = serde_json::from_str(&json).unwrap(); + assert_eq!(lock, back); + } + + #[test] + fn lock_rejects_unknown_fields() { + let bad = r#"{"github": null, "azure_devops": null, "stray": 1}"#; + let err = serde_json::from_str::(bad).unwrap_err(); + assert!(err.to_string().contains("stray"), "got: {err}"); + } + + #[test] + fn read_returns_diagnostic_for_missing_file() { + let err = Manifest::read(Path::new("does/not/exist.json")).unwrap_err(); + assert!( + err.to_string().contains("failed to read manifest"), + "diagnostic was: {err}" + ); + } +} diff --git a/crates/devdev-test-env/src/reset.rs b/crates/devdev-test-env/src/reset.rs new file mode 100644 index 0000000..b803756 --- /dev/null +++ b/crates/devdev-test-env/src/reset.rs @@ -0,0 +1,155 @@ +//! `reset-comments` implementation. +//! +//! Strategy: every test-issued comment is required to begin with the +//! manifest's `comment_tag_prefix` (`[devdev-live-test...]`). On +//! cleanup we sweep: +//! +//! - Every comment authored by a non-admin user (definitionally +//! test-originated; admin tokens never run from test code). +//! - Every comment authored by the admin identity that *also* +//! starts with the tag prefix (so admin-pinned canonical notes +//! without the tag survive). +//! +//! On ADO we additionally skip system-status comments +//! (`commentType == "system"`) — those represent vote/policy events +//! and are not deletable anyway. + +use crate::ado::{AdoClient, ThreadComment}; +use crate::github::{GithubClient, IssueComment}; +use crate::manifest::{AdoFixture, AdoLock, GithubFixture, GithubLock}; + +/// Sweep stray comments off the canonical GitHub PR. +/// +/// `admin_login` is the GitHub login of the admin identity whose +/// token authored the canonical PR. Comments NOT by that login are +/// always removed; admin-authored comments are removed only if +/// they begin with the configured tag prefix. +pub async fn reset_github_comments( + client: &GithubClient, + fixture: &GithubFixture, + lock: &GithubLock, + admin_login: &str, +) -> anyhow::Result { + let comments = client + .list_pr_comments(&fixture.org, &fixture.repo, lock.canonical_pr_number) + .await?; + let mut deleted = 0usize; + for c in comments { + if should_delete_github(&c, admin_login, &fixture.comment_tag_prefix) { + client + .delete_issue_comment(&fixture.org, &fixture.repo, c.id) + .await?; + deleted += 1; + } + } + Ok(deleted) +} + +/// Sweep stray comments off the canonical ADO PR. Returns the +/// number of comments deleted. +pub async fn reset_ado_comments( + client: &AdoClient, + fixture: &AdoFixture, + lock: &AdoLock, + admin_unique_name: &str, +) -> anyhow::Result { + let threads = client + .list_pr_threads(fixture, &lock.repo_id, lock.canonical_pr_id) + .await?; + let mut deleted = 0usize; + for thread in threads { + if thread.status.as_deref() == Some("closed") { + continue; + } + for c in &thread.comments { + if should_delete_ado(c, admin_unique_name, &fixture.comment_tag_prefix) { + client + .delete_thread_comment( + fixture, + &lock.repo_id, + lock.canonical_pr_id, + thread.id, + c.id, + ) + .await?; + deleted += 1; + } + } + } + Ok(deleted) +} + +fn should_delete_github(c: &IssueComment, admin: &str, tag_prefix: &str) -> bool { + if c.user.login.eq_ignore_ascii_case(admin) { + body_carries_tag(&c.body, tag_prefix) + } else { + true + } +} + +fn should_delete_ado(c: &ThreadComment, admin: &str, tag_prefix: &str) -> bool { + if c.author.unique_name.eq_ignore_ascii_case(admin) { + body_carries_tag(&c.content, tag_prefix) + } else { + true + } +} + +/// A comment "carries the tag" if its trimmed body opens with the +/// configured prefix's leading-marker portion. The manifest stores +/// `[devdev-live-test]` as the canonical prefix, but real test +/// comments use `[devdev-live-test::]`, so we compare +/// against the prefix sans the trailing `]`. +fn body_carries_tag(body: &str, tag_prefix: &str) -> bool { + let opener = tag_prefix.strip_suffix(']').unwrap_or(tag_prefix); + body.trim_start().starts_with(opener) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::github::CommentUser; + + fn gh(body: &str, login: &str) -> IssueComment { + IssueComment { + id: 1, + body: body.into(), + user: CommentUser { + login: login.into(), + }, + } + } + + #[test] + fn github_keeps_admin_canonical_note() { + let c = gh("Canonical reviewer note (do not delete).", "devdev-bot"); + assert!(!should_delete_github(&c, "devdev-bot", "[devdev-live-test]")); + } + + #[test] + fn github_deletes_admin_tagged_comment() { + let c = gh("[devdev-live-test:foo:42] hello", "devdev-bot"); + assert!(should_delete_github(&c, "devdev-bot", "[devdev-live-test]")); + } + + #[test] + fn github_deletes_any_non_admin_comment() { + let c = gh("hi from a contributor", "someone-else"); + assert!(should_delete_github(&c, "devdev-bot", "[devdev-live-test]")); + } + + #[test] + fn github_login_match_is_case_insensitive() { + let c = gh("plain admin note", "DevDev-Bot"); + assert!(!should_delete_github(&c, "devdev-bot", "[devdev-live-test]")); + } + + #[test] + fn github_tag_must_be_at_start_after_trim() { + // Leading whitespace is fine; tag in the middle is not. + let leading = gh(" [devdev-live-test] yo", "devdev-bot"); + assert!(should_delete_github(&leading, "devdev-bot", "[devdev-live-test]")); + let middle = gh("hi [devdev-live-test] yo", "devdev-bot"); + assert!(!should_delete_github(&middle, "devdev-bot", "[devdev-live-test]")); + } +} diff --git a/crates/devdev-test-env/src/secret.rs b/crates/devdev-test-env/src/secret.rs new file mode 100644 index 0000000..70898af --- /dev/null +++ b/crates/devdev-test-env/src/secret.rs @@ -0,0 +1,68 @@ +//! Redacting wrapper for short-lived auth tokens (PATs, GitHub App +//! installation tokens, AAD bearer tokens). +//! +//! Holds the raw value as a `String`, but `Debug`/`Display` always +//! redact. The only way to read the value is [`Token::expose`], which +//! is intentionally verbose to make leak-introducing edits show up in +//! review. +//! +//! This is a deliberately *minimal* type — we don't need full +//! `secrecy::Secret` semantics (no zero-on-drop guarantee; the OS can +//! always page the value to disk regardless). The goal is "no +//! accidental `{:?}` leak", not memory hardening. + +use std::fmt; + +#[derive(Clone)] +pub struct Token(String); + +impl Token { + pub fn new(raw: impl Into) -> Self { + Self(raw.into()) + } + + /// Borrow the raw token. The verbose name is intentional — + /// every call site should be auditable. + pub fn expose(&self) -> &str { + &self.0 + } +} + +impl fmt::Debug for Token { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "Token([redacted; {} bytes])", self.0.len()) + } +} + +impl fmt::Display for Token { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "[redacted]") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn debug_redacts_value() { + let t = Token::new("ghs_supersecret"); + let s = format!("{t:?}"); + assert!(s.contains("redacted")); + assert!(!s.contains("ghs_")); + assert!(!s.contains("supersecret")); + } + + #[test] + fn display_redacts_value() { + let t = Token::new("ghs_supersecret"); + let s = format!("{t}"); + assert_eq!(s, "[redacted]"); + } + + #[test] + fn expose_returns_raw() { + let t = Token::new("ghs_supersecret"); + assert_eq!(t.expose(), "ghs_supersecret"); + } +} diff --git a/docs/internals/ghe-gap.md b/docs/internals/ghe-gap.md new file mode 100644 index 0000000..17d4a68 --- /dev/null +++ b/docs/internals/ghe-gap.md @@ -0,0 +1,117 @@ +# GitHub Enterprise — the gap and how to close it + +DevDev's `RepoHostId` plumbing, `RepoHostAdapter` trait, and +`RepoHostRegistry` were designed to support **GitHub Enterprise +Server** (GHE) on equal footing with GitHub.com and Azure DevOps. +The code paths are real: + +- A single `GitHubAdapter` impl serves both github.com and GHE; the + `RepoHostId::api_base` field switches between + `https://api.github.com` and `https:///api/v3`. +- `PrRef::parse` recognises GHE-shaped URLs and threads the right + `RepoHostId` through to the dispatch maps and event bus. +- `RepoHostRegistry::for_url` classifies arbitrary GHE hosts via + the `ghe.*` heuristic + explicit registration. +- `CredentialStore` keys on `RepoHostId`, so per-GHE-host tokens + (`GHE_TOKEN_` env vars) drop in cleanly. + +What we **do not** have today is a live test against an actual GHE +instance. This document explains why, what we still rely on for +GHE confidence, and what it would take to close the gap. + +## Why GHE isn't in CI today + +Three options exist, and all three have costs we're choosing not +to absorb in the first cut: + +1. **Self-hosted GHE VM in CI** + - Highest fidelity (real on-prem product). + - Requires a GHE license, a host with ~32 GB RAM, the operational + burden of upgrades, backups, and TLS certificates. + - Not justifiable for a project at our current scale. +2. **GHE.com / data residency** + - A real GHE-classified host suffix (`*.ghe.com`) on + GitHub-managed infrastructure. + - Same wire protocol as on-prem. + - Paid plan; pricing scales with seat count. +3. **Record/replay (VCR-style)** + - Capture real GHE responses once; replay in CI. + - Cheap, fast, but no longer a *live* test — adapter changes + against newer GHE versions go undetected until the next + re-record. + - Violates the "real path" rule from + [`spirit/05-validation.md`](../../spirit/05-validation.md). + +We picked option 4: **explicitly skip GHE in CI**, document the +gap, and lean on adapter unit tests + URL-parsing tests + +`host_id`-plumbing identity proofs. + +## What we still rely on for GHE confidence + +| Coverage | Where | +|---|---| +| URL classification (`ghe.*` heuristic, `from_browse_host` round-trip) | `crates/devdev-integrations/src/host.rs` (8 unit tests) | +| `PrRef::parse` accepts GHE URLs and produces the right `RepoHostId` | `crates/devdev-tasks/src/pr_ref.rs` (16 unit tests) | +| `GitHubAdapter::ghe(host, token)` constructor sets `api_base` correctly | `crates/devdev-integrations/src/github.rs` (acceptance) | +| `RepoHostRegistry::for_url` routes GHE URLs to the right adapter | `crates/devdev-daemon/src/host_registry.rs` (8 unit tests) | +| Dispatch keys disambiguate `(owner, repo)` collisions across hosts | `crates/devdev-tasks/src/events.rs` (`pr_target_disambiguates_by_host`) | +| End-to-end IPC routing across hosts (mock GHE adapter) | `crates/devdev-scenarios/tests/scenarios.rs` (`s08_multi_host_registry_routes_by_host`) | + +What this catches: + +- Adapter boilerplate diverging between github.com and GHE codepaths. +- URL parsing regressions when someone refactors `RepoHostId`. +- Ledger-key collisions across hosts. + +What this does **not** catch: + +- A real GHE instance returning a slightly different response shape + (rare, but historically possible during major-version rollouts). +- TLS chain issues against on-prem GHE installs with private CAs. +- Auth-header dialect shifts (e.g. if GHE adds a new required + header in a future major release). + +These are the risks the "live GHE" tests would close. They're real, +but bounded. + +## What it would take to close the gap + +If a contributor or sponsor wants this: + +1. **Pick a GHE flavour** — GHE.com data-residency tier is the + easiest provisioning path; a self-hosted VM is the most + faithful. +2. **Provision a fixture there.** The `devdev-test-env` crate + already abstracts the manifest; add a `ghe` block alongside + `github` and `azure_devops` in + [`test-env/manifest.json`](../../test-env/manifest.json). + Reuse `GithubClient` (point it at the GHE API base) for `apply`. +3. **Wire CI.** Add a `live-tests-ghe` job to + [`.github/workflows/live-tests.yml`](../../.github/workflows/live-tests.yml). + Gate it on a `LIVE_GHE_HOST` repository variable so the same + workflow handles "GHE configured" and "GHE not configured". +4. **Add a live test.** A GHE-flavoured twin of the planned + `live_ado_pr` test (read-only PR fetch, host-id assertion, + write-mode-gated comment round-trip). + +Estimated work: a day for the IaC + workflow plumbing, plus +ongoing license / runtime cost for the GHE instance itself. + +## How to volunteer + +If your org runs a GHE instance and you would be willing to: + +- Provision a small fixture org/repo on it, scoped to a + long-lived bot account. +- Issue a fine-grained PAT for that bot, rotated on a 90-day + cadence. +- Allow inbound traffic from GitHub Actions runners (or run a + self-hosted runner inside your network). + +…open an issue tagged `live-tests:ghe` and we'll work the rest +together. We'd rather have one real GHE in CI for one quarter +than recorded fixtures forever. + +The same is true for any **other** repo host we don't currently +test live (Bitbucket, Gitea, Forgejo) — the abstraction is built +to extend, and a contributor sponsorship is the missing piece. diff --git a/docs/internals/live-test-fixtures.md b/docs/internals/live-test-fixtures.md new file mode 100644 index 0000000..fb44959 --- /dev/null +++ b/docs/internals/live-test-fixtures.md @@ -0,0 +1,186 @@ +# Live-test fixture environment + +DevDev's live-test claims (`AGENT-FS-WRITE`, `DAEMON-AGENT-FS-WRITE`, +and the multi-host claims tracked in [`spec-repo-hosts.md`](./spec-repo-hosts.md)) +talk to **real** services. To prove they work without burdening +contributors with bespoke setup, we provision a single shared fixture +environment from a committed manifest. + +This document is the operational story: + +- How the environment is *defined*. +- How it is *bootstrapped* the first time. +- Which *principals* and *secrets* power it. +- How CI *consumes* it. + +The architectural decisions live in +[`spec-repo-hosts.md`](./spec-repo-hosts.md). The GHE gap and the +sponsorship invitation live in [`ghe-gap.md`](./ghe-gap.md). + +## Source of truth + +| File | Purpose | +|---|---| +| [`test-env/manifest.json`](../../test-env/manifest.json) | Declarative manifest. Hand-edited. Reviewed via `CODEOWNERS`. | +| [`test-env/manifest.lock.json`](../../test-env/manifest.lock.json) | Server-assigned ids (PR numbers, repo ids). Generated by `devdev-test-env apply`; committed. | +| [`crates/devdev-test-env/`](../../crates/devdev-test-env/) | The Rust binary that reads the manifest and reconciles state via the GitHub and ADO REST APIs. | +| [`.github/workflows/live-tests.yml`](../../.github/workflows/live-tests.yml) | The CI surface. Three jobs: `provision` → `live-tests` → `cleanup`. | + +## Provisioned fixtures + +``` +GitHub.com +└── account "goldenwitch" + └── repo "devdev-test-environment" + ├── branch main + ├── branch fixture/canonical + └── PR "Canonical fixture PR — DO NOT MERGE" (open, against main) + +Azure DevOps +└── org "devdev-fixtures" + └── project "DevDev-Live" + └── repo "devdev-test-environment" + ├── branch main + ├── branch fixture/canonical + └── PR "Canonical fixture PR — DO NOT MERGE" (active) +``` + +Both PRs stay **open indefinitely**. Tests post tagged comments +(`[devdev-live-test::]...`) which the post-test +`reset-comments` step sweeps. *Never* merge or close these PRs; +treat them as immutable test fixtures. + +## Principal model + +Two distinct identities. Token rotation cadence: 90 days. + +### Admin identity (one bot per host) + +- **GitHub bot**: `devdev-bot` (or whatever account owns + `devdev-fixtures` org). Holds a fine-grained PAT scoped to that + org with `Contents: write`, `Pull requests: write`, + `Issues: write`, and `Administration: write`. Stored as + `secrets.GITHUB_TOKEN_ADMIN`. Login string stored as + `vars.DEVDEV_TEST_ENV_GITHUB_ADMIN_LOGIN`. +- **ADO bot**: an Entra/MSA identity with admin on the + `devdev-fixtures` ADO org. Holds an org-scoped PAT with + `Code (Read & write)`, `Pull Request Threads (Read & write)`. + Stored as `secrets.ADO_PAT_ADMIN`; the bot's `uniqueName` + (e.g. an email) is stored as `vars.DEVDEV_TEST_ENV_ADO_ADMIN_NAME`. + +Admin tokens are **only** visible to the `provision` and `cleanup` +jobs, both pinned to the `live-tests-admin` GitHub Environment for +the manual approval gate. + +### Consumer identity (used by tests) + +- **`secrets.DEVDEV_GH_TOKEN`** — fine-grained PAT scoped to + `devdev-fixtures/live-tests` only. Read + comment, no admin. +- **`secrets.DEVDEV_ADO_TOKEN`** — project-scoped PAT for + `devdev-fixtures/DevDev-Live`. Read + Code-write. +- **`secrets.DEVDEV_COPILOT_GH_TOKEN`** — a *separate* GH account's + token. The account holds a Copilot Individual seat. The + `live-tests` job calls `gh auth login --with-token` with this + value so `copilot --acp` can authenticate. + +Visible only to the `live-tests` job (`live-tests-consumer` +Environment). Admin tokens never leak into test code. + +## Bootstrap (one-time) + +You only do this when standing up a fresh fixture host. + +1. Create the bot accounts and the PATs above. Record their values + in the corresponding `secrets`/`vars` of the GitHub repo. +2. On GitHub: create the `devdev-fixtures` org. The repo will be + created on first `apply`. +3. On ADO: create the `devdev-fixtures` org and the `DevDev-Live` + project (ADO org-and-project creation is not REST-driven for + first-time tenants). Create an empty `main` branch and an + empty `fixture/canonical` branch in the `live-tests` repo + (we do this via `git push` once; subsequent apply runs only + touch the PR-level state). +4. From your dev machine, with the admin tokens in env: + + ```pwsh + $env:GITHUB_TOKEN_ADMIN = "" + $env:ADO_PAT_ADMIN = "" + cargo run -p devdev-test-env --release -- apply + ``` + +5. Verify the lock file looks right and commit it: + + ```pwsh + cargo run -p devdev-test-env --quiet -- verify + git add test-env/manifest.lock.json + git commit -m "live-test: bootstrap manifest.lock" + ``` + +6. Configure two GitHub Environments on the repo settings page: + - `live-tests-admin` — required reviewer = a maintainer; secrets + `GITHUB_TOKEN_ADMIN`, `ADO_PAT_ADMIN`; vars + `DEVDEV_TEST_ENV_GITHUB_ADMIN_LOGIN`, `DEVDEV_TEST_ENV_ADO_ADMIN_NAME`. + - `live-tests-consumer` — no required reviewers (provision job + gated upstream is the lock); secrets `DEVDEV_GH_TOKEN`, + `DEVDEV_ADO_TOKEN`, `DEVDEV_COPILOT_GH_TOKEN`. +7. Trigger one manual `workflow_dispatch` run of `live-tests` to + confirm the wiring before relying on the nightly cron. + +## Day-to-day operations + +```pwsh +# After editing test-env/manifest.json: +cargo run -p devdev-test-env --quiet -- verify # is reality already in sync? +cargo run -p devdev-test-env --quiet -- apply # reconcile. +git diff test-env/manifest.lock.json # any new PR ids? +git add test-env/manifest.lock.json && git commit ... + +# Before running live tests locally: +$env:GITHUB_TOKEN_ADMIN = "<...>" +$env:ADO_PAT_ADMIN = "<...>" +cargo run -p devdev-test-env --quiet -- print-env | Out-File .env.live +. ./.env.live +cargo test --workspace -- --ignored +``` + +## Cost + +| Item | Approx. cost | +|---|---| +| GitHub Free org + repo | $0 | +| Azure DevOps Free tier | $0 (5 free users / 2 GiB) | +| Copilot Individual seat (one) | ~$10/mo | + +The Copilot seat is the only paid line item, and it's scoped to a +*single* CI bot account. + +## Tagging convention + +Every comment a test posts MUST begin with the prefix from the +manifest (`[devdev-live-test]`), typically extended with a +`:` segment, e.g.: + +``` +[devdev-live-test:live_ado_pr:1737abf] hello from the live test +``` + +`reset-comments` deletes every non-admin comment plus any admin +comment whose body starts with `[devdev-live-test`. Untagged +admin notes (e.g. canonical reviewer reminders pinned by hand) +survive cleanup. + +A small helper `live_test_comment(test_name, body)` in the shared +test-utils module enforces this convention so individual tests +cannot forget the tag. + +## Falsification + +You can verify this isn't a paper claim: + +1. Manually rename the canonical PR title in the GitHub UI. +2. Run `cargo run -p devdev-test-env -- verify`. +3. The command should exit non-zero with a diff diagnostic naming + the manifest's expected title and the live title. + +Same for ADO: rename the canonical PR and re-run; `verify` fails +loudly. diff --git a/docs/internals/spec-repo-hosts.md b/docs/internals/spec-repo-hosts.md new file mode 100644 index 0000000..055248b --- /dev/null +++ b/docs/internals/spec-repo-hosts.md @@ -0,0 +1,170 @@ +# Spec: Multi-host repo support (GitHub.com, GHE, Azure DevOps) + +Status: living document; reflects the state shipped on the +`feature/ado-ghe-support` branch (Phases 1–7). + +## Why + +DevDev was originally built around a single hard-coded GitHub.com +adapter. Customers on GitHub Enterprise (GHE) and Azure DevOps +(ADO) need first-class support without forking the daemon. This +document captures the abstractions that let one running daemon +serve several hosts simultaneously without any of them shadowing +or impersonating the others. + +## The four seams + +``` + PR URL or "owner/repo#N" + │ + ▼ + ┌─────────────────────┐ + │ PrRef │ carries (host_id, owner, repo, number) + └──────────┬──────────┘ + │ + ┌────────────┴────────────┐ + ▼ ▼ + ┌────────────────────┐ ┌──────────────────────┐ + │ RepoHostId │ │ CredentialStore │ + │ (kind/api/host) │ │ keyed by host_id │ + └─────────┬──────────┘ └──────────┬───────────┘ + │ │ + ▼ ▼ + ┌────────────────────┐ ┌──────────────────────┐ + │ RepoHostRegistry │ │ AskRequest.host → │ + │ host_id → adapter │ │ Credential lookup │ + └─────────┬──────────┘ └──────────────────────┘ + │ + ▼ + ┌────────────────────┐ + │ RepoHostAdapter │ github / ghe / azure_devops + └────────────────────┘ +``` + +1. **`RepoHostId`** (`devdev-integrations::host`) — the routing key. + `{kind, api_base, host}`. Constructed via `github_com()`, + `ghe(host)`, `azure_devops()`, or by classifying a browser host + string with `from_browse_host(host)`. Serialises to a stable + `:` ledger key (`github:github.com`, + `github:ghe.acme.io`, `ado:dev.azure.com`). + +2. **`RepoHostAdapter`** (`devdev-integrations`) — the API surface. + One implementation per forge family. `host_id()` returns its + `RepoHostId`; the daemon never assumes which one until it asks. + +3. **`RepoHostRegistry`** (`devdev-daemon::host_registry`) — the + adapter lookup table. `for_host(&host_id)` and `for_url(url)`. + Built once at `devdev up`; immutable thereafter (the same + lifecycle invariant that protects `CredentialStore`). + +4. **`CredentialStore`** (`devdev-daemon::credentials`) — frozen + token snapshot keyed by `RepoHostId`. Sampled once at boot via + `CredentialProvider`s (env vars, `gh auth token`, + `az account get-access-token`). The agent never sees a token + except via approved `devdev_ask` round-trips, and the response + only releases the token bound to the requested `host`. + +## Key invariants + +- **Identity disambiguation.** `(owner, repo)` and + `(owner, repo, number)` are *not* unique on their own. Every map + in dispatch (`repo_watch_ids`, `monitor_pr_ids`), every event in + `DaemonEvent`, and every ledger key includes the `RepoHostId`. + Cross-host collisions are tested in `events.rs`'s + `pr_target_disambiguates_by_host` and in scenario S08. +- **Snapshot-once credentials.** `CredentialStore` clones into an + `Arc` at construction. Mutating the source environment + after `devdev up` cannot leak into the daemon's auth state. See + `store_is_immutable_after_snapshot_env_mutation`. +- **Hard rejection on unknown hosts.** Both `repo/watch` and + `devdev_ask` reject unknown host strings (`-32602` IPC error or + `AskResponse::Rejected`) rather than silently routing to + github.com. Silent fallback would be a credential-leakage + footgun. +- **Default host is github.com.** Clients that don't supply a + `host` field — including legacy single-host MCP clients — + resolve to `RepoHostId::github_com()`. This keeps the entire + pre-multi-host surface wire-compatible. + +## URL parsing + +`PrRef::parse` accepts: + +| Shape | host_id | +| ------------------------------------------------------------ | ------------------------ | +| `owner/repo#N` | `github_com()` | +| `https://github.com/owner/repo/pull/N[/files]` | `github_com()` | +| `https://ghe.example.com/owner/repo/pull/N` | `ghe("ghe.example.com")` | +| `https://dev.azure.com/{org}/{project}/_git/{repo}/pullrequest/{id}` | `azure_devops()` | +| `https://{org}.visualstudio.com/{project}/_git/{repo}/pullrequest/{id}` | `azure_devops()` | + +ADO's three-level identity (`org`/`project`/`repo`) is encoded as +`owner = "{org}/{project}"`, `repo = "{repo}"` to fit the existing +`(owner, repo, number)` adapter surface. The `AzureDevOpsAdapter` +re-splits this on the way out to its REST API. + +## Wire surface + +### `repo/watch` and `repo/unwatch` + +```jsonc +{ + "method": "repo/watch", + "params": { + "owner": "platform", + "repo": "api", + "host": "ghe.acme.io" // optional; default "github.com" + } +} +``` + +The `host` field is the browser-shaped host (no scheme, no path). +Unknown hosts → `-32602`. + +### `devdev_ask` (MCP tool) + +```jsonc +{ + "kind": "post_review", + "summary": "approve PR #42 on the GHE mirror", + "host": "ghe.acme.io", // optional; default "github.com" + "payload": { ... } +} +``` + +When the request is approved AND the kind requires a token +(`post_review`, `post_comment`, `request_token`), the response's +`token` field is the credential bound to the requested host — +*never* a different host's token. If no credential is stored for +that host, `token: null` is returned (the agent must surface a +helpful error, not retry on a different host). + +## Test landmarks + +| Concern | Test | +| ------------------------------- | ----------------------------------------------------------------------------------------------------- | +| `RepoHostId` classification | `crates/devdev-integrations/src/host.rs` unit tests | +| `PrRef` URL parsing (all forms) | `crates/devdev-tasks/src/pr_ref.rs` unit tests (16) | +| Credential snapshot lifecycle | `crates/devdev-daemon/src/credentials.rs::store_is_immutable_after_snapshot_env_mutation` | +| Registry routing | `crates/devdev-daemon/src/host_registry.rs` (8 tests) | +| Event host-id disambiguation | `crates/devdev-tasks/src/events.rs::pr_target_disambiguates_by_host` | +| Ask host selector + rejection | `crates/devdev-daemon/src/mcp/provider.rs::ask_routes_token_by_host_selector`, `ask_unknown_host_is_rejected` | +| End-to-end IPC | `crates/devdev-scenarios/tests/scenarios.rs::s08_multi_host_registry_routes_by_host` | + +## Open follow-ups + +- **Preferences-driven registry seeding.** Today `devdev up` seeds + the registry with one entry: github.com → the default adapter. + A `[[repo]]` block in `.devdev/preferences.toml` should let + users register additional GHE/ADO hosts at boot, each with its + own credential provider chain. +- **Per-host credential provider chains.** `EnvVarProvider` and + `GhCliProvider` are wired up for github.com only; GHE/ADO hosts + need their own `EnvVarProvider` (e.g. `GHE_TOKEN_`, + `AZURE_DEVOPS_PAT`) and `AzCliProvider` instances appended to + the snapshot at boot. +- **Event coordinator routing.** `ensure_monitor_pr_task` accepts + a `&RepoHostId` but the only event source today + (`RepoWatchTask`) only fires for the host its own watch is + bound to. A future webhook receiver will need to set the + correct `host_id` on events it publishes. diff --git a/samples/pr-reviewer/Cargo.toml b/samples/pr-reviewer/Cargo.toml new file mode 100644 index 0000000..49494b1 --- /dev/null +++ b/samples/pr-reviewer/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "pr-reviewer" +version = "0.1.0" +edition.workspace = true +license.workspace = true +publish = false +description = "Sample: poll a GitHub repository for new/updated PRs and ask a Copilot ACP agent to review each one." + +[dependencies] +devdev-acp = { path = "../../crates/devdev-acp" } +devdev-cli = { path = "../../crates/devdev-cli" } +devdev-daemon = { path = "../../crates/devdev-daemon" } +devdev-integrations = { path = "../../crates/devdev-integrations" } + +anyhow = { workspace = true } +async-trait = { workspace = true } +clap = { workspace = true } +tokio = { workspace = true } +tracing = { workspace = true } +tracing-subscriber = { workspace = true } diff --git a/samples/pr-reviewer/src/main.rs b/samples/pr-reviewer/src/main.rs new file mode 100644 index 0000000..19bfd49 --- /dev/null +++ b/samples/pr-reviewer/src/main.rs @@ -0,0 +1,309 @@ +//! # `pr-reviewer` — canonical end-to-end DevDev sample +//! +//! Polls a GitHub repository for open PRs and asks a Copilot ACP agent +//! to review each one as it appears or updates. Reviews print to +//! stdout. **Read-only** — never posts to the PR. +//! +//! The point of this sample is to exercise the **library surface** of +//! every DevDev crate without going through the daemon or its IPC: +//! +//! | Library | What this sample uses | +//! |-----------------|-------------------------------------------| +//! | `devdev-acp` | `AcpClient::connect_process`, prompt loop | +//! | `devdev-cli` | `agent_command::prepare` (resolve+rewrite)| +//! | `devdev-daemon` | `CredentialStore` + provider chain | +//! | `devdev-integrations` | `GitHubAdapter`, `pr_state_hash` | +//! +//! If anything below ends up doing the daemon's work by hand, that's a +//! signal the daemon is hoarding logic that should live in a library +//! crate. Keep this file boring on purpose. +//! +//! ## Usage +//! +//! ```text +//! cargo run -p pr-reviewer -- goldenwitch/devdev +//! cargo run -p pr-reviewer -- goldenwitch/devdev --poll-secs 30 --once +//! ``` +//! +//! Authentication: same providers the daemon uses — `GH_TOKEN` env var +//! or `gh auth login`. No new config surface. + +use std::collections::HashMap; +use std::sync::Arc; +use std::time::Duration; + +use anyhow::{Context, Result, anyhow, bail}; +use async_trait::async_trait; +use clap::Parser; +use devdev_acp::handler::{AcpHandler, HandlerResult}; +use devdev_acp::types::{ + CreateTerminalParams, CreateTerminalResult, KillTerminalParams, NewSessionParams, + PermissionRequestParams, PermissionResponse, PromptContent, PromptParams, ReadTextFileParams, + ReadTextFileResult, ReleaseTerminalParams, SessionUpdate, SessionUpdateParams, + TerminalOutputParams, TerminalOutputResult, WaitForExitParams, WaitForExitResult, + WriteTextFileParams, +}; +use devdev_acp::{AcpClient, AcpClientConfig, AcpError}; +use devdev_cli::agent_command; +use devdev_daemon::credentials::{ + CredentialProvider, CredentialStore, EnvVarProvider, GhCliProvider, +}; +use devdev_integrations::{GitHubAdapter, PullRequest, RepoHostAdapter, RepoHostId, pr_state_hash}; +use tokio::sync::Mutex; + +#[derive(Parser, Debug)] +#[command(about = "Sample: poll a GitHub repo and have a Copilot agent review every PR.")] +struct Args { + /// Repository in `owner/repo` form. github.com only. + repo: String, + + /// Poll interval in seconds. + #[arg(long, default_value_t = 60)] + poll_secs: u64, + + /// Review every currently-open PR once and exit. + #[arg(long)] + once: bool, + + /// Agent program to spawn. Defaults to `copilot`; resolved via + /// `agent_command::prepare`, which handles PATHEXT on Windows and + /// the Copilot SEA-launcher rewrite. + #[arg(long, default_value = "copilot")] + agent_program: String, + + /// Extra arguments to pass to the agent. Defaults match the + /// daemon: `--acp --allow-all-tools` (Copilot CLI's ACP/NDJSON + /// mode with non-interactive tool permissions). + #[arg(long, num_args = 1.., default_values_t = ["--acp".to_string(), "--allow-all-tools".to_string()])] + agent_arg: Vec, +} + +#[tokio::main(flavor = "multi_thread")] +async fn main() -> Result<()> { + init_tracing(); + let args = Args::parse(); + let (owner, repo) = parse_repo(&args.repo)?; + + // 1. Build a `CredentialStore` exactly the way the daemon does: + // env-var first, then `gh auth login`. Same provider chain → + // same precedence → no surprises when users move from this + // sample to `devdev up`. + let host_id = RepoHostId::github_com(); + let providers: Vec> = vec![ + Arc::new(EnvVarProvider::new(host_id.clone(), "GH_TOKEN")), + Arc::new(GhCliProvider::new(host_id.clone())), + ]; + let credentials = CredentialStore::snapshot(providers).await; + let cred = credentials.get(&host_id).ok_or_else(|| { + anyhow!( + "no github.com credential found. Set GH_TOKEN or run `gh auth login` and try again." + ) + })?; + tracing::info!(source = ?cred.source(), "github.com credential captured"); + + // 2. Build the GitHub adapter. The same `GitHubAdapter` the daemon + // uses — no fork, no parallel implementation. + let github: Arc = Arc::new(GitHubAdapter::github_com( + cred.token().expose().to_string(), + )); + + // 3. Spawn the agent. `agent_command::prepare` is the one + // canonical entry-point that handles PATHEXT on Windows and + // the Copilot SEA-launcher rewrite. Don't call `Command::new` + // directly — that's how this PR's first dogfood found bug. + let (program, agent_args) = agent_command::prepare(&args.agent_program, &args.agent_arg); + let handler = Arc::new(ChunkCollector::default()); + let argv: Vec<&str> = agent_args.iter().map(String::as_str).collect(); + let acp_config = AcpClientConfig { + idle_timeout: Duration::from_secs(300), + ..AcpClientConfig::default() + }; + let client = AcpClient::connect_process( + &program, + &argv, + handler.clone() as Arc, + acp_config, + ) + .await + .context("spawn ACP agent")?; + let init = client.initialize().await.context("initialize ACP agent")?; + let methods: Vec = init.auth_methods.iter().map(|m| m.id.clone()).collect(); + if !methods.is_empty() { + match client.authenticate(&methods).await { + Ok(_) | Err(AcpError::NoAuth) => {} + Err(e) => return Err(anyhow!("authenticate ACP agent: {e}")), + } + } + + // 4. Loop: poll, review, sleep. Track each PR's `pr_state_hash` + // so we only re-review on real changes (head-sha bump or + // metadata edit). Mirrors the daemon's ledger dedup. + let mut seen: HashMap = HashMap::new(); + let cwd = std::env::current_dir()?.to_string_lossy().into_owned(); + loop { + let prs = github + .list_open_prs(&owner, &repo) + .await + .with_context(|| format!("list open PRs for {owner}/{repo}"))?; + tracing::info!(count = prs.len(), "polled open PRs"); + + for pr in &prs { + let hash = pr_state_hash(pr); + if seen.get(&pr.number) == Some(&hash) { + continue; + } + seen.insert(pr.number, hash); + if let Err(e) = review_pr(&client, &handler, &cwd, &owner, &repo, pr).await { + tracing::warn!(pr = pr.number, error = %e, "review failed; will retry on next poll"); + } + } + + if args.once { + return Ok(()); + } + tokio::time::sleep(Duration::from_secs(args.poll_secs)).await; + } +} + +/// Open one ACP session per PR review and stream the agent's reply. +async fn review_pr( + client: &AcpClient, + handler: &Arc, + cwd: &str, + owner: &str, + repo: &str, + pr: &PullRequest, +) -> Result<()> { + println!(); + println!("─── PR {owner}/{repo}#{} — {} ───", pr.number, pr.title); + + let session = client + .new_session(NewSessionParams { + cwd: cwd.to_string(), + mcp_servers: Vec::new(), + }) + .await + .context("session/new")?; + + handler.start_session(session.session_id.clone()).await; + + let prompt = format!( + "Review pull request #{pr_num} in {owner}/{repo}. Use the `gh` CLI to fetch the diff and \ + metadata. Identify any substantive correctness, security, or design issues. Skip style \ + nits. Be terse — one paragraph plus a bulleted issue list, or 'no significant issues' if \ + none. Do not post anything to the PR; this is a read-only review.", + pr_num = pr.number, + ); + let result = client + .prompt(PromptParams { + session_id: session.session_id.clone(), + prompt: vec![PromptContent::Text { text: prompt }], + }) + .await + .context("session/prompt")?; + tracing::debug!(stop_reason = ?result.stop_reason, "prompt completed"); + + let reply = handler.finish_session(&session.session_id).await; + if reply.trim().is_empty() { + println!("(agent returned an empty reply)"); + } else { + println!("{reply}"); + } + Ok(()) +} + +/// Minimal `AcpHandler` that collects every `agentMessageChunk` text +/// fragment into a per-session buffer. All tool/permission/fs hooks +/// reject — Copilot CLI runs its own tools when launched with +/// `--allow-all-tools`, so we never see those callbacks. +#[derive(Default)] +struct ChunkCollector { + buffers: Mutex>, +} + +impl ChunkCollector { + async fn start_session(&self, session_id: String) { + self.buffers.lock().await.insert(session_id, String::new()); + } + + async fn finish_session(&self, session_id: &str) -> String { + self.buffers + .lock() + .await + .remove(session_id) + .unwrap_or_default() + } +} + +#[async_trait] +impl AcpHandler for ChunkCollector { + async fn on_session_update(&self, params: SessionUpdateParams) { + if let SessionUpdate::AgentMessageChunk { content } = params.update { + let mut buffers = self.buffers.lock().await; + if let Some(buf) = buffers.get_mut(¶ms.session_id) { + buf.push_str(&content.text); + } + } + } + + async fn on_permission_request( + &self, + _params: PermissionRequestParams, + ) -> HandlerResult { + Err(unsupported("session/request_permission")) + } + async fn on_terminal_create( + &self, + _params: CreateTerminalParams, + ) -> HandlerResult { + Err(unsupported("terminal/create")) + } + async fn on_terminal_output( + &self, + _params: TerminalOutputParams, + ) -> HandlerResult { + Err(unsupported("terminal/output")) + } + async fn on_terminal_wait( + &self, + _params: WaitForExitParams, + ) -> HandlerResult { + Err(unsupported("terminal/wait_for_exit")) + } + async fn on_terminal_kill(&self, _params: KillTerminalParams) -> HandlerResult<()> { + Err(unsupported("terminal/kill")) + } + async fn on_terminal_release(&self, _params: ReleaseTerminalParams) -> HandlerResult<()> { + Err(unsupported("terminal/release")) + } + async fn on_fs_read(&self, _params: ReadTextFileParams) -> HandlerResult { + Err(unsupported("fs/read_text_file")) + } + async fn on_fs_write(&self, _params: WriteTextFileParams) -> HandlerResult<()> { + Err(unsupported("fs/write_text_file")) + } +} + +fn unsupported(method: &str) -> devdev_acp::protocol::RpcError { + devdev_acp::protocol::RpcError { + code: devdev_acp::protocol::error_codes::METHOD_NOT_FOUND, + message: format!("{method} not supported by pr-reviewer sample"), + data: None, + } +} + +fn parse_repo(s: &str) -> Result<(String, String)> { + let mut parts = s.splitn(2, '/'); + let owner = parts.next().filter(|s| !s.is_empty()); + let repo = parts.next().filter(|s| !s.is_empty()); + match (owner, repo) { + (Some(o), Some(r)) => Ok((o.to_string(), r.to_string())), + _ => bail!("expected `owner/repo`, got `{s}`"), + } +} + +fn init_tracing() { + let filter = tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("pr_reviewer=info,warn")); + let _ = tracing_subscriber::fmt().with_env_filter(filter).try_init(); +} diff --git a/scripts/devdev-secrets.ps1 b/scripts/devdev-secrets.ps1 new file mode 100644 index 0000000..51208d2 --- /dev/null +++ b/scripts/devdev-secrets.ps1 @@ -0,0 +1,51 @@ +# Helper functions for reading DevDev secrets from the local SecretStore vault. +# Dot-source this in other scripts: . "$PSScriptRoot/devdev-secrets.ps1" +# +# Local vault layout (Microsoft.PowerShell.SecretStore, password-less DPAPI): +# gh-app-admin-pem RSA private key (PEM, RSA PKCS#1) +# gh-app-admin-id GitHub App numeric ID +# gh-app-admin-client-id GitHub App client ID (Iv23...) +# gh-app-consumer-pem +# gh-app-consumer-id +# gh-app-consumer-client-id +# ado-sp-admin-client-id (added later) +# ado-sp-admin-tenant-id +# ado-sp-consumer-client-id +# ado-sp-consumer-tenant-id + +Set-StrictMode -Version Latest + +function Get-DevDevSecret { + param([Parameter(Mandatory)][string]$Name) + Import-Module Microsoft.PowerShell.SecretManagement -ErrorAction Stop + Get-Secret -Name $Name -Vault DevDev -AsPlainText -ErrorAction Stop +} + +function New-GitHubAppJwt { + param( + [Parameter(Mandatory)][string]$AppId, + [Parameter(Mandatory)][string]$Pem + ) + $rsa = [System.Security.Cryptography.RSA]::Create() + $rsa.ImportFromPem($Pem) + $now = [DateTimeOffset]::UtcNow.ToUnixTimeSeconds() + function ToB64Url([byte[]]$b) { + [Convert]::ToBase64String($b).TrimEnd('=').Replace('+','-').Replace('/','_') + } + $h = ToB64Url ([Text.Encoding]::UTF8.GetBytes('{"alg":"RS256","typ":"JWT"}')) + $p = ToB64Url ([Text.Encoding]::UTF8.GetBytes("{`"iat`":$($now-30),`"exp`":$($now+540),`"iss`":`"$AppId`"}")) + $signingInput = "$h.$p" + $sig = $rsa.SignData( + [Text.Encoding]::UTF8.GetBytes($signingInput), + [Security.Cryptography.HashAlgorithmName]::SHA256, + [Security.Cryptography.RSASignaturePadding]::Pkcs1) + return "$signingInput." + (ToB64Url $sig) +} + +function Get-GitHubAppJwt { + # Convenience: name = 'admin' or 'consumer' + param([Parameter(Mandatory)][ValidateSet('admin','consumer')][string]$App) + $pem = Get-DevDevSecret -Name "gh-app-$App-pem" + $id = Get-DevDevSecret -Name "gh-app-$App-id" + New-GitHubAppJwt -AppId $id -Pem $pem +} diff --git a/scripts/seed-ci-secrets.ps1 b/scripts/seed-ci-secrets.ps1 new file mode 100644 index 0000000..2a30e8f --- /dev/null +++ b/scripts/seed-ci-secrets.ps1 @@ -0,0 +1,100 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Seed the live-tests CI environments with GitHub App credentials. + +.DESCRIPTION + Idempotent. Reads from the local DevDev SecretStore vault and pushes + to two GitHub Actions Environments on the workflow repo: + + live-tests-admin + var DEVDEV_GH_APP_ADMIN_ID + var DEVDEV_GH_FIXTURE_OWNER + var DEVDEV_GH_FIXTURE_REPO + var DEVDEV_LIVE_ADO_ENABLED = "0" (until ADO is wired) + secret DEVDEV_GH_APP_ADMIN_PEM + + live-tests-consumer + var DEVDEV_GH_APP_CONSUMER_ID + var DEVDEV_GH_FIXTURE_OWNER + var DEVDEV_GH_FIXTURE_REPO + secret DEVDEV_GH_APP_CONSUMER_PEM + + Re-running this script overwrites existing values; safe to run after + rotating PEMs. + +.PARAMETER WorkflowRepo + The repo where live-tests.yml runs. Default: goldenwitch/devdev. + +.PARAMETER FixtureOwner + GitHub account that owns the fixture repo. Default: goldenwitch. + +.PARAMETER FixtureRepo + Fixture repo name. Default: devdev-test-environment. + +.NOTES + Requires `gh` CLI signed in with at least `repo` scope on + $WorkflowRepo. Personal-account repos work with the default scopes + obtained via `gh auth login`. +#> +[CmdletBinding()] +param( + [string]$WorkflowRepo = 'goldenwitch/devdev', + [string]$FixtureOwner = 'goldenwitch', + [string]$FixtureRepo = 'devdev-test-environment' +) + +$ErrorActionPreference = 'Stop' +. "$PSScriptRoot/devdev-secrets.ps1" + +function Ensure-Environment { + param([string]$Repo, [string]$Env) + Write-Host " ensuring environment $Env exists on $Repo..." + $owner, $name = $Repo -split '/', 2 + gh api --method PUT "repos/$owner/$name/environments/$Env" --silent | Out-Null +} + +function Set-EnvVariable { + param([string]$Repo, [string]$Env, [string]$Name, [string]$Value) + Write-Host " var $Env/$Name" + # `gh variable set` upserts; --env scopes to environment. + # NOTE: `--body -` is interpreted as the literal value `-`, not + # as a stdin marker. Pass the value directly via --body. + & gh variable set $Name --env $Env --repo $Repo --body $Value | Out-Null + if ($LASTEXITCODE -ne 0) { throw "gh variable set $Name failed (exit $LASTEXITCODE)" } +} + +function Set-EnvSecret { + param([string]$Repo, [string]$Env, [string]$Name, [string]$Value) + Write-Host " secret $Env/$Name (length=$($Value.Length))" + # `gh secret set` upserts; same caveat as variables (`-` is literal). + # Value is encrypted client-side by gh before transmission. + & gh secret set $Name --env $Env --repo $Repo --body $Value | Out-Null + if ($LASTEXITCODE -ne 0) { throw "gh secret set $Name failed (exit $LASTEXITCODE)" } +} + +Write-Host "Seeding CI secrets/vars on $WorkflowRepo" -ForegroundColor Cyan +Write-Host " fixture: $FixtureOwner/$FixtureRepo" + +# ---- admin ----------------------------------------------------------- +$adminEnv = 'live-tests-admin' +Ensure-Environment -Repo $WorkflowRepo -Env $adminEnv +$adminId = Get-DevDevSecret -Name 'gh-app-admin-id' +$adminPem = Get-DevDevSecret -Name 'gh-app-admin-pem' +Set-EnvVariable -Repo $WorkflowRepo -Env $adminEnv -Name 'DEVDEV_GH_APP_ADMIN_ID' -Value $adminId +Set-EnvVariable -Repo $WorkflowRepo -Env $adminEnv -Name 'DEVDEV_GH_FIXTURE_OWNER' -Value $FixtureOwner +Set-EnvVariable -Repo $WorkflowRepo -Env $adminEnv -Name 'DEVDEV_GH_FIXTURE_REPO' -Value $FixtureRepo +Set-EnvVariable -Repo $WorkflowRepo -Env $adminEnv -Name 'DEVDEV_LIVE_ADO_ENABLED' -Value '0' +Set-EnvSecret -Repo $WorkflowRepo -Env $adminEnv -Name 'DEVDEV_GH_APP_ADMIN_PEM' -Value $adminPem + +# ---- consumer -------------------------------------------------------- +$consumerEnv = 'live-tests-consumer' +Ensure-Environment -Repo $WorkflowRepo -Env $consumerEnv +$consumerId = Get-DevDevSecret -Name 'gh-app-consumer-id' +$consumerPem = Get-DevDevSecret -Name 'gh-app-consumer-pem' +Set-EnvVariable -Repo $WorkflowRepo -Env $consumerEnv -Name 'DEVDEV_GH_APP_CONSUMER_ID' -Value $consumerId +Set-EnvVariable -Repo $WorkflowRepo -Env $consumerEnv -Name 'DEVDEV_GH_FIXTURE_OWNER' -Value $FixtureOwner +Set-EnvVariable -Repo $WorkflowRepo -Env $consumerEnv -Name 'DEVDEV_GH_FIXTURE_REPO' -Value $FixtureRepo +Set-EnvSecret -Repo $WorkflowRepo -Env $consumerEnv -Name 'DEVDEV_GH_APP_CONSUMER_PEM' -Value $consumerPem + +Write-Host "DONE" -ForegroundColor Green diff --git a/scripts/verify-gh-apps.ps1 b/scripts/verify-gh-apps.ps1 new file mode 100644 index 0000000..a62843f --- /dev/null +++ b/scripts/verify-gh-apps.ps1 @@ -0,0 +1,32 @@ +#!/usr/bin/env pwsh +# Verify GitHub App credentials from the local DevDev SecretStore vault. +# Usage: pwsh scripts/verify-gh-apps.ps1 +$ErrorActionPreference = "Stop" +. "$PSScriptRoot/devdev-secrets.ps1" + +foreach ($app in @('admin', 'consumer')) { + Write-Host "=== $app ===" -ForegroundColor Cyan + try { + $jwt = Get-GitHubAppJwt -App $app + $headers = @{ + Authorization = "Bearer $jwt" + Accept = "application/vnd.github+json" + "X-GitHub-Api-Version" = "2022-11-28" + } + $info = Invoke-RestMethod -Uri "https://api.github.com/app" -Headers $headers + Write-Host " slug: $($info.slug)" + Write-Host " owner: $($info.owner.login)" + Write-Host " permissions: $($info.permissions | ConvertTo-Json -Compress)" + $installs = Invoke-RestMethod -Uri "https://api.github.com/app/installations" -Headers $headers + if (-not $installs -or $installs.Count -eq 0) { + Write-Host " installations: NONE" -ForegroundColor Yellow + } else { + foreach ($inst in $installs) { + Write-Host " install id=$($inst.id) account=$($inst.account.login) repo_selection=$($inst.repository_selection)" + } + } + } + catch { + Write-Host " ERROR: $_" -ForegroundColor Red + } +} diff --git a/scripts/verify-gh-e2e.ps1 b/scripts/verify-gh-e2e.ps1 new file mode 100644 index 0000000..3386015 --- /dev/null +++ b/scripts/verify-gh-e2e.ps1 @@ -0,0 +1,92 @@ +#!/usr/bin/env pwsh +# End-to-end exercise of GitHub App credentials against the fixture repo. +# Proves: token mint -> read -> branch create -> PR open -> comment -> comment delete. +# Idempotent: tolerates pre-existing branch / PR by reusing them. +$ErrorActionPreference = "Stop" +. "$PSScriptRoot/devdev-secrets.ps1" + +$Owner = 'goldenwitch' +$Repo = 'devdev-test-environment' +$Base = 'main' +$Branch = 'fixture/canonical' +$PrTitle = 'Canonical fixture PR — DO NOT MERGE' +$TagPrefix = '[devdev-live-test]' + +function New-InstallationToken { + param([string]$App) + $jwt = Get-GitHubAppJwt -App $App + $h = @{ Authorization = "Bearer $jwt"; Accept = 'application/vnd.github+json' } + $installs = Invoke-RestMethod -Uri 'https://api.github.com/app/installations' -Headers $h + if ($installs.Count -ne 1) { throw "expected exactly one install for $App, got $($installs.Count)" } + (Invoke-RestMethod -Method Post -Uri "https://api.github.com/app/installations/$($installs[0].id)/access_tokens" -Headers $h).token +} + +function GhHeaders([string]$Token) { + @{ Authorization = "token $Token"; Accept = 'application/vnd.github+json'; 'X-GitHub-Api-Version' = '2022-11-28' } +} + +Write-Host "[1/7] Minting installation tokens..." -ForegroundColor Cyan +$adminTok = New-InstallationToken -App admin +$consumerTok = New-InstallationToken -App consumer +$adminH = GhHeaders $adminTok +$consumerH = GhHeaders $consumerTok +Write-Host " admin: minted ($($adminTok.Length) chars) [redacted]" +Write-Host " consumer: minted ($($consumerTok.Length) chars) [redacted]" + +Write-Host "[2/7] Consumer reads repo metadata..." -ForegroundColor Cyan +$repoInfo = Invoke-RestMethod -Uri "https://api.github.com/repos/$Owner/$Repo" -Headers $consumerH +Write-Host " $($repoInfo.full_name) default_branch=$($repoInfo.default_branch) visibility=$($repoInfo.visibility)" + +Write-Host "[3/7] Admin ensures '$Branch' branch exists..." -ForegroundColor Cyan +try { + $existing = Invoke-RestMethod -Uri "https://api.github.com/repos/$Owner/$Repo/git/refs/heads/$Branch" -Headers $adminH + Write-Host " branch already exists at $($existing.object.sha)" +} catch { + $sc = $_.Exception.Response.StatusCode.value__ + Write-Host " branch lookup returned $sc, falling back to base lookup" + if ($sc -ne 404) { throw } + $baseUrl = "https://api.github.com/repos/$Owner/$Repo/git/refs/heads/$Base" + Write-Host " GET $baseUrl" + $baseRef = Invoke-RestMethod -Uri $baseUrl -Headers $adminH + Write-Host " base $Base sha: $($baseRef.object.sha)" + $body = @{ ref = "refs/heads/$Branch"; sha = $baseRef.object.sha } | ConvertTo-Json + $created = Invoke-RestMethod -Method Post -Uri "https://api.github.com/repos/$Owner/$Repo/git/refs" -Headers $adminH -Body $body -ContentType 'application/json' + Write-Host " created branch at $($created.object.sha)" +} + +Write-Host "[4/7] Admin ensures FIXTURE.md exists on '$Branch'..." -ForegroundColor Cyan +try { + $cur = Invoke-RestMethod -Uri "https://api.github.com/repos/$Owner/$Repo/contents/FIXTURE.md?ref=$Branch" -Headers $adminH + Write-Host " FIXTURE.md already at sha $($cur.sha)" +} catch { + if ($_.Exception.Response.StatusCode.value__ -ne 404) { throw } + $contents = "# DevDev fixture branch`n`nOwned by devdev-test-env. Reset on each apply.`n" + $b64 = [Convert]::ToBase64String([Text.Encoding]::UTF8.GetBytes($contents)) + $body = @{ message = 'seed FIXTURE.md'; content = $b64; branch = $Branch } | ConvertTo-Json + $put = Invoke-RestMethod -Method Put -Uri "https://api.github.com/repos/$Owner/$Repo/contents/FIXTURE.md" -Headers $adminH -Body $body -ContentType 'application/json' + Write-Host " created FIXTURE.md commit $($put.commit.sha)" +} + +Write-Host "[5/7] Admin ensures canonical PR exists..." -ForegroundColor Cyan +$prList = Invoke-RestMethod -Uri "https://api.github.com/repos/$Owner/$Repo/pulls?state=open&head=${Owner}:${Branch}&base=$Base" -Headers $adminH +if ($prList.Count -gt 0) { + $pr = $prList[0] + Write-Host " PR #$($pr.number) already open: $($pr.title)" +} else { + $body = @{ title = $PrTitle; head = $Branch; base = $Base; body = "Provisioned by devdev-test-env. Do not merge." } | ConvertTo-Json + $pr = Invoke-RestMethod -Method Post -Uri "https://api.github.com/repos/$Owner/$Repo/pulls" -Headers $adminH -Body $body -ContentType 'application/json' + Write-Host " opened PR #$($pr.number)" +} + +Write-Host "[6/7] Consumer posts a tagged comment on PR #$($pr.number)..." -ForegroundColor Cyan +$nonce = [Guid]::NewGuid().ToString('N').Substring(0,8) +$commentBody = "$TagPrefix`:e2e-verify:$nonce`:hello from installation token" +$commentReq = @{ body = $commentBody } | ConvertTo-Json +$comment = Invoke-RestMethod -Method Post -Uri "https://api.github.com/repos/$Owner/$Repo/issues/$($pr.number)/comments" -Headers $consumerH -Body $commentReq -ContentType 'application/json' +Write-Host " comment id $($comment.id) by $($comment.user.login)" + +Write-Host "[7/7] Consumer sweeps the tagged comment..." -ForegroundColor Cyan +Invoke-RestMethod -Method Delete -Uri "https://api.github.com/repos/$Owner/$Repo/issues/comments/$($comment.id)" -Headers $consumerH | Out-Null +Write-Host " deleted" + +Write-Host "`nALL CHECKS PASSED" -ForegroundColor Green diff --git a/test-env/README.md b/test-env/README.md new file mode 100644 index 0000000..ab4df5d --- /dev/null +++ b/test-env/README.md @@ -0,0 +1,21 @@ +# Live-test fixture environment + +This directory is the **declarative source of truth** for the +DevDev live-test fixtures. The `devdev-test-env` binary +([`crates/devdev-test-env/`](../crates/devdev-test-env/)) reads +[`manifest.json`](manifest.json), provisions the fixtures, and +backfills server-assigned ids into [`manifest.lock.json`](manifest.lock.json) +(committed alongside the manifest, just like `Cargo.lock`). + +What lives where: + +| File | Purpose | +|---|---| +| `manifest.json` | Org/project/repo names, canonical PR title/body, fixture branch, comment tag prefix. Hand-edited. | +| `manifest.lock.json` | PR numbers, repo ids, other server-assigned values. Generated by `devdev-test-env apply`; committed. | + +The bootstrap walkthrough, principal/secret roster, and cost shape +live in [`docs/internals/live-test-fixtures.md`](../docs/internals/live-test-fixtures.md). + +The GitHub Enterprise gap and how a sponsor could close it lives in +[`docs/internals/ghe-gap.md`](../docs/internals/ghe-gap.md). diff --git a/test-env/manifest.json b/test-env/manifest.json new file mode 100644 index 0000000..50e7374 --- /dev/null +++ b/test-env/manifest.json @@ -0,0 +1,34 @@ +{ + "github": { + "org": "goldenwitch", + "repo": "devdev-test-environment", + "default_branch": "main", + "fixture_branch": "fixture/canonical", + "canonical_pr": { + "title": "Canonical fixture PR — DO NOT MERGE", + "body": "Provisioned by `devdev-test-env`. This PR is the canonical target for DevDev's live tests. Do not merge, rename, or close. Stray comments left here will be swept by the post-test cleanup hook.\n\nIf you arrived here by accident: see `docs/internals/live-test-fixtures.md`.", + "base": "main", + "fixture_files": [ + { + "path": "FIXTURE.md", + "contents": "# DevDev fixture branch\n\nThis branch is owned by `devdev-test-env`. The contents of every file under this branch are reset to manifest values on each `apply`. Do not commit anything here you want to keep.\n" + } + ] + }, + "comment_tag_prefix": "[devdev-live-test]" + }, + "azure_devops": { + "org": "devdev-fixtures", + "project": "DevDev-Live", + "repo": "devdev-test-environment", + "default_branch": "main", + "fixture_branch": "fixture/canonical", + "canonical_pr": { + "title": "Canonical fixture PR — DO NOT MERGE", + "body": "Provisioned by `devdev-test-env`. See `docs/internals/live-test-fixtures.md`.", + "base": "main", + "fixture_files": [] + }, + "comment_tag_prefix": "[devdev-live-test]" + } +} diff --git a/test-env/manifest.lock.json b/test-env/manifest.lock.json new file mode 100644 index 0000000..2a6a044 --- /dev/null +++ b/test-env/manifest.lock.json @@ -0,0 +1,8 @@ +{ + "github": { + "repo_id": 1228332594, + "canonical_pr_number": 1, + "canonical_pr_node_id": "PR_kwDOSTbeMs7X1eoR" + }, + "azure_devops": null +}