diff --git a/docs/howto/ingest-ecosystem-hardening-fixes.md b/docs/howto/ingest-ecosystem-hardening-fixes.md new file mode 100644 index 000000000..7c24999af --- /dev/null +++ b/docs/howto/ingest-ecosystem-hardening-fixes.md @@ -0,0 +1,202 @@ +--- +title: How to ingest the ecosystem runner-hardening fixes (PR #131 batch) +description: > + The downstream done-gate for the PR #131 runner-hardening batch: once the + upstream P2 (#1018), P3 (#1025) and P5 (#1024) fixes land in amplihack-rs, + bump Simard's amplihack-agent-eval git-rev pin to the audited SHA, refresh + Cargo.lock, and re-verify — so the fixes Simard ships actually run in her own + build. P1 (recipe-runner) and P4/P6 are ops/merge escalations, not rev-bumps. +last_updated: 2026-07-26 +review_schedule: as-needed +owner: simard +doc_type: how-to +status: design — not yet implemented +related: + - ../reference/ecosystem-hardening-pr131.md + - ../howto/self-maintain-dependency-pins.md + - ../reference/amplihack-pin-bump-2626.md + - ../reference/dependency-trust-policy.md + - ../reference/supply-chain-audit.md + - ../howto/route-a-goal-to-its-target-repo.md + - ../howto/triage-stale-pull-requests.md + - ../safe-self-update.md +--- + +# How to ingest the ecosystem runner-hardening fixes (PR #131 batch) + +> **Status: design — not yet implemented.** This guide specifies the **only** +> action the `rysweet/Simard` checkout will take for the +> [PR #131 runner-hardening batch](../reference/ecosystem-hardening-pr131.md): +> ingesting the upstream fixes by rev-bump **once they land**. As of this +> writing the P2/P3/P5 fixes are **not yet merged** upstream, so the rev-bump +> is not yet actionable. The code fixes themselves live upstream in +> `amplihack-recipe-runner` and `amplihack-rs` and are **not** editable here. + +Use this when the upstream P2 (#1018), P3 (#1025), and/or P5 (#1024) fixes have +**merged** to `amplihack-rs` `main` and you need them running in Simard's own +build. It is a concrete instance of the reactive done-gate in +[How to keep Simard's dependency pins up to date](./self-maintain-dependency-pins.md), +worked exactly like [amplihack pin bump (#2626)](../reference/amplihack-pin-bump-2626.md). + +## What is (and is not) a rev-bump + +| Problem | Repo | Action here | +| --- | --- | --- | +| **P2** #1018 (version derivation) | `amplihack-rs` | ✅ ingest via `amplihack-agent-eval` rev-bump once landed | +| **P3** #1025 (graceful reflect stop) | `amplihack-rs` | ✅ ingest via `amplihack-agent-eval` rev-bump once landed | +| **P5** #1024 (subscriber lifecycle) | `amplihack-rs` | ✅ ingest via `amplihack-agent-eval` rev-bump once landed | +| **P1** PR #131 (Repo Guardian) | `amplihack-recipe-runner` | ❌ **not** a Simard rev-bump — ops secret rotation + upstream probe | +| **P4** #1015 / **P6** backlog | `amplihack-rs` / `Simard` | ❌ **escalations** — merge steward, not a rev-bump | + +> **P1 is not ingested here.** The `Repo Guardian` blocker is an expired +> `ANTHROPIC_API_KEY` (infra 401). Its fix is a **secret rotation** (ops) plus a +> liveness probe in the recipe-runner workflow — neither is a Simard dependency. +> See the [batch reference, P1](../reference/ecosystem-hardening-pr131.md#p1--repo-guardian-credential-liveness--e2big-child-env-allow-list). + +## Preconditions (gate the bump) + +Do **not** bump until **all** of these hold: + +1. The target fix (P2 / P3 / P5) has **merged** to `amplihack-rs` `main` with all + required checks green. +2. You have the exact **40-char merged SHA** — an **audited** commit, not a + moving branch ref. Bumping to a branch would ingest unrelated/unaudited + commits (see the [supply-chain risk](../reference/ecosystem-hardening-pr131.md#security-considerations)). +3. For P2 specifically, its fix cannot land until the amplihack-rs build/Test + checks are green — which is what P2 itself unblocks (#1022 / #1007 pattern). + +## Steps + +### 1. Confirm the fix is on `main` and get the audited SHA + +```bash +# The merged SHA for the target fix (example: P3 / #1025). +gh pr view --repo rysweet/amplihack-rs \ + --json mergeCommit,mergedAt,state --jq '{state, mergedAt, sha: .mergeCommit.oid}' + +# Cross-check it is reachable from main HEAD (audited, not a stray ref). +gh api repos/rysweet/amplihack-rs/compare/...main --jq '{status, behind: .behind_by}' +# status: "identical" or "behind" (SHA is an ancestor of main) — never "diverged". +``` + +### 2. Bump the pin in `Cargo.toml` + +`amplihack-agent-eval` is Simard's pin on `amplihack-rs`. Re-point its `rev` to +the audited SHA: + +```bash +grep -nE 'amplihack-agent-eval.*rev = "[0-9a-f]{40}"' Cargo.toml +# Edit the rev = "…" value to the audited SHA from step 1. +``` + +### 3. Refresh only that crate in the lock + +```bash +cargo update -p amplihack-agent-eval # scoped: no unrelated churn +``` + +`Cargo.lock` must record the identical SHA. `version` fields are never +hand-edited — they update from the rev bump automatically. + +### 4. Build and test against the ingested fix + +```bash +cargo build --release # or: scripts/cargo-low-space build +cargo test +``` + +A bump that does not build/test clean is **rolled back, not shipped**. If a +call-site broke, fix it **forward** to the new API — never add a fallback shim +(a silent fallback is a silent failure this repo treats as a defect). + +### 5. Re-verify the supply chain + +```bash +cargo deny --locked check # advisories + licenses + bans + sources +cargo audit # RUSTSEC +cargo vet --locked # transitive trust +``` + +The `[sources]` allowlist is unchanged (same `amplihack-rs.git` remote), so these +stay green. + +### 6. Open the bump PR against `rysweet/Simard` + +Follow the shared +[bump-PR convention](./self-maintain-dependency-pins.md#bump-pr-conventions-and-de-duplication) +— one PR per upstream repo, keyed on the repo not the crate: + +| Field | Value | +| --- | --- | +| Branch | `chore/bump-amplihack-rs-pin` | +| Title | `chore(deps): bump amplihack-rs pin to (ingest #1018/#1025/#1024)` | +| Base | `rysweet/Simard` `main` | +| Body | Names each ingested upstream issue/PR and the audited SHA | + +Check for an existing open bump PR for `amplihack-rs` first and **update** it +rather than opening a duplicate: + +```bash +gh pr list --repo rysweet/Simard --state open --head chore/bump-amplihack-rs-pin \ + --json number,title,headRefName +``` + +The bump PR rides the normal +[PR-finalization pipeline](../reference/pr-finalization-pipeline.md): **all +required CI green before merge**, no `--no-verify`, no `--admin`. + +## Done-gate + +The ingestion is "done" **only** once: + +1. `Cargo.toml` `amplihack-agent-eval` rev == the audited `amplihack-rs` SHA. +2. `Cargo.lock` records that same SHA. +3. `cargo build --release` and `cargo test` pass. +4. Supply-chain jobs are green. +5. The bump PR has **merged** to `rysweet/Simard` with all required CI green. + +Rolling the merged bump into the **running daemon** is the operator's step, via +[Safe Self-Update](../safe-self-update.md) (`simard safe-update`) — not required +for this ingestion's goal to report done. + +## Handle the escalations (P4 / P6) + +These are **not** rev-bumps — route them, do not ingest: + +- **P4 (`amplihack-rs` #1015):** a green, MERGEABLE PR → merge steward. See + [Triage stale pull requests](./triage-stale-pull-requests.md). +- **P6 (Simard backlog):** 16 green PRs vs `per-cycle launch cap reached` → + delivery steward and/or raise the per-cycle launch cap. Partially relieved once + P3 lands (green runs stop reflecting, freeing launch slots). See + [Review Overseer workstream gaps](./review-overseer-workstream-gaps.md). + +## Verify end-to-end + +```bash +# 1. Pin equals the audited upstream SHA and is not behind an unaudited main. +PINNED=$(grep 'amplihack-agent-eval' Cargo.toml | grep -oE '[0-9a-f]{40}') +gh api repos/rysweet/amplihack-rs/compare/$PINNED...main --jq '{status, behind: .behind_by}' + +# 2. Lock agrees with the manifest. +grep -A3 'name = "amplihack-agent-eval"' Cargo.lock # source …# + +# 3. Build, test, supply-chain all green. +cargo build --release && cargo test +cargo deny --locked check && cargo audit && cargo vet --locked + +# 4. At most one open amplihack-rs bump PR. +gh pr list --repo rysweet/Simard --state open --search 'in:title "bump amplihack-rs pin"' +``` + +## Related reading + +- [Batch reference: Ecosystem runner-hardening (PR #131)](../reference/ecosystem-hardening-pr131.md) — + the finished-state spec for all six problems. +- [How to keep Simard's dependency pins up to date](./self-maintain-dependency-pins.md) — + the reactive done-gate and proactive reconcile this ingestion instantiates. +- [amplihack pin bump to upstream main (#2626)](../reference/amplihack-pin-bump-2626.md) — + a worked prior instance of the same rev-bump. +- [How to route a goal to its target repo](./route-a-goal-to-its-target-repo.md) — + why the upstream code fixes belong upstream, and the bump belongs here. +- [Safe Self-Update](../safe-self-update.md) — the operator step that rolls a + merged bump into the running daemon. diff --git a/docs/reference/ecosystem-hardening-pr131.md b/docs/reference/ecosystem-hardening-pr131.md new file mode 100644 index 000000000..3fbaecb3e --- /dev/null +++ b/docs/reference/ecosystem-hardening-pr131.md @@ -0,0 +1,424 @@ +--- +title: "Reference: Ecosystem runner-hardening batch (PR #131)" +description: > + Reference and specification for the cross-cutting runner-hardening batch that + landed alongside recipe-runner PR #131: the Repo Guardian credential-liveness + probe and E2BIG child-env allow-list (P1), the publish step-14 version + derivation (P2, #1018), the graceful reflect/iterate cancellation (P3, #1025), + and the setsid/PGID-aware signal-subscriber lifecycle (P5, #1024) — plus the + two merge/ops escalations (P4 #1015, P6 Simard backlog) and the downstream + rev-bump lever that ingests the landed upstream fixes into Simard's own build. +last_updated: 2026-07-26 +review_schedule: as-needed +owner: simard +doc_type: reference +status: design — not yet implemented +related: + - ../howto/ingest-ecosystem-hardening-fixes.md + - ../howto/self-maintain-dependency-pins.md + - ./amplihack-pin-bump-2626.md + - ./subordinate-kill-pid-guard-api.md + - ./completion-evidence-gate-api.md + - ./overseer-recipe-launch-idempotency.md + - ../concepts/e2big-elimination.md + - ../howto/route-a-goal-to-its-target-repo.md + - ../ecosystem-map.md +--- + +# Reference: Ecosystem runner-hardening batch (PR #131) + +> **Status: design — not yet implemented.** This page specifies the intended +> behaviour of the batch; nothing here has shipped yet. P2/P3/P5 are not merged +> upstream, the P1 credential is not yet rotated, and Simard's current +> `amplihack-agent-eval` pin (`14dc30b1`) predates all of them. Present-tense is +> used to describe the **specified** behaviour, not a claim that it is live. This +> batch spans **three repositories** — the fixes will live **upstream** in +> [`rysweet/amplihack-recipe-runner`](https://github.com/rysweet/amplihack-recipe-runner) +> and [`rysweet/amplihack-rs`](https://github.com/rysweet/amplihack-rs); this +> repository (`rysweet/Simard`) **ingests** them by exact git-rev pin. Simard +> only *consumes* the runner as a pinned dependency, so the downstream lever +> here is a rev-bump + `Cargo.lock` refresh, described in +> [How to ingest the ecosystem runner-hardening fixes](../howto/ingest-ecosystem-hardening-fixes.md). + +This page is the single specification the batch is verified against. It records +the finished state of six problems observed by the Overseer against the +amplihack ecosystem (see the [Ecosystem map](../ecosystem-map.md)): + +| ID | Repo (where the fix lives) | Problem | Class | Outcome | +| --- | --- | --- | --- | --- | +| **P1** | `amplihack-recipe-runner` (PR #131) | `Repo Guardian / agent` required check red on a MERGEABLE PR | quality_regression | Root cause = expired `ANTHROPIC_API_KEY` (infra 401), **not** the E2BIG code change. Fixed by secret rotation (ops) + a fail-fast liveness probe (code). | +| **P2** | `amplihack-rs` (#1018) | Publish **step-14** version bump collides with a hardcoded version-pin test | cross_cutting | Test derives the expected version from the single source of truth; bump behaviour preserved. Unblocks #1022 / #1007. | +| **P3** | `amplihack-rs` (#1025) | Recipe-runner keeps reflecting after its deliverable PR is already green | resource_pressure | Bounded, cooperative graceful-stop on verified-green goal + hard iteration/wall-clock ceiling. | +| **P5** | `amplihack-rs` (#1024) | Signal-subscriber daemons orphaned to init on session end | resource_pressure | setsid/PGID-scoped reaping of **owned** process groups on session teardown. | +| **P4** | `amplihack-rs` (#1015) | Non-draft, MERGEABLE, all-green PR awaiting merge | delivery_ready | **Escalation only** — routed to the merge steward; no code brief. | +| **P6** | `rysweet/Simard` | 16 green PRs accumulating vs `per-cycle launch cap reached` | goal_hygiene | **Escalation only** — routed to the delivery steward; partially relieved by P3 freeing launch slots. | + +> **Non-goal for this checkout.** P1–P5 are **not** implementable by editing this +> tree. Attempting to "fix" upstream code by editing a downstream consumer is a +> hollow result. The sole native action in `rysweet/Simard` is the audited +> rev-bump once the upstream fixes land — see +> [downstream ingestion](#downstream-ingestion-the-only-native-action-here). + +--- + +## Cross-cutting constraints (apply to every code workstream) + +All four code workstreams (P1 probe, P2, P3, P5) are held to the same contract, +which is also what the regression tests and CI assert: + +- **Additive / non-breaking by default.** Behaviour is unchanged for the paths a + problem does *not* describe. The E2BIG fix from PR #131 is **preserved**, never + reverted; the publish version-bump keeps working; active-session subscriber + behaviour is untouched; runs whose goal is *not* yet met keep iterating. +- **Reference the issue in the PR.** Each PR names its originating issue + (#1018, #1025, #1024) or PR (#131). +- **Preserve the PRD.** No product-requirement regressions. +- **No `bridge` naming.** New identifiers never introduce the forbidden `bridge` + term — the same guard as + [Fix a No-`bridge` Naming Guard Failure](../howto/fix-a-no-bridge-naming-guard-failure.md). +- **No stray `print!` / `println!` / `eprint!` / `dbg!`.** New code emits only + structured `tracing` events and OpenTelemetry (OTel) spans. This is asserted by + the standing AST meta-test (`syn` scan of the diff). +- **Regression test required.** Every workstream ships a test that fails before + the change and passes after it. + +--- + +## P1 — Repo Guardian credential liveness + E2BIG child-env allow-list + +**Where it lives:** upstream `amplihack-recipe-runner` +(`.github/workflows/repo-guardian.*`) and `amplihack-rs` (child-env construction). + +### Root cause (recorded, not assumed) + +The brief hypothesised that PR #131's E2BIG child-env-bounding change had stripped +a variable the guardian's `agent` step depends on. **GitHub Actions evidence +disproves that hypothesis:** the `agent` job returns `401 authentication_failed` +with `apiKeySource: none` on *every* branch since 2026-04-28 — an expired +`ANTHROPIC_API_KEY` org/repo secret, an **infra credential-lifecycle failure**. +The `agent` job never invokes the Rust binary that PR #131 touched, so **no code +patch fixes P1**. The merge unblock is a **secret rotation** (ops); the *code* +deliverable is a fail-fast liveness probe so this class of failure is loud and +immediate next time. + +### Deliverable A — credential-liveness probe (code, upstream) + +The Repo Guardian workflow gains a cheap **authenticated** probe that runs before +the agent step. The shallow presence-check that preceded it passed on a dead key +(a non-empty string is not a valid credential); the liveness probe makes an +expired key fail fast with a clear, actionable message instead of ~10 silent +retries. + +```yaml +# .github/workflows/repo-guardian.* — runs before the `agent` step +# (illustrative; the step and script live upstream and are not yet written) +- name: Verify ANTHROPIC_API_KEY liveness + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + # Cheap authenticated call; fail closed with a clear message on 401. + scripts/check-anthropic-key-liveness.sh # illustrative name +``` + +Contract: + +- **Fail-closed.** A `401` / `apiKeySource: none` fails the job **immediately** + with `Repo Guardian: ANTHROPIC_API_KEY is invalid or expired — rotate the + org/repo secret (see runbook)`, not after the agent's retry budget is spent. +- **Least data.** The probe issues the smallest authenticated request that + distinguishes "valid key" from "invalid key"; it logs **only** the auth + outcome, never the key or any response body. `::add-mask::` coverage is kept. +- **Fork isolation.** The probe (like the agent step) does **not** receive the + secret in `pull_request` runs from forks. + +### Deliverable B — E2BIG child-env allow-list (code, upstream `amplihack-rs`) + +PR #131's fix that prevents `E2BIG` (`os error 7`, argument/environment list too +long) on bash steps is **preserved and made explicit**. Child processes receive a +**minimal, documented allow-list** rather than the full ambient environment: + +| Preserved var(s) | Why | +| --- | --- | +| `ANTHROPIC_API_KEY` | agent/LLM auth | +| `GH_TOKEN`, `GITHUB_TOKEN` | GitHub API / `gh` auth | +| `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` (and lower-case) | egress through corporate proxy | +| `SSL_CERT_FILE`, `SSL_CERT_DIR`, `REQUESTS_CA_BUNDLE`, `NODE_EXTRA_CA_CERTS` | custom CA trust | +| `PATH`, `HOME`, `USER`, `LANG`, `TERM` | baseline process hygiene | + +- **Bounded (fixes E2BIG).** The child environment is capped to the allow-list + plus the step's declared vars, so the total env size cannot grow unbounded and + trip `E2BIG`. This is a **complementary** bound — it caps the *count/size of + ambient environment variables* on `envp` — alongside the distinct out-of-band + payload invariant of + [Comprehensive E2BIG elimination](../concepts/e2big-elimination.md), which + keeps large *values* (prompt/context) off `argv`/`envp` entirely. The two + mechanisms bound different inputs to the same `execve` limit; neither replaces + the other. +- **Not too narrow.** The list explicitly keeps the auth/proxy/CA vars the + guardian's agent step needs; a regression test asserts each one survives the + bounding. +- **Not too wide.** Unrelated ambient secrets are **not** forwarded to children, + so bounding does not become a secret-leak vector. + +> **Interaction with P1-A:** the allow-list keeps `ANTHROPIC_API_KEY` present; +> the liveness probe verifies it is *valid*. The two are complementary — presence +> is necessary, liveness is sufficient. + +--- + +## P2 — Publish step-14 version derivation (#1018, upstream `amplihack-rs`) + +**Symptom.** The workflow-publish step-14 auto-bumps the crate version; a separate +test asserts a **hardcoded/pinned** literal version. After a bump the literal no +longer matches, so feature-branch CI fails — corroborated by PRs #1022 and #1007 +stuck with build/Test checks in a `null` (not-passing) state. + +**Fix (finished state).** The version-pin test **derives** the expected version +from the **single source of truth** (`Cargo.toml` / a version constant) instead +of embedding a literal, and validates it is well-formed semver: + +```rust +// Before: brittle literal that step-14's bump invalidates. +// assert_eq!(reported_version(), "0.42.0"); + +// After: derived from the single source of truth. +let expected = env!("CARGO_PKG_VERSION"); // from Cargo.toml +assert!(semver::Version::parse(expected).is_ok()); // well-formed +assert_eq!(reported_version(), expected); +``` + +Contract: + +- **Bump behaviour preserved.** Step-14 still bumps and publishes exactly as + before; only the *test's* expectation is decoupled from a literal. +- **Single source of truth.** The expected version has exactly one authoritative + origin; the test never hardcodes a second copy. +- **Credential isolation.** Fork-`pull_request`-triggered runs must **not** + receive crates.io / publish credentials — the publish path is gated to trusted + contexts only. +- **Unblocks the pattern.** A branch that lands this also unblocks the #1022 / + #1007 build/Test failures; #1018 is referenced in the PR. + +--- + +## P3 — Graceful reflect/iterate cancellation (#1025, upstream `amplihack-rs`) + +**Symptom.** The recipe-runner keeps running its reflect/iterate loop **after** its +deliverable PR is already green, with no cooperative cancellation — burning LLM +budget and engineer cycles, and (per the Overseer) contributing to +`per-cycle launch cap reached`. + +**Fix (finished state).** The reflect step gains a **bounded, cooperative +graceful-stop**. Its termination predicate is keyed on an **authoritative, +authenticated** green status plus a **hard ceiling**: + +```mermaid +flowchart TD + R([reflect / iterate cycle]) --> G{"deliverable PR
verified green?
(authenticated GitHub Checks API)"} + G -->|yes| S[graceful stop:
flush OTel spans, exit 0] + G -->|no| C{"iteration ≥ max
OR wall-clock ≥ ceiling?"} + C -->|yes| B[bounded stop:
surface 'ceiling reached', exit] + C -->|no| I[iterate once more] --> R +``` + +Termination contract: + +- **Terminate on verified-green.** When the deliverable PR exists and **all + required checks are green** — read from the authenticated GitHub **Checks API**, + the authoritative source, never scraped from agent stdout — the loop stops + cleanly. +- **Keep going when unmet.** If the goal is **not** yet met, behaviour is + unchanged: the loop keeps iterating. This is the additive guarantee. +- **Hard ceiling (fail-closed cost guard).** An independent + `max-iteration` **and** wall-clock ceiling bound a *never-green* goal so it + cannot run unbounded (a cost-DoS). Hitting the ceiling stops with a distinct, + visible outcome rather than looping forever. +- **Clean shutdown.** On stop, in-flight `tracing` / OTel spans are **flushed** + with secrets scrubbed from span attributes (no tokens, keys, or full prompt + bodies). + +This composes with the existing +[Overseer recipe-launch idempotency](./overseer-recipe-launch-idempotency.md) and +mirrors, upstream, the Simard-native +[completion-evidence gate](./completion-evidence-gate-api.md) posture (a goal is +"done" only on *verified* evidence). Those Simard modules are **reference +patterns only**, not edit targets for this batch. + +### P3 configuration + +| Setting | Purpose | Default | +| --- | --- | --- | +| `AMPLIHACK_REFLECT_GRACEFUL_STOP` | Kill-switch for the graceful-stop path (`off` reverts to legacy loop) | on | +| `AMPLIHACK_REFLECT_MAX_ITERATIONS` | Hard per-run iteration ceiling for a never-green goal | bounded (non-zero) | +| `AMPLIHACK_REFLECT_WALL_CLOCK_SECS` | Hard wall-clock ceiling for a never-green goal | bounded (non-zero) | + +--- + +## P5 — Signal-subscriber daemon lifecycle (#1024, upstream `amplihack-rs`) + +**Symptom.** Signal-subscriber daemons spawned during a Copilot session are +**orphaned to init** (re-parented to PID 1) when the session ends — an unbounded +background-process/resource leak. + +**Fix (finished state).** Signal-subscriber daemons are tied to their **session +lifecycle** and reliably reaped on session end via **process-group / setsid-aware** +termination: + +- **Owned process group.** Each subscriber is spawned into its **own process + group** (`setsid` / `setpgid`) recorded in a supervised child registry. +- **Deterministic teardown.** A session shutdown hook signals the **owned PGID** + (`SIGTERM`, then `SIGKILL` after a grace window) so no child survives teardown. + Teardown is **idempotent** and runs on the panic and `SIGTERM` paths, not only + the clean-exit path. +- **Privilege-boundary safety.** The reaper signals **only** process groups it + **owns** — it verifies PGID/session ownership before signalling, so it can never + kill an unrelated group. This is the same class of guard as the Simard-native + [subordinate-kill PID guard](./subordinate-kill-pid-guard-api.md) (kill by + verified identity, not by a possibly-reused PID). +- **Active session unchanged.** Subscriber behaviour **during** a live session is + untouched; only teardown is added. + +### P5 configuration + +| Setting | Purpose | Default | +| --- | --- | --- | +| `AMPLIHACK_SIGNAL_SUBSCRIBER_REAP` | Kill-switch for lifecycle reaping (`off` reverts to legacy spawn) | on | +| `AMPLIHACK_SIGNAL_SUBSCRIBER_GRACE_MS` | Grace window between `SIGTERM` and `SIGKILL` on teardown | bounded (non-zero) | + +Verification harness: after a session ends, **no** signal-subscriber process +survives with `PPID == 1`. + +--- + +## P4 & P6 — escalations (no code brief) + +These two problems are **merge/ops throughput actions**, not code fixes. They are +routed to the delivery/merge steward and carry no implementation brief. + +- **P4 — `amplihack-rs` #1015.** A non-draft, MERGEABLE, fully-green PR. This is a + **merge action**: land it through the normal merge-ready gate. No code change. +- **P6 — `rysweet/Simard` backlog.** 16 non-draft / MERGEABLE / all-green PRs + accumulating while the Overseer hits `per-cycle launch cap reached`. This is a + **merge-draining / launch-capacity throughput** issue: escalate to the delivery + steward and/or raise the per-cycle launch cap. It is **partially relieved by + P3**, which frees launch slots by stopping already-green runs from reflecting. + +> Escalations are surfaced, not silently dropped. See +> [Triage stale pull requests](../howto/triage-stale-pull-requests.md) and +> [Review Overseer workstream gaps](../howto/review-overseer-workstream-gaps.md). + +--- + +## Downstream ingestion — the only native action here + +Editing this checkout cannot fix upstream code. The **one** native lever in +`rysweet/Simard` is to **ingest** the landed P2 / P3 / P5 fixes by bumping the +`amplihack-agent-eval` git-rev pin (source repo `amplihack-rs`) and regenerating +`Cargo.lock`, exactly as in +[amplihack pin bump to upstream main (#2626)](./amplihack-pin-bump-2626.md). + +**Ingestion is gated:** + +1. The upstream fix must have **merged** to `amplihack-rs` `main` with all + required checks green (P1 rotation must have unblocked the required checks + first). +2. The pin is bumped to a **specific audited SHA**, never a moving branch ref, so + the bump ingests exactly the audited commits and nothing unrelated. +3. `cargo build && cargo test` pass; the + [supply-chain guardrails](./supply-chain-audit.md) stay green. + +Do **not** bump unaudited upstream changes. The full procedure and done-gate are +in [How to ingest the ecosystem runner-hardening fixes](../howto/ingest-ecosystem-hardening-fixes.md) +and [How to keep Simard's dependency pins up to date](../howto/self-maintain-dependency-pins.md). + +--- + +## Configuration summary + +| Var / setting | Repo | Governs | +| --- | --- | --- | +| `ANTHROPIC_API_KEY` (rotation) | `amplihack-recipe-runner` (ops) | Unblocks the `Repo Guardian / agent` required check for PR #131 (P1) | +| `check-anthropic-key-liveness.sh` step | `amplihack-recipe-runner` | Fail-fast credential liveness (P1) | +| child-env allow-list (code constant) | `amplihack-rs` | Bounded child env / E2BIG allow-list (P1-B) — fixed in source, not a runtime tunable | +| `AMPLIHACK_REFLECT_GRACEFUL_STOP` | `amplihack-rs` | Graceful reflect cancellation kill-switch (P3) | +| `AMPLIHACK_REFLECT_MAX_ITERATIONS` / `_WALL_CLOCK_SECS` | `amplihack-rs` | Never-green hard ceiling (P3) | +| `AMPLIHACK_SIGNAL_SUBSCRIBER_REAP` | `amplihack-rs` | Subscriber lifecycle reaping kill-switch (P5) | +| `AMPLIHACK_SIGNAL_SUBSCRIBER_GRACE_MS` | `amplihack-rs` | `SIGTERM`→`SIGKILL` grace window (P5) | +| `amplihack-agent-eval` git rev (`Cargo.toml`) | `rysweet/Simard` | Downstream ingestion of P2/P3/P5 | + +> **Names illustrative.** The `AMPLIHACK_*` env-var names above are the specified +> intent; the authoritative names are owned by `amplihack-rs`. Treat this table as +> the contract's *shape*, not a frozen key list, until the upstream fixes land. + +--- + +## Security considerations + +- **P1 is the security-critical item — a credential-lifecycle failure.** Rotate to + a **least-privilege**, org/repo-scoped key; keep it out of fork `pull_request` + runs; keep `::add-mask::` coverage; audit the agent-output artifact for + accidental secret capture. +- **Fail-closed.** Invalid credentials (P1) and unmet/never-green goals (P3) fail + **visibly and bounded** — they never retry silently or run unbounded. +- **No secrets in telemetry.** `tracing` / OTel span attributes are scrubbed of + tokens, keys, and full prompt bodies (P3). +- **Untrusted-input isolation.** Fork/PR-triggered contexts receive **no** publish + (crates.io) or API credentials (P1 / P2). +- **Least privilege for children.** Env allow-lists (P1-B) and spawned daemons + (P5) receive only the vars / signals they need; the reaper signals **only** + owned process groups. +- **Supply-chain integrity.** The downstream rev-bump pins an **audited SHA** and + passes `cargo build` / `cargo test`; unaudited upstream is never ingested (P4 / + P6 governance). + +--- + +## Verify end-to-end + +**P1 (upstream / ops):** + +```bash +# PR #131's remaining red required check goes green after rotation. +gh pr checks 131 --repo rysweet/amplihack-recipe-runner --required +# The liveness probe fails fast (non-zero) on an invalid key, with a clear message. +``` + +**P2 / P3 / P5 (upstream `amplihack-rs`):** each ships a regression test — + +- P2: a test asserting the version **derives** from the source of truth, not a + literal, and build/Test are green on a branch unblocking #1022 / #1007. +- P3: a test proving the loop **terminates on verified-green** *and* **keeps + iterating when unmet** *and* **respects the max-iteration / wall-clock ceiling** + (never-green ⇒ bounded, not infinite). +- P5: a harness proving **no** signal-subscriber process survives session teardown + (`PPID == 1`). + +**Downstream (this repo), after the upstream fixes land and are audited:** + +```bash +# Bump amplihack-agent-eval to the audited amplihack-rs SHA, then: +cargo update -p amplihack-agent-eval +cargo build --release && cargo test +cargo deny --locked check && cargo audit && cargo vet --locked +``` + +--- + +## See also + +- [How to ingest the ecosystem runner-hardening fixes](../howto/ingest-ecosystem-hardening-fixes.md) — + the downstream rev-bump procedure and done-gate for this batch. +- [How to keep Simard's dependency pins up to date](../howto/self-maintain-dependency-pins.md) + and [amplihack pin bump to upstream main (#2626)](./amplihack-pin-bump-2626.md) — + the reconcile pattern this ingestion instantiates. +- [Comprehensive E2BIG elimination](../concepts/e2big-elimination.md) — the + failure class the P1-B child-env allow-list bounds. +- [Subordinate-kill PID-guard API](./subordinate-kill-pid-guard-api.md) — the + "kill by verified identity" pattern P5 mirrors. +- [Completion-evidence gate API](./completion-evidence-gate-api.md) — the + "done only on verified evidence" pattern P3 mirrors. +- [Overseer recipe-launch idempotency](./overseer-recipe-launch-idempotency.md) + and [How to review Overseer workstream gaps](../howto/review-overseer-workstream-gaps.md) — + the launch-capacity context for P3 / P6. +- [Ecosystem map](../ecosystem-map.md) — the repos this batch spans and how Simard + depends on them. diff --git a/mkdocs.yml b/mkdocs.yml index e26b9d50a..852215e37 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -244,6 +244,7 @@ nav: - Check for Updates: howto/check-for-updates.md - Configure the amplihack Freshness Gate: howto/configure-amplihack-freshness-gate.md - Self-Maintain Dependency Pins: howto/self-maintain-dependency-pins.md + - Ingest the Ecosystem Runner-Hardening Fixes (PR #131): howto/ingest-ecosystem-hardening-fixes.md - Respond to a Proactive Advisory Remediation: howto/respond-to-a-proactive-advisory-remediation.md - Configure Adaptive Scaling: howto/configure-adaptive-scaling.md - Configure Pluggable Identity: howto/configure-pluggable-identity.md @@ -478,6 +479,7 @@ nav: - Dependency Trust Policy: reference/dependency-trust-policy.md - Supply-Chain Advisory Stewardship: reference/supply-chain-advisory-stewardship.md - amplihack Pin Bump to Upstream main (#2626): reference/amplihack-pin-bump-2626.md + - Ecosystem Runner-Hardening Batch (PR #131): reference/ecosystem-hardening-pr131.md - Release Integrity (SBOM + Signing): reference/release-integrity.md - Idempotent Release Publishing: reference/release-idempotency.md - Distill Raw-Capture on Parse Failure: reference/distill-raw-capture-on-parse-failure.md diff --git a/tests/issue_131_ecosystem_hardening_batch.rs b/tests/issue_131_ecosystem_hardening_batch.rs new file mode 100644 index 000000000..e1dc1979f --- /dev/null +++ b/tests/issue_131_ecosystem_hardening_batch.rs @@ -0,0 +1,365 @@ +//! TDD acceptance / gate suite for the ecosystem runner-hardening batch +//! (recipe-runner PR #131; upstream amplihack-rs #1018 / #1025 / #1024). +//! +//! # Why this suite is downstream-shaped +//! +//! The batch spans **three repositories**. The *code* fixes live upstream and +//! are **not** editable in this checkout: +//! +//! * **P1** `Repo Guardian` credential-liveness probe + E2BIG child-env +//! allow-list — upstream `rysweet/amplihack-recipe-runner` (PR #131) and +//! `rysweet/amplihack-rs`. Root cause is an expired `ANTHROPIC_API_KEY` +//! (infra 401), so the merge unblock is an **ops secret rotation**, not a +//! code patch. The upstream regression tests for the probe live in those +//! repos (see `tests/upstream/` spec artifacts shipped with this batch). +//! * **P2** publish step-14 version derivation — upstream `amplihack-rs` #1018. +//! * **P3** graceful reflect/iterate cancellation — upstream `amplihack-rs` #1025. +//! * **P5** signal-subscriber daemon lifecycle — upstream `amplihack-rs` #1024. +//! * **P4** #1015 and **P6** Simard backlog are **merge/ops escalations** with +//! no code brief. +//! +//! Editing `rysweet/Simard` cannot fix upstream code. The **only** native lever +//! here is to *ingest* the landed P2/P3/P5 fixes by bumping the +//! `amplihack-agent-eval` git-rev pin (source repo `amplihack-rs`) to an +//! **audited SHA** and refreshing `Cargo.lock` — exactly the reactive done-gate +//! specified in +//! `docs/howto/ingest-ecosystem-hardening-fixes.md` and worked before by +//! `tests/issue_2626_amplihack_pin_bump.rs`. +//! +//! # What this suite therefore verifies (and what it deliberately defers) +//! +//! Two contracts are checkable in *this* tree today, so they are the acceptance +//! tests the batch's downstream deliverable is verified against: +//! +//! 1. **Documentation contract.** The reference + how-to design pages exist, +//! are registered in the mkdocs nav, carry the honest +//! `status: design — not yet implemented` frontmatter (never a premature +//! `status: implemented`), name every problem's issue/PR, and do **not** +//! invent the `AMPLIHACK_CHILD_ENV_ALLOWLIST` runtime tunable the design +//! review struck (the child-env allow-list is a code constant upstream). +//! +//! 2. **Premature-bump gate (fail-closed).** Until the upstream fixes are +//! **merged and audited**, the pin MUST stay at the pre-batch audited rev +//! and MUST be an immutable full-SHA on the allow-listed remote. A bump to +//! an unaudited / moving ref reds this suite. This is the same "audited SHA, +//! not a moving branch" invariant the how-to's done-gate spells out. +//! +//! The forward **rev-bump acceptance** (pin == the *new* audited SHA, lockfile +//! refreshed, `cargo build && cargo test` green) is **not** hard-coded here: the +//! target SHA does not exist until P2/P3/P5 merge upstream. Fabricating a +//! placeholder SHA would be a false assertion. When the ingest step runs, whoever +//! performs the bump advances `EXPECTED_AGENT_EVAL_REV` below (and the +//! `Cargo.toml` / `Cargo.lock` pins) to the audited SHA in one commit — the +//! RED→GREEN transition, mirroring how #2626 landed. +//! +//! # Why these are file-shaped (std-only, rg/grep-shaped) +//! +//! Like `issue_2626_amplihack_pin_bump.rs` and `docs_integrity.rs`, they read the +//! raw `Cargo.toml` / `Cargo.lock` / docs with std only — no network, no crate +//! import — so an operator running the equivalent `grep` gets the same answer CI +//! does, decoupled from the heavy `simard` build. + +use std::fs; +use std::path::PathBuf; + +// ── Pin constants (verified against the current tree at authoring time) ────── + +/// The audited pre-batch `amplihack-agent-eval` rev (source repo `amplihack-rs`). +/// It **predates** every P2/P3/P5 fix — the reference page records this exact +/// SHA as the pin that "predates all of them". Until the upstream fixes land and +/// are audited, the pin must stay here; the ingest step advances this constant. +const EXPECTED_AGENT_EVAL_REV: &str = "14dc30b10e87764120c6f2bae7f3630522c29e5d"; + +/// The only git remote `amplihack-agent-eval` may resolve from. A bump must never +/// introduce a new git source (typosquat / allow-list-bypass guard). +const AGENT_EVAL_REMOTE: &str = "https://github.com/rysweet/amplihack-rs.git"; + +const REFERENCE_DOC: &str = "docs/reference/ecosystem-hardening-pr131.md"; +const HOWTO_DOC: &str = "docs/howto/ingest-ecosystem-hardening-fixes.md"; + +// ── Path / IO helpers ──────────────────────────────────────────────────────── + +fn repo_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +fn read_repo_file(rel: &str) -> String { + let path = repo_root().join(rel); + fs::read_to_string(&path).unwrap_or_else(|e| panic!("could not read {} ({e})", path.display())) +} + +fn cargo_toml() -> String { + read_repo_file("Cargo.toml") +} + +fn cargo_lock() -> String { + read_repo_file("Cargo.lock") +} + +// ── Tiny structural matchers (std-only, comment-aware) ─────────────────────── + +/// The first non-comment manifest line whose key is exactly `name` (`name = …`). +fn manifest_dep_line(contents: &str, name: &str) -> Option { + contents + .lines() + .map(str::trim) + .find(|l| { + if l.starts_with('#') { + return false; + } + match l.strip_prefix(name) { + Some(rest) => rest.trim_start().starts_with('='), + None => false, + } + }) + .map(str::to_string) +} + +/// Extract the value of a `key = "value"` field from a single manifest line. +fn field_value(line: &str, key: &str) -> Option { + let needle = format!("{key} = \""); + let start = line.find(&needle)? + needle.len(); + let rest = &line[start..]; + let end = rest.find('"')?; + Some(rest[..end].to_string()) +} + +fn dep_rev(contents: &str, name: &str) -> Option { + field_value(&manifest_dep_line(contents, name)?, "rev") +} + +fn dep_git_remote(contents: &str, name: &str) -> Option { + field_value(&manifest_dep_line(contents, name)?, "git") +} + +/// The `source = "…"` string of the `[[package]]` named `name` in Cargo.lock. +fn locked_source(lockfile: &str, name: &str) -> Option { + let needle = format!("name = \"{name}\""); + let mut lines = lockfile.lines().peekable(); + while let Some(line) = lines.next() { + if line.trim() != needle { + continue; + } + for following in lines.by_ref() { + let t = following.trim(); + if let Some(v) = t.strip_prefix("source = \"") { + return v.strip_suffix('"').map(str::to_string); + } + if t.starts_with("[[package]]") { + break; + } + } + } + None +} + +/// True when `rev` is a full 40-char lowercase hex git SHA (not a branch/tag). +fn is_full_sha(rev: &str) -> bool { + rev.len() == 40 + && rev + .chars() + .all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase()) +} + +/// The YAML frontmatter block (between the first two `---` fences). +fn frontmatter(contents: &str) -> &str { + let body = contents.strip_prefix("---").unwrap_or(contents); + match body.find("\n---") { + Some(end) => &body[..end], + None => "", + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Contract 1 — the batch design documentation +// ───────────────────────────────────────────────────────────────────────────── + +#[test] +fn batch_reference_and_howto_docs_exist() { + for rel in [REFERENCE_DOC, HOWTO_DOC] { + let path = repo_root().join(rel); + assert!( + path.is_file(), + "batch design page `{rel}` must exist — it is the specification this \ + suite verifies against." + ); + } +} + +#[test] +fn batch_docs_declare_honest_design_status_not_implemented() { + // The design review's Issue 1: status must be honest. Nothing has shipped — + // P2/P3/P5 are unmerged, P1's secret is unrotated, the pin predates all — + // so claiming `status: implemented` would be a false completion signal. + for rel in [REFERENCE_DOC, HOWTO_DOC] { + let fm = read_repo_file(rel); + let head = frontmatter(&fm); + assert!( + head.contains("status: design — not yet implemented"), + "`{rel}` frontmatter must carry the honest \ + `status: design — not yet implemented` — nothing in this batch has \ + shipped yet." + ); + assert!( + !head.contains("status: implemented"), + "`{rel}` frontmatter must NOT claim `status: implemented`: P2/P3/P5 are \ + unmerged upstream and the P1 credential is unrotated." + ); + } +} + +#[test] +fn batch_reference_names_every_problem_issue() { + // Cross-cutting constraint: each problem is traceable to its issue/PR. + let doc = read_repo_file(REFERENCE_DOC); + for (id, token) in [ + ("P1 (Repo Guardian PR)", "#131"), + ("P2 (version derivation)", "#1018"), + ("P3 (graceful reflect stop)", "#1025"), + ("P5 (subscriber lifecycle)", "#1024"), + ("P4 (merge escalation)", "#1015"), + ] { + assert!( + doc.contains(token), + "reference page must name {id} by its tracking ref `{token}` so the \ + batch stays traceable." + ); + } +} + +#[test] +fn batch_docs_do_not_invent_child_env_allowlist_tunable() { + // Design review Issue 2: the E2BIG child-env allow-list is a *code constant* + // upstream, NOT a Simard runtime env var. The invented + // `AMPLIHACK_CHILD_ENV_ALLOWLIST` tunable was struck; it must not reappear. + for rel in [REFERENCE_DOC, HOWTO_DOC] { + let doc = read_repo_file(rel); + assert!( + !doc.contains("AMPLIHACK_CHILD_ENV_ALLOWLIST"), + "`{rel}` must not advertise a `AMPLIHACK_CHILD_ENV_ALLOWLIST` runtime \ + tunable — the child-env allow-list is a fixed code constant upstream, \ + not a Simard-settable variable." + ); + } +} + +#[test] +fn batch_docs_are_registered_in_mkdocs_nav() { + // docs_integrity.rs enforces that every nav entry resolves to a file; this + // enforces the converse for the batch — the two pages are actually wired into + // the human-maintained nav so they are discoverable, not orphaned. + let nav = read_repo_file("mkdocs.yml"); + for rel in [REFERENCE_DOC, HOWTO_DOC] { + let nav_path = rel.strip_prefix("docs/").unwrap_or(rel); + assert!( + nav.contains(nav_path), + "mkdocs.yml nav must reference `{nav_path}` so the batch page is \ + discoverable (and covered by the docs-integrity gate)." + ); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Contract 2 — the premature-bump gate (fail-closed until upstream is audited) +// ───────────────────────────────────────────────────────────────────────────── + +#[test] +fn agent_eval_pin_not_prematurely_bumped_before_upstream_lands() { + // The ingest gate: do NOT bump `amplihack-agent-eval` until P2/P3/P5 have + // merged upstream AND been audited. Until then the pin stays at the audited + // pre-batch rev. When the ingest step runs, advance BOTH this constant and + // `Cargo.toml`/`Cargo.lock` to the new audited SHA in one commit. + let rev = dep_rev(&cargo_toml(), "amplihack-agent-eval") + .expect("Cargo.toml must declare a git `amplihack-agent-eval` dependency with a `rev`"); + assert_eq!( + rev, EXPECTED_AGENT_EVAL_REV, + "amplihack-agent-eval pin drifted to `{rev}`. Until the upstream P2/P3/P5 \ + fixes land and are audited it must stay at the pre-batch rev \ + {EXPECTED_AGENT_EVAL_REV}. If you are performing the audited ingest, \ + update EXPECTED_AGENT_EVAL_REV in this test in the same commit." + ); +} + +#[test] +fn agent_eval_pin_is_immutable_full_sha_on_allowlisted_remote() { + // A pin must be an immutable 40-char SHA on the known remote — never a + // `branch`/`tag` (a moving ref could swap the linked code between builds) and + // never a new git host (typosquat / allow-list bypass). + let toml = cargo_toml(); + let line = manifest_dep_line(&toml, "amplihack-agent-eval") + .expect("missing `amplihack-agent-eval` dependency line in Cargo.toml"); + assert!( + !line.contains("branch =") && !line.contains("tag ="), + "`amplihack-agent-eval` must be pinned by an immutable `rev` SHA, not a \ + branch/tag: {line}" + ); + let rev = dep_rev(&toml, "amplihack-agent-eval").expect("no `rev` pin"); + assert!( + is_full_sha(&rev), + "`amplihack-agent-eval` rev `{rev}` is not a full 40-char lowercase hex SHA." + ); + let remote = dep_git_remote(&toml, "amplihack-agent-eval").unwrap_or_default(); + assert_eq!( + remote, AGENT_EVAL_REMOTE, + "`amplihack-agent-eval` must stay on {AGENT_EVAL_REMOTE}, found `{remote}`." + ); +} + +#[test] +fn cargo_lock_agrees_with_agent_eval_pin() { + // The lockfile must resolve the same audited rev from the same remote — no + // drift between manifest intent and the resolved source. + let source = locked_source(&cargo_lock(), "amplihack-agent-eval") + .expect("Cargo.lock must contain the amplihack-agent-eval [[package]] source"); + assert!( + source.contains(EXPECTED_AGENT_EVAL_REV), + "Cargo.lock amplihack-agent-eval source must record rev \ + {EXPECTED_AGENT_EVAL_REV}; found `{source}`." + ); + assert!( + source.contains(AGENT_EVAL_REMOTE.trim_end_matches(".git")), + "Cargo.lock amplihack-agent-eval source must resolve from {AGENT_EVAL_REMOTE}; \ + found `{source}`." + ); +} + +#[test] +fn ingest_howto_encodes_audited_scoped_bump_gate() { + // The how-to must specify the *scoped* lock refresh and the "audited SHA, not + // a moving branch" gate — the operational contract the ingest step follows. + let howto = read_repo_file(HOWTO_DOC); + assert!( + howto.contains("cargo update -p amplihack-agent-eval"), + "the ingest how-to must document the scoped `cargo update -p \ + amplihack-agent-eval` (no unrelated lockfile churn)." + ); + assert!( + howto.contains("audited") && howto.contains("moving branch"), + "the ingest how-to must gate the bump on an *audited* SHA and forbid a \ + *moving branch* ref (supply-chain guard)." + ); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Pure-function pins — matcher logic verified independently of the tree +// ───────────────────────────────────────────────────────────────────────────── + +#[test] +fn is_full_sha_accepts_only_immutable_revs() { + assert!(is_full_sha(EXPECTED_AGENT_EVAL_REV)); + assert!(!is_full_sha("main")); // branch ref + assert!(!is_full_sha("14dc30b")); // short SHA + assert!(!is_full_sha("14DC30B10E87764120C6F2BAE7F3630522C29E5D")); // uppercase + assert!(!is_full_sha(&"z".repeat(40))); // non-hex +} + +#[test] +fn frontmatter_extracts_only_the_yaml_block() { + let doc = "---\ntitle: x\nstatus: design — not yet implemented\n---\n\n# Body\nstatus: implemented (in prose, not frontmatter)\n"; + let fm = frontmatter(doc); + assert!(fm.contains("status: design — not yet implemented")); + assert!( + !fm.contains("in prose"), + "frontmatter() must stop at the closing fence and ignore the body" + ); +}