From da304aa9d7f48c1e0d0b1aee9ab4e5125a3bcfa1 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Mon, 22 Jun 2026 16:19:09 -0400 Subject: [PATCH] docs(agent-team): fold GPT-4.1 plan-review findings into the P3-live-flip plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REQUEST CHANGES from the cross-family plan-review (2026-06-22), dispositioned: - expanded denylist (§4.2): submodules/.gitmodules, git hooks, .gitattributes filters, lockfile postinstall, generated artifacts - runner-trust assertion (privileged jobs GitHub-hosted only) - concrete diff-transport spec + threat model (signed artifact / branch-only token, nonce anti-replay) - gate-weakening detection (noqa/skip/excludes/--no-verify) - PR-metadata secret sanitization; ledger diff-hash anti-tamper - Phase 1b: recovery for an accidentally-merged/applied privileged change + draft-PR rate monitoring + stale-PR cleanup - required-check-name discovery; deploy-before-merge enforcement; no-write-token audit Notes which BLOCK items are already implemented in PR #17's CI (Phase 1 verifies, not rebuilds). --- docs/provisioning/P3-LIVE-FLIP-PLAN.md | 91 +++++++++++++++++++++----- 1 file changed, 76 insertions(+), 15 deletions(-) diff --git a/docs/provisioning/P3-LIVE-FLIP-PLAN.md b/docs/provisioning/P3-LIVE-FLIP-PLAN.md index 793091d..781c078 100644 --- a/docs/provisioning/P3-LIVE-FLIP-PLAN.md +++ b/docs/provisioning/P3-LIVE-FLIP-PLAN.md @@ -45,21 +45,58 @@ The box never gains a write token. The flip does NOT proceed until `/sh-security-review` AND the GPT-4.1 cross-review on the CI surface both pass. +> **Plan-review disposition (GPT-4.1 cross-family, 2026-06-22 — REQUEST CHANGES).** Findings +> folded into §4 and Phases 1/1b: expanded denylist vectors, runner-trust, concrete +> diff-transport + threat model, gate-weakening detection, PR-metadata sanitization, ledger +> anti-tamper, recovery for a merged-privileged change, required-check-name discovery, +> deploy-before-merge enforcement, no-write-token audit, draft-PR rate monitoring + stale-PR +> cleanup. Several items the reviewer marked BLOCK are **already implemented in PR #17's CI** +> (canonicalization, egress, SHA-pin, empty-hash fail-closed, Checks-API) — Phase 1 verifies +> them rather than rebuilding. Open QUESTIONs to answer when building: how human reviewers are +> notified of new draft PRs, and how "every deployable repo has CI" is enforced for targets. + ## 4. The CI trust boundary (design B4 — what the workflow must enforce) +> **Already implemented in the merged P3-live CI hardening (PR #17) — Phase 1 VERIFIES, does not rebuild:** +> denylist path **canonicalization + symlink/rename/traversal resistance**, **egress +> restriction** on the untrusted job, **SHA-pinned** actions, **empty/missing-hash +> fail-closed**, and **authenticated Checks-API** result consumption. The GPT-4.1 +> plan-review (2026-06-22) flagged these as "missing" because the *plan* under-restated +> them; confirm each against the actual `agent-team/ci/agent-team-apply-verify.yml` + +> `ci_fetcher.py`/`ci_gate.py` rather than re-authoring. + 1. **Split CI.** An **untrusted build/test job**: `contents: read` only, **no secrets / no - OIDC / no write token**, egress-restricted. A **separate privileged job** that **never - checks out the patch code** (no `pull_request_target` + head checkout) opens the draft PR. -2. **Denylist.** A diff touching `.github/workflows/**`, IAM/permission IaC, - branch-protection / `CODEOWNERS` / Dependabot config, or out-of-scope files is - **rejected / escalated to a human — never auto-built**. -3. **Diff-hash integrity.** The box records the diff hash in its ledger; CI verifies the - hash before apply. The diff reaches CI as a signed artifact / short-lived branch-only - token (the box has no write token). + OIDC / no write token**, **egress-restricted** (verify/audit the restriction, don't just + assert it). A **separate privileged job** that **never checks out the patch code** (no + `pull_request_target` + head checkout) opens the draft PR. **Privileged jobs MUST run only + on GitHub-hosted runners — assert no self-hosted/user-provided runner can pick them up.** +2. **Denylist** (reject/escalate, never auto-build). Beyond `.github/workflows/**`, + IAM/permission IaC, branch-protection / `CODEOWNERS` / Dependabot config, and + out-of-scope files, the denylist MUST also cover these RCE/priv-esc vectors: + `.gitmodules` / submodule changes, **git hooks** (`.git/hooks`, `core.hooksPath`, + `.husky/**`), **`.gitattributes`** (filter/clean-smudge process), lockfiles + + package-manager **postinstall/preinstall** hooks, and **generated/build-artifact** files + (codegen output is not reviewable as source). Path matching is **canonicalized** (PR #17) + so symlinks/renames/traversal can't slip a denied path past. +3. **Diff-transport integrity (concretely specified, threat-modeled).** The box records the + diff **content hash** in its ledger; CI **verifies the hash before apply**. The diff + reaches CI as a **content-addressed signed artifact** (HMAC/keyed digest the box and the + privileged job share via an Actions secret) **OR** a short-lived, **branch-only** token + scoped to a single ref — the box never holds a write token. Threat model the path: + tamper-in-transit (defeated by hash+signature verify), replay of an old diff (defeated by + per-task nonce + the `status='open'`/one-shot ledger state), and a hostile artifact name. 4. **Pure-code green gate.** Pass/fail is owned by a **pure-code gate** reading **authenticated Checks-API results** (run id + diff hash). The **LLM verifier may propose fixes but can never declare a build green**. -5. **Merge gate.** Draft PR + required checks + `/sh-security-review` + Claude Code App +5. **Gate-weakening detection.** A diff that **lowers a gate** — adds `# noqa`, `# type: ignore`, + test `skip`/`xfail`, coverage/lint **excludes**, `--no-verify`, or edits the gate config + itself — is flagged and escalated (a build can't make itself pass by disabling the checks). +6. **PR-metadata sanitization.** The draft-PR **title / body / comments** are sanitized so a + hostile diff or LLM output can't exfiltrate secrets/env or inject content into the PR text. +7. **Ledger anti-tamper.** The diff-hash ledger entry is integrity-protected (the existing + atomic-write + integrity-check substrate; verify the hash row can't be silently rewritten + between record and apply). +8. **Merge gate.** Draft PR + required checks + `/sh-security-review` + Claude Code App review + **human approval**. ## 5. Phases @@ -70,15 +107,39 @@ the CI surface both pass. - [ ] Resolve `GH_TOKEN`→`GITHUB_TOKEN` (transport accepts both / box env updated). - **Rollback:** none (no state changed). -### Phase 1 — Author the split-CI apply/verify workflow 🤖 (review-gated) -- [ ] Write `agent-team/ci/agent-team-apply-verify.yml` per §4: split jobs, denylist, - diff-hash verification, pure-code gate reading Checks-API. **SHA-pin all actions.** -- [ ] Implement/confirm `agent_team/ci_fetcher.py` (read-only Checks-API result fetcher; - fails closed: missing token / 404 / auth fail → `None` → gate BLOCKs, task parks) - and `agent_team/ci_gate.py` (pure-code green decision). +### Phase 1 — Author/verify the split-CI apply/verify workflow 🤖 (review-gated) +- [ ] Reconcile `agent-team/ci/agent-team-apply-verify.yml` with §4. **First confirm** the + PR-#17 controls are present (canonicalized denylist, egress restriction, SHA-pins, + empty-hash fail-closed, Checks-API consumption); only then add the new §4 items. +- [ ] **Add denylist vectors** (§4.2): submodules/`.gitmodules`, git hooks/`core.hooksPath`/`.husky`, + `.gitattributes` filters, lockfile postinstall/preinstall, generated/build artifacts. + Add a test suite proving canonicalization resists symlink/rename/traversal. +- [ ] **Runner-trust assertion** (§4.1): test that privileged jobs cannot run on a + self-hosted/user-provided runner. +- [ ] **Concretize + threat-model the diff transport** (§4.3): pick content-addressed signed + artifact (shared HMAC secret) or short-lived branch-only token; add per-task nonce + anti-replay; document and test it. +- [ ] **Gate-weakening detector** (§4.5): CI step that fails on a diff adding + `noqa`/`type: ignore`/skip/xfail/excludes/`--no-verify` or editing the gate config. +- [ ] **PR-metadata sanitization** (§4.6) and **ledger anti-tamper** (§4.7) implemented + tested. +- [ ] `agent_team/ci_fetcher.py` (read-only Checks-API fetcher; fails closed) + + `ci_gate.py` (pure-code green). Add a mechanism for the gate to **discover the correct + required check names per repo/branch** (avoid hardcoded check-name drift across repos). +- [ ] **Deploy-before-merge enforcement:** a documented/CI gate ensuring the privileged flip + is exercised on the box before the workflow change is merged (Sea Haven deploy-then-merge). +- [ ] **Concrete "no write token on the box" audit** (a test/script, not just a claim). - [ ] `/sh-security-review` + GPT-4.1 cross-review on this surface. **Hard stop until both pass.** - **Rollback:** workflow file stays inert (`if: ${{ false }}` not yet flipped); delete the file. +### Phase 1b — Recovery for an accidentally-merged/applied privileged change 🤖/🧑 +- [ ] Document + **exercise once** a rollback for the case where a privileged change (the + apply/verify workflow, the `agent-apply` environment, the GitHub App perms, or + branch-protection) is merged or applied in error: revert the SHA, rotate the GitHub App + token, restore branch-protection/environment from a recorded baseline, and confirm no + draft-PR apply ran in the window. (The plan previously only covered reverting inert files.) +- [ ] Add light **monitoring on draft-PR creation rate** (runaway-volume alarm) and an + **orphaned/stale draft-PR cleanup** step. + ### Phase 2 — Provision the GitHub App + environment 🧑 OPERATOR (browser/admin) - [ ] Create a dedicated **GitHub App** with **`pull-requests:write`** (+ minimal contents to open a branch/PR); install on the org. Token lives in **CI**, never on the box.