From 27721f9f45ded2b7e464c47da7c0ea3585ef3038 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:42:27 +0800 Subject: [PATCH] docs(rfc): record real-API e2e CI decision + security model Adds docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md covering the rationale for running the real-API e2e suite in a separate secret-consuming workflow, the fork/Dependabot/secret threat model, the residual exposure of the pull_request trigger, and what changes when the repo goes public. Indexes it in the RFC README. Also adds a SECURITY comment on the pull_request trigger forbidding a switch to pull_request_target (an untrusted-code-with-secrets leak vector, especially once public), pointing at the RFC. --- .github/workflows/e2e.yml | 8 ++ docs/rfc/README.md | 1 + .../implemented/2026-06-19-real-api-e2e-ci.md | 95 +++++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index b5ff4a4231..2de3a6cea6 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -17,6 +17,14 @@ name: E2E (real DeepSeek API) # job skipped by a job-level `if:` reports as a SUCCESSFUL check, so this # workflow is safe to use as a required status check if desired. # +# SECURITY — NEVER change this trigger to `pull_request_target`. `pull_request` +# checks out and runs the PR's HEAD code WITHOUT secrets for forks, which is what +# keeps an untrusted fork from exfiltrating the key. `pull_request_target` runs +# in the BASE repo's context WITH secrets while still able to check out untrusted +# fork code — a textbook key-leak vector, especially once this repo is public. +# The fork/secret model and its public-repo implications are recorded in +# docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md. +# # Note: scheduled triggers are auto-disabled after 60 days of repo inactivity; # push/pull_request/workflow_dispatch act as backstops. on: diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 62d625cc84..385885b610 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -59,6 +59,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [pnpm as the package manager instead of Yarn 4](implemented/2026-06-16-pnpm-over-yarn.md) | 2026-06-16 | | [Rich ACP bash rendering — the terminal card (`_meta`) and command classification](implemented/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 | | [ACP snapshot tests — record-once / replay-deterministic](implemented/2026-06-19-acp-snapshot-tests.md) | 2026-06-19 | +| [Real-API e2e in CI against the external DeepSeek API](implemented/2026-06-19-real-api-e2e-ci.md) | 2026-06-19 | ## Rejected diff --git a/docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md b/docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md new file mode 100644 index 0000000000..ed52c9f23c --- /dev/null +++ b/docs/rfc/implemented/2026-06-19-real-api-e2e-ci.md @@ -0,0 +1,95 @@ +# RFC: Real-API e2e in CI against the external DeepSeek API + +Status: implemented (accepted 2026-06-19) + + + +## Context + +The harness leans hard on real-API tests by policy: AGENTS.md § Secrets argues that a no-key suite proves the plumbing but not the product, and the [ACP inject postmortem](../../postmortem/0001-acp-default-export-drops-inject.md) is the standing proof — 178 keyless tests stayed green while a real editor session crashed instantly. The real-API e2e suite (`pnpm run test:e2e`, the `*.e2e.ts` files) exists precisely to close that gap: it drives the agent against the live DeepSeek API — real model calls, real bash tools, multi-turn, resume, ACP-over-stdio. + +But until this change **nothing in CI ran it**. The default gate ([.github/workflows/ci.yml](../../../.github/workflows/ci.yml)) is deliberately keyless — it carries no secret, runs on every push and PR including from forks, and stays green for any contributor. `test:e2e` self-skips without a key (`describe.skipIf(!process.env.DEEPSEEK_API_KEY)`), so even if ci.yml invoked it, a keyless runner would skip it green. The real-API safety net therefore only fired when a developer happened to run it locally with a key in their environment — i.e. unreliably, and never as a merge gate. + +This RFC records the decision to add a **second, secret-consuming workflow** that runs the real-API suite in CI, and — because introducing the first CI secret into a repo that may later go public is a security/isolation decision — the threat model it relies on and what changes when the repo becomes public. + +## Decision + +Add a dedicated workflow, [.github/workflows/e2e.yml](../../../.github/workflows/e2e.yml), separate from ci.yml. It runs only `pnpm run test:e2e` against the external API using a repo secret, on trusted events, with a preflight that converts a missing secret into a loud failure instead of a false green. ci.yml is left untouched. + +### A separate workflow, not a job in ci.yml + +ci.yml's value is that it is keyless, forkable, and always-green: any contributor (including an outside fork) gets a complete keyless signal with no secret in the blast radius. Adding a secret-consuming job there would couple that always-green gate to credential availability and a different trigger policy. Keeping the secret-bearing work in its own file isolates the secret, trigger, and concurrency policy, and preserves ci.yml's property for forks. Different lifecycles → different files. + +### Cost is not the constraint; reliability is + +The usual reason to ration real-API CI — token cost — does not apply here: we are DeepSeek and internal inference is effectively free. So the design optimizes for *coverage and signal*, not for minimizing calls. The suite runs in full (all six `*.e2e.ts` files), on multiple triggers, on every trusted PR. This is the CI embodiment of the AGENTS.md "lean on with-key e2e tests" policy. + +### Triggers: trusted events only + +`workflow_dispatch` + `push` to `main`/`master` + nightly `schedule` (`17 0 * * *`, 08:17 Asia/Shanghai) + `pull_request`. Push gives a post-merge signal; schedule catches drift in the external API itself even with no commits; dispatch is the manual escape hatch; `pull_request` gives a pre-merge gate. The user explicitly chose to include PR runs for the pre-merge signal, accepting the larger key-exposure surface that implies (see § Security). + +### The untrusted-PR gate + +GitHub withholds repo secrets from two kinds of PR: those from **forks**, and **Dependabot** PRs (same-repo branch, so `head.repo.fork == false`, but secrets are still withheld). A job-level `if:` skips the whole job for both: + +``` +github.event_name != 'pull_request' + || !(github.event.pull_request.head.repo.fork || github.event.pull_request.user.login == 'dependabot[bot]') +``` + +The Dependabot clause keys on the PR **author** (`pull_request.user.login`), not `github.actor` (the run trigger): a maintainer who reopens or re-runs a Dependabot PR would make `github.actor` a human while the PR is still keyless, and an author-based test stays correct across that. A job skipped by a **job-level** `if:` reports as a *successful* check (unlike a workflow/trigger-level skip, which stays pending), so this workflow is safe to mark as a required status check if desired — a fork/Dependabot PR's skipped-but-green check does not block the merge. + +The gate is a *clean-skip nicety*, not the secret's security boundary (see § Security — the boundary is GitHub's own fork-secret withholding under `pull_request`). Without the gate, forks still could not read the key; they would just hit a confusing preflight hard-fail and waste compute. + +### Preflight: fail loud, never false-green + +Because the job only runs on trusted events where the secret is expected, the preflight is an unconditional presence check: empty key → `exit 1` with a `::error::` annotation naming the secret to configure. This is the crux that makes a self-skipping suite safe to gate on. Without it, a deleted/renamed/misconfigured secret would make `test:e2e` skip every real suite and report all-green — a silent regression of the entire safety net. The guard turns "secret missing" from an invisible false pass into a visible failure. (Its correctness was verified live: the run before the secret existed failed at exactly this step.) + +### Secret mapping and hygiene + +The repo secret is named `DEEPSEEK_API_KEY_EXTERNAL`; it is mapped to the `DEEPSEEK_API_KEY` env var the adapters and tests read (`process.env.DEEPSEEK_API_KEY`). The distinct secret name documents intent (this is the *external* public-API key, not an internal-endpoint key) and lets an internal-endpoint key coexist later without collision. Hygiene choices, each defensive: + +- **Step-scoped secret.** `DEEPSEEK_API_KEY` is set in the `env:` of only the preflight and e2e steps, never job-level — so checkout/setup-node/install never see it. A compromised install-time lifecycle script in a dependency cannot read a secret that isn't in its environment. +- **`permissions: contents: read`.** The job only reads the repo to run tests; it needs no write scopes (no PR comments, no status writes), so the `GITHUB_TOKEN` is dropped to least privilege. +- **`DEEPSEEK_BASE_URL` pinned** to `https://api.deepseek.com` on the e2e step. The adapter would default to this when unset ([packages/llm-deepseek/src/index.ts](../../../packages/llm-deepseek/src/index.ts) `PUBLIC_BASE_URL`), but pinning is self-documenting and hermetic — a stray repo-root `.env` (which `vitest.e2e.config.ts` loads if present) cannot silently redirect the run to another endpoint. +- **No secret echoed.** The preflight prints only `DEEPSEEK_API_KEY present.` — not the value, not its length. (An earlier draft echoed `${#KEY}`; dropped as needless metadata.) + +### Scope, runtime shape + +Run **only** `test:e2e`. The keyless gates (typecheck/lint/coverage/snapshot/build/hygiene) already run in ci.yml on every push and PR; repeating them here would duplicate signal and slow the real-API job. No build step — e2e tests run unbuilt via tsx + the tsconfig paths map. Single Node 24 (the `engines` floor): these tests exercise *API integration*, not node-version compat, which ci.yml's `[24, 26]` matrix already owns; a second Node version would double real-API calls for no added signal. `timeout-minutes: 45` bounds a wedged run given serial files (`fileParallelism: false`), 120s/test, and `retry: 2`. `cancel-in-progress` is enabled only for `pull_request` runs — a superseded PR run is on a stale commit and worth cancelling, whereas a push/schedule run is already producing the post-merge/nightly signal and is never cancelled. + +## Security + +Introducing the first CI secret is the part of this change that warrants a recorded threat model, because the natural question — *"can anyone who opens a PR steal the key?"* — has a non-obvious answer, and the answer shifts when the repo goes public. + +### Who can reach the secret today (private repo) + +- **No write access (fork PRs): cannot.** Two independent facts block it. First, the workflow uses `pull_request`, **not** `pull_request_target` — GitHub does not pass repo secrets to fork-PR runs of `pull_request`, so `secrets.DEEPSEEK_API_KEY_EXTERNAL` resolves to empty on a fork runner. Second, the `if:` gate skips fork PRs entirely. The withholding is the real boundary; the gate is defense-in-depth and UX. +- **Write (push) access: can.** A same-repo branch PR receives secrets, so a write-access author could modify test code (or an install lifecycle script, or the workflow YAML on their branch) to exfiltrate the key. This is **inherent to GitHub Actions, not introduced here**: anyone with push access to any repo can already exfiltrate any of its Actions secrets by authoring a workflow. Write access ⇒ secret access, always. The mitigation lives in who is granted write and in branch protection, not in this file. + +So "everyone who could open a PR can steal it" is false: only the write-access set can, and that set could already steal any secret the repo holds. + +### The residual exposure the `pull_request` trigger adds + +Because PR runs are enabled, the key is handed to **the code on a write-access author's PR branch** — code under review, not yet merged — which is a strictly larger surface than `push`-to-main + `schedule` + `workflow_dispatch` alone (where the key only ever touches already-merged or manually-dispatched code). This was the explicit round-1 tradeoff: the pre-merge real-API gate is worth it for a trusted internal write set and a low-value (internal, free) key. If that calculus changes, the hardening is one line — drop the `pull_request` trigger — keeping post-merge + nightly + on-demand coverage. + +### What changes when the repo goes public + +The secret stays protected from the public **through this workflow**: `pull_request` behaves identically on a public repo — fork PRs (now openable by anyone) still receive no secret, and on public repos GitHub additionally gates fork-PR runs behind maintainer approval, where even an approved run gets no secret (approving the run is not the same as handing over the key). The write-access set is unchanged by visibility, so the insider reality is also unchanged. + +What gets worse is the *surrounding* model, and these are the things to address before flipping visibility: + +- **Logs become world-readable.** A careless secret echo that today leaks to org members would leak to the entire internet and be scraped within minutes. Secret-handling discipline (no value/length echoes — already done) matters far more. +- **The `pull_request_target` footgun becomes catastrophic.** If anyone ever "fixes" PR runs by switching the trigger to `pull_request_target`, the workflow would run untrusted fork code in the base-repo context **with** secrets — a full key-leak vector. This is benign-ish on a private repo and disastrous on a public one. A `SECURITY —` comment on the trigger in e2e.yml forbids the change and points here. +- **Rotate on flip.** The key lived in a private repo's CI; treat going-public as "assume exposed" and rotate `DEEPSEEK_API_KEY_EXTERNAL` at that moment. +- **Settle the secret behind controls.** Confirm Settings → Actions → *"Send secrets to workflows from fork pull requests"* stays **off** (the one setting that would actually break the fork boundary), and consider moving the key into a GitHub **Environment** with required reviewers so even merged code uses it only under controlled conditions and rotation has a single home. + +None of these require changing the workflow to go public; they are operational steps plus the already-added `pull_request_target` guard comment. + +## Consequences + +A second CI workflow and the first repo secret to maintain. The real-API suite now gates merges (pre-merge on trusted PRs, post-merge on the main branch) and runs nightly, so a real break in the agent's interaction with the external API surfaces in CI rather than only in a developer's local run — at the cost of real (but internally free) API calls on every trusted PR and merge. The preflight makes secret misconfiguration self-announcing instead of silently disabling the net. + +The design carries a documented constraint surface: the `pull_request` trigger's key-exposure tradeoff (drop it to harden), the `if:` gate's dependence on the author-based Dependabot test, and the hard prohibition on `pull_request_target`. The going-public checklist above is the operational companion — this RFC is the place a future maintainer should re-read before changing the trigger set or flipping repo visibility, rather than re-deriving the fork/secret model from scratch. + +The scheduled trigger auto-disables after 60 days of repo inactivity (a GitHub behavior); push/PR/dispatch are backstops, and an active monorepo will not hit it. Runner egress to `https://api.deepseek.com` is assumed — GitHub-hosted `ubuntu-latest` has it; an egress-restricted self-hosted runner would need connectivity confirmed before relying on the nightly.