diff --git a/docs/architecture/slice-dag.md b/docs/architecture/slice-dag.md index 8094c9256..7a0e26e8b 100644 --- a/docs/architecture/slice-dag.md +++ b/docs/architecture/slice-dag.md @@ -551,9 +551,10 @@ shape: after `record_failure`) and the per-slice green gate (`slice_green_gate.run_slice_green_gate`, #3398, which spawns a sandboxed one-shot check-runner Job to execute the - repo's configured checks at the integration-branch tip and - blocks PR-open on a red verdict; staged rollout via - `EGG_SLICE_GREEN_GATE`, fail-open on infra errors, including + repo's configured checks at the integration-branch tip; staged + rollout via `EGG_SLICE_GREEN_GATE` — default `on` (a red verdict + withholds the slice PR), `log` runs the checks and logs the + verdict without blocking; fail-open on infra errors, including infra-signature-tagged reds inside check execution, #3417) — calls `GatewayClient.create_slice_pr` with `base` resolved from the slice's DAG parent (root → latest completed chain tip, else the @@ -1058,10 +1059,10 @@ on parse failure. The green-gate knobs below are read directly via | `EGG_ORCH_STACKED_PR_RECONCILER_INTERVAL_SECONDS` | float | 30.0 | Reconciler polling cadence for orphaned child PRs. | | `EGG_ORCH_CROSS_REPO_MERGE_GATE_MAX_ATTEMPTS` | int | 240 | Poll-attempt budget for the cross-repo merge-sequencing gate (#3393) before a never-merging upstream escalates to a HITL hold; ~2h at the default reconciler cadence. See [Cross-repo merge-sequencing hold](#cross-repo-merge-sequencing-hold-two-tier). | | `EGG_SLICE_BASE_ANCESTRY_GATE` | str | `on` | Operator kill switch for the admission-time base-ancestry gate (#3541 — see [Root linearization & the base-ancestry gate](#root-linearization--the-base-ancestry-gate-3541)): any of `off`/`0`/`false`/`no` (case-insensitive, whitespace-tolerant) disables the gate; any other value (including unset) leaves it enabled. | -| `EGG_SLICE_GREEN_GATE` | str | `off` | Per-slice green gate rollout switch (#3398): `off` skips the gate entirely; `log` runs the repo's configured checks at the slice tip and logs a red verdict without blocking; `on` blocks slice PR-open on a red verdict. Case-insensitive, with aliases — `on` also accepts `1`/`true`/`yes`, and `log` also accepts `log-only`/`log_only`. Unknown values resolve to `off`. | +| `EGG_SLICE_GREEN_GATE` | str | `on` | Per-slice green gate rollout switch (#3398): `off` skips the gate entirely; `log` runs the repo's configured checks at the slice tip and logs a red verdict without blocking; `on` (the default) withholds the slice PR on a red verdict. Case-insensitive, with aliases — `on` also accepts `1`/`true`/`yes`, `log` also accepts `log-only`/`log_only`, and `off` also accepts `0`/`false`/`no`. Weakening the gate takes an explicit, correctly-spelled `off` or `log`; unset or unrecognised values resolve to `on`, so a typo cannot silently drop a deployment below the product default. Expect the first reds to be gate wiring rather than slice code — a stale contract snapshot reddening contract-hygiene tests (#3301), or `make test`'s changeset narrowing resolving its baseline against `git merge-base` in a fresh worktree — and note either of those reds *every* slice close until fixed (a missing prebuilt-deps snapshot is not in this list: the runner exits non-zero and the gate fails open, costing coverage rather than throughput); the failure message names the branch to fix and quotes `EGG_SLICE_GREEN_GATE=off` as the bypass, and the slice's commits stay on the integration branch (a red gate withholds the PR, it does not discard work). Latency is identical under `log` and `on` — both run the checks and wait for the runner pod, so slice-close latency grows by the check duration (bounded by `EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS`). The worst case is not a slow suite but a runner pod that never schedules — the wait is `timeout` plus a 120s scheduling grace (~32 min at the defaults) before failing open, so a capacity-starved cluster pays that per slice close. A *partially* delayed pod is quieter and more common: the runner's deadline is the **Job's** `activeDeadlineSeconds`, counted from the Job's `startTime` (before any pod is bound), so time spent Pending or pulling is subtracted from the in-pod check budget rather than added to the wait — the scheduling grace widens only the orchestrator's wait. A pod delayed N seconds gets N fewer seconds to run checks, and a `DeadlineExceeded` kill emits no verdict line, so the gate fails open with no verdict at all. Capacity starvation therefore raises the rate of spurious no-verdict fail-opens as well as dead time ([#3622](https://github.com/jwbron/egg/issues/3622)) — under the `on` default that is a slice close you believed was gated and wasn't, though the direction is always *under*-blocking, never a false red. Grep for *both* fail-open log lines when diagnosing one: the Job controller deletes the active pod on `DeadlineExceeded` rather than leaving it terminal, so the wait normally times out ("runner pod did not reach a terminal state"); a poll that catches the pod reporting `Failed` mid-termination reads partial output and lands on "no parseable verdict from runner" instead. | | `EGG_SLICE_GREEN_GATE_SKIP_CHECKS` | str (comma-separated) | `security` | Configured check *names* (from `repositories.yaml` `checks`) the gate skips. | -| `EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS` | int | 1800 | Wall-clock budget for the check-runner pod (spawn-to-terminal); a hung suite degrades to fail-open rather than wedging the slice close. | -| `EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN` | str | `on` | Infra-red fail-open (#3417): the runner tags red checks whose full output matches an exact infra signature (the sandbox git wrapper's gateway-down / missing-env / session-auth errors, the kernel's ENOSPC message) or whose process died by SIGKILL; a verdict where *every* red check is infra-tagged fails open instead of blocking, and mixed verdicts block on the genuine reds only. `off`/`0`/`false`/`no` restores strict every-red-blocks behavior; any other value resolves to `on`. | +| `EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS` | int | 1800 | Wall-clock budget for the check-runner pod (spawn-to-terminal); a hung suite degrades to fail-open rather than wedging the slice close. Enforced as a Job-level `activeDeadlineSeconds` (`timeout + 60`) counted from Job start, so it is a ceiling on scheduling *plus* checks, not on checks alone — see the `EGG_SLICE_GREEN_GATE` row and [#3622](https://github.com/jwbron/egg/issues/3622). | +| `EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN` | str | `on` | Infra-red fail-open (#3417): the runner tags red checks whose full output matches an exact infra signature (the sandbox git wrapper's gateway-down / missing-env / session-auth errors, the kernel's ENOSPC message) or whose process died by SIGKILL; a verdict where *every* red check is infra-tagged fails open instead of blocking, and mixed verdicts block on the genuine reds only. `off`/`0`/`false`/`no` restores strict every-red-blocks behavior; `on`/`1`/`true`/`yes` (and unset, and an empty or whitespace-only value) enable it silently; any other value resolves to `on` and logs a warning, since the typo direction here is strict → lenient. | ### Per-pipeline vs. global slice caps diff --git a/docs/development/STRUCTURE.md b/docs/development/STRUCTURE.md index dc4754083..3af9f5812 100644 --- a/docs/development/STRUCTURE.md +++ b/docs/development/STRUCTURE.md @@ -110,7 +110,7 @@ orchestrator/ ├── slice_scheduler.py # Wave-based scheduler for the implement-phase slice DAG: computes execution waves, caps concurrency, two-tier max_cycles accounting, failure-cascade detection (#2137) ├── stacked_pr_reconciler.py # Stacked-PR rebase reconciler: detects child slice PRs whose base branch was deleted after a parent merge and retargets them via gateway rebase_onto (#2137) ├── cross_repo_merge_gate.py # Cross-repo merge-sequencing gate for multi-repo pipelines: auto-readies (or HITL-holds) a dependent slice's draft PR once its cross-repo upstream PR merges; rides the stacked-PR reconciler cadence (#3393 slice-5) -├── slice_green_gate.py # Per-slice green gate: sandboxed one-shot Job runs the repo's configured checks at the integration-branch tip and blocks PR-open on red; staged rollout via EGG_SLICE_GREEN_GATE (off/log/on), fail-open on infra errors (#3398) and on infra-signature-tagged reds inside check execution (#3417) +├── slice_green_gate.py # Per-slice green gate: sandboxed one-shot Job runs the repo's configured checks at the integration-branch tip; staged rollout via EGG_SLICE_GREEN_GATE (off/log/on, default on — on withholds the slice PR on a red verdict, log runs the checks and logs the verdict without blocking), fail-open on infra errors (#3398) and on infra-signature-tagged reds inside check execution (#3417) ├── action_guards.py # Formal BRC state machine action guards (preconditions for propose/ack/nack/confirm/withdraw) ├── approval_matrix.py # Per-reviewer ACK/NACK matrix for BRC consensus ├── attestation_schemas.py # Attestation payload validation for BRC proposals diff --git a/orchestrator/evidence_gatherer.py b/orchestrator/evidence_gatherer.py index 04ae24f00..4b529834a 100644 --- a/orchestrator/evidence_gatherer.py +++ b/orchestrator/evidence_gatherer.py @@ -62,13 +62,14 @@ # --------------------------------------------------------------------------- # # The shared-evidence prefix ships behind the same ``off -> log -> on`` staged -# flag every #3523 behaviour-shift rides, resolved EXACTLY like -# ``slice_green_gate.green_gate_mode()``: an operator typo must degrade to -# "reviewer prompts unchanged", never to "one gatherer silently anchors every -# lens". The resolver lives HERE, with the feature's core, and is imported by -# the S7 wiring (``_criteria.py`` assembly seam, ``consensus_wrapper.py`` log -# recording) — mirroring how ``risk_router`` owns ``ReviewStance`` for the S6 -# wiring to consume. +# flag every #3523 behaviour-shift rides, using that shared pattern but keeping +# an ``off``-default (``slice_green_gate.green_gate_mode()`` now defaults to +# ``on`` and degrades unknown to ``on`` — this resolver deliberately does +# not): an operator typo must degrade to "reviewer prompts unchanged", never +# to "one gatherer silently anchors every lens". The resolver lives HERE, with +# the feature's core, and is imported by the S7 wiring (``_criteria.py`` +# assembly seam, ``consensus_wrapper.py`` log recording) — mirroring how +# ``risk_router`` owns ``ReviewStance`` for the S6 wiring to consume. EVIDENCE_PREFIX_ENV_VAR = "EGG_REVIEW_EVIDENCE_PREFIX" _ENABLED_VALUES = frozenset({"on", "1", "true", "yes"}) diff --git a/orchestrator/review_findings_verdict.py b/orchestrator/review_findings_verdict.py index f47e92072..2fa49a00a 100644 --- a/orchestrator/review_findings_verdict.py +++ b/orchestrator/review_findings_verdict.py @@ -30,14 +30,17 @@ on escalation) is the point. **Staged rollout.** The whole path is gated behind ``EGG_REVIEW_FINDINGS_MODE``, -resolved EXACTLY like ``slice_green_gate.green_gate_mode()`` (``off`` default, -unknown => ``off``, ``log`` records the computed-vs-legacy verdict into the BRC -artifacts without acting, ``on`` uses the computed verdict). Everything in this -module is a pure function of its inputs; it never reads/writes matrix state or -the environment except through :func:`review_findings_mode`. The caller (a -later wiring slice) decides — based on the mode — whether to *act* on the -computed verdict or merely *log* it, which is what keeps ``off``/``log`` -outcomes byte-identical to the legacy prose-NACK path. +resolved with the shared staged ``off``/``log``/``on`` pattern but keeping an +``off``-default (unknown => ``off``; note ``slice_green_gate.green_gate_mode()`` +now defaults to ``on`` and degrades unknown to ``on`` — this resolver +deliberately does not, so a typo leaves the legacy path authoritative). ``log`` +records the computed-vs-legacy verdict into the BRC artifacts without acting; +``on`` uses the computed verdict. Everything in this module is a pure function +of its inputs; it never reads/writes matrix state or the environment except +through :func:`review_findings_mode`. The caller (a later wiring slice) decides +— based on the mode — whether to *act* on the computed verdict or merely *log* +it, which is what keeps ``off``/``log`` outcomes byte-identical to the legacy +prose-NACK path. """ from __future__ import annotations @@ -60,7 +63,7 @@ VERDICT_NACK = "NACK" -# --- staged-flag resolution (mirrors slice_green_gate.green_gate_mode) -------- +# --- staged-flag resolution (shared off/log/on pattern, off-default) --------- # Operator switch for the findings-computed verdict path. Three-state, # default off during rollout (#3523 S3): "off"/unset => the legacy @@ -77,9 +80,13 @@ def review_findings_mode() -> Literal["off", "log", "on"]: """Resolve the operator switch to one of ``off`` / ``log`` / ``on``. - Resolved EXACTLY like ``slice_green_gate.green_gate_mode()``: unknown - values resolve to ``off`` so an operator typo degrades to "legacy path - unchanged", never to "computed verdict silently drives consensus". + Shares the staged ``off``/``log``/``on`` shape but keeps an ``off``-default: + unknown values resolve to ``off`` so an operator typo degrades to "legacy + path unchanged", never to "computed verdict silently drives consensus". + (Unlike ``slice_green_gate.green_gate_mode()``, which now defaults to + ``on`` and degrades unknown values to ``on`` + a warning: a wrong + resolution there blocks a slice visibly and recoverably, whereas a wrong + resolution here would silently drive consensus off a computed verdict.) """ raw = os.environ.get(FINDINGS_MODE_ENV_VAR, "off").strip().lower() if raw in _ENABLED_VALUES: diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index d722a74f2..b61e7c20f 100644 --- a/orchestrator/review_graph.py +++ b/orchestrator/review_graph.py @@ -475,8 +475,10 @@ def register_phase_graph(phase: str, graph: ReviewGraph) -> None: # (``agent_model_resolution.resolve_agent_model`` imports :func:`risk_router_mode` # and :func:`resolve_risk_decision` from here). # -# Everything rides ONE staged flag, ``EGG_RISK_ROUTER``, resolved EXACTLY like -# ``slice_green_gate.green_gate_mode()`` (``off`` default, unknown => ``off``): +# Everything rides ONE staged flag, ``EGG_RISK_ROUTER``, resolved with the +# shared staged ``off``/``log``/``on`` pattern but keeping an ``off``-default +# (unknown => ``off``; ``slice_green_gate.green_gate_mode()`` now defaults to +# ``on`` and degrades unknown to ``on`` — this resolver deliberately does not): # * ``off`` — inert. The live graph + efforts are byte-identical to legacy. # * ``log`` — compute the would-be gated graph / tier / effort and record it # (:func:`risk_route_log_record` + a structured log line), but @@ -497,9 +499,12 @@ def register_phase_graph(phase: str, graph: ReviewGraph) -> None: def risk_router_mode() -> Literal["off", "log", "on"]: """Resolve the ``EGG_RISK_ROUTER`` switch to ``off`` / ``log`` / ``on``. - Resolved EXACTLY like ``slice_green_gate.green_gate_mode()``: an unknown - value resolves to ``off`` so an operator typo degrades to "router does - nothing" (full graph, legacy effort), never to "silently review less". + Shares the staged ``off``/``log``/``on`` shape but keeps an ``off``-default: + an unknown value resolves to ``off`` so an operator typo degrades to "router + does nothing" (full graph, legacy effort), never to "silently review less". + (Unlike ``slice_green_gate.green_gate_mode()``, which now defaults to + ``on`` and degrades unknown values to ``on`` + a warning: over-verifying + is that switch's safe direction, under-reviewing is never this one's.) """ raw = os.environ.get(RISK_ROUTER_ENV_VAR, "off").strip().lower() if raw in _RISK_ROUTER_ENABLED_VALUES: diff --git a/orchestrator/routes/pipelines/_run_implement.py b/orchestrator/routes/pipelines/_run_implement.py index 231bc9969..3dfd62fed 100644 --- a/orchestrator/routes/pipelines/_run_implement.py +++ b/orchestrator/routes/pipelines/_run_implement.py @@ -910,14 +910,18 @@ def _run_one_slice_inner( # #3398 — per-slice green gate: execute the repo's # configured checks (repositories.yaml, via # get_repo_checks) against the integration-branch tip - # in a sandboxed one-shot runner, and refuse to open - # the slice PR while any check is red. Closes the - # trust-vs-verify gap in the propose-time + # in a sandboxed one-shot runner and, in "on" mode, + # refuse to open the slice PR while any check is red. + # Closes the trust-vs-verify gap in the propose-time # checks_passed self-report. Same posture as the # evidence gate above: fail-open on infra errors, # fail-closed only on a definitive red verdict; # EGG_SLICE_GREEN_GATE is the operator switch - # (off during rollout / log / on). + # (off / log / on), defaulting to "on": the checks + # run on every slice close and a definitive red + # withholds the PR. "log" runs them without + # blocking; "off" is the escape hatch, and is + # quoted in the failure message itself. if pipeline.repo: try: import slice_green_gate as _green_gate diff --git a/orchestrator/slice_green_gate.py b/orchestrator/slice_green_gate.py index 09869431b..420ee7264 100644 --- a/orchestrator/slice_green_gate.py +++ b/orchestrator/slice_green_gate.py @@ -67,11 +67,84 @@ OOM and fails open. ``EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN=off`` restores the strict every-red-blocks behavior. -Rollout is staged via ``EGG_SLICE_GREEN_GATE``: ``off`` (default) → -``log`` (run checks, log the verdict loudly, never block — the soak mode -while #3301 contract-single-writer is still landing, since a stale -contract snapshot on the slice tip can red contract-hygiene tests for -reasons unrelated to the slice's code) → ``on`` (block). +Rollout is staged via ``EGG_SLICE_GREEN_GATE``: ``off`` → ``log`` (run +the checks, log the verdict loudly, never block) → ``on`` (**the +default**: block PR-open on a definitive red). + +``on`` is the default rather than ``off`` because a gate nobody runs +verifies nothing: the switch shipped in #3398 defaulting to ``off`` and +was never set in any deployment, so the check-runner path had not +executed once in the ~3 weeks after it landed. #3602 is the shape that +costs — a contract task marked ``complete`` while five tests failed on +the slice tip — and it is a shape only a *blocking* gate prevents. + +``log`` stays available for the deployment that wants verdicts without +the blocking decision, and it is the right posture for a fleet, where a +false red stalls a pipeline whose owner is not the person watching the +rollout. It is not the default because its evidence is **passive**: a +verdict is a structured log line, with no metric, audit event, or PR +comment behind it (#3623). ``log`` therefore only informs an operator +who goes looking, and this switch's own history is that nobody does. +Under ``on`` a wrong verdict announces itself on the next slice close, +to the operator who can act on it — which makes ``on`` the *better* +instrument for measuring the false-red rate, not merely the stricter +one. + +Expect the first reds to be the gate's own wiring rather than the +slice's code, and note that either of these reds *every* slice close +until it is fixed: a stale contract snapshot on the slice tip can red +contract-hygiene tests for reasons unrelated to the slice (#3301), and +``make test``'s changeset narrowing derives its baseline from ``git +merge-base`` inside a fresh worktree, so it sees the cumulative slice +diff rather than the tester's own-files scope. (A missing prebuilt-deps +snapshot is *not* in this list: the runner exits non-zero and the gate +fails open — see the toolchain paragraph below — so it costs coverage, +not slice throughput.) Recovery from a wrong red is bounded and +self-documenting: the failure message names the branch to fix, the +slice restarts, and ``EGG_SLICE_GREEN_GATE=off`` is quoted inline as +the bypass. The slice's commits stay on the integration branch through +all of it — a red gate withholds the PR, it does not discard work. + +The latency cost is identical under ``log`` and ``on`` — both spawn the +runner and wait for the pod — so it is the price of *running* the gate, +not of blocking on it. Slice-close latency grows by the check duration +(bounded by ``EGG_SLICE_GREEN_GATE_TIMEOUT_SECONDS``, default 1800s, +plus the ``_POD_SCHEDULING_GRACE_SECONDS`` the orchestrator's wait adds +on top — see the next paragraph — after which the gate fails open). +``off`` remains available for deployments that cannot absorb that. + +Whoever watches the rollout should know the worst case is **not** a slow +check suite: it is a runner pod that never schedules. ``_wait_for_runner_pod`` +waits ``timeout + _POD_SCHEDULING_GRACE_SECONDS`` (~32 min at the defaults) +before failing open, so a capacity-starved cluster pays that in dead time on +*every* slice close, in every deployment, from the moment this default lands. + +A *partially* delayed pod is the quieter half of the same problem, and on a +busy cluster the more common one. The runner's deadline is the **Job's** +``activeDeadlineSeconds``, counted from the Job's ``startTime`` — before any +pod is bound — so time spent Pending or pulling comes out of the check budget +rather than being added to it (``_POD_SCHEDULING_GRACE_SECONDS`` widens only +the orchestrator's wait). A pod delayed N seconds gets N fewer seconds to run +checks, and a ``DeadlineExceeded`` kill emits no verdict line, so the gate +fails open with no verdict at all. Capacity starvation therefore raises the +rate of *spurious no-verdict fail-opens* as well as dead time — silently +narrowing how much of the slice stream the gate actually covers. + +Which log line an operator sees for that is not fixed, so grep for both: on +``DeadlineExceeded`` the Job controller *deletes* the active pod rather than +leaving it terminal, so ``_wait_for_runner_pod`` usually never observes +``Succeeded``/``Failed``, polls out its own (larger) budget, and the gate logs +"runner pod did not reach a terminal state". If a poll happens to catch the +pod reporting ``Failed`` mid-termination, the log read returns partial output +and "no parseable verdict from runner" fires instead. Both fail open, and +neither is distinguishable from a runner-harness crash. Tracked in #3622, +which this default makes the top follow-up: under ``on`` a missing verdict +is a slice close you believed was gated and wasn't. Note the direction — a +no-verdict fail-open can only *under*-block, never produce a false red, so +it is a coverage gap rather than a correctness risk. + +Someone should watch the first wave directly rather than discovering the cost +from a slice-throughput drop later; ``off`` is the escape hatch. The check toolchain is the **repo-defined** one, not the sandbox image's: ``repositories.yaml::build_commands`` builds the repo's pinned @@ -106,14 +179,28 @@ logger = get_logger("orchestrator.slice_green_gate") -# Operator switch for the green gate. Three-state, default off during -# rollout (#3398): "off"/unset → gate skipped entirely; "log" → checks -# run and a red verdict is logged loudly but never blocks; "on" → a red -# verdict blocks the slice PR from opening. +# Operator switch for the green gate. Three-state, default "on": +# "off" → gate skipped entirely; "log" → checks run and a red verdict is +# logged loudly but never blocks; "on"/unset → a red verdict blocks the +# slice PR from opening. GREEN_GATE_ENV_VAR = "EGG_SLICE_GREEN_GATE" _ENABLED_VALUES = frozenset({"on", "1", "true", "yes"}) _LOG_ONLY_VALUES = frozenset({"log", "log-only", "log_only"}) +_DISABLED_VALUES = frozenset({"off", "0", "false", "no"}) + +# Mode used when the switch is unset or carries a value we do not +# recognise. Weakening the gate takes an explicit, correctly-spelled +# _DISABLED_VALUES or _LOG_ONLY_VALUES entry: a typo must not leave a +# deployment with less verification than the product default. Note the +# earlier "a typo must not start blocking slices" half of this rule +# retired with the "log" default — under an "on" default, setting +# nothing at all blocks, so a mistyped value resolving to "on" is no +# stricter than the deployment an operator gets by doing nothing. This +# is also the same rule _infra_fail_open_enabled() applies (unrecognised +# → the default, loudly), so both switches in this module degrade +# alike. +_DEFAULT_MODE: Literal["off", "log", "on"] = "on" # Comma-separated check *names* (from repositories.yaml ``checks``) the # gate skips. Default skips ``security``: the full scan belongs on the @@ -125,8 +212,16 @@ # red verdict where every failed check matches an infra signature fails # open instead of blocking. "off" (or 0/false/no) restores the strict # pre-#3417 behavior where every red blocks. Any other value degrades -# to the default, matching green_gate_mode's typo posture. +# to the default *and logs a warning*, matching green_gate_mode's typo +# posture on both the resolution and the signal: the typo direction +# here is strict -> lenient, so it must not be silent. GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR = "EGG_SLICE_GREEN_GATE_INFRA_FAIL_OPEN" +# Deliberately separate from _ENABLED_VALUES / _DISABLED_VALUES above, +# not a missed dedup: this is an independent operator switch, and the +# two are free to diverge (e.g. if the mode switch grows a fourth state). +# The alias sets happening to be equal today is a coincidence to +# preserve, not a duplication to collapse. +_INFRA_FAIL_OPEN_ENABLED_VALUES = frozenset({"on", "1", "true", "yes"}) _INFRA_FAIL_OPEN_DISABLED_VALUES = frozenset({"off", "0", "false", "no"}) # Exact output signatures that identify an infrastructure fault inside a @@ -184,13 +279,30 @@ # Extra wall-clock the orchestrator's wait loop allows on top of the # in-pod check budget, to absorb pod scheduling + image-pull latency. -# The wait clock starts at Job submit, before the pod is scheduled/pulled -# — so on a cold node a long image pull would otherwise eat into the -# check budget and trip a spurious fail-open timeout even when the checks -# would have passed. The pod's own ``activeDeadlineSeconds`` (which the -# kubelet counts from pod *start*, i.e. after scheduling) still caps the -# actual check duration, so a genuinely hung check is killed by the pod -# deadline rather than lingering for the full grace-padded wait. +# The wait clock starts at Job submit, before the pod is scheduled or +# pulled — so on a cold node that latency would otherwise be charged to +# the orchestrator's own timeout and trip a spurious fail-open even when +# the checks would have passed. +# +# The grace widens the *orchestrator's* wait only; it does not protect +# the check budget. The deadline this module sets is the **Job's** +# ``activeDeadlineSeconds`` (``spec.activeDeadlineSeconds``, see +# ``_build_runner_job_manifest`` / ``_submit_runner_job``), which Kubernetes +# counts from the Job's ``status.startTime`` — set by the controller +# *before* any pod is bound. ``PodSpec.activeDeadlineSeconds``, the field +# the kubelet would count from pod start on the node, is never set. So +# scheduling and image-pull time count against the deadline: a pod that +# waits N seconds for capacity gets N fewer seconds to run checks, and +# with ``backoffLimit: 0`` / ``restartPolicy: Never`` a ``DeadlineExceeded`` +# kill prints no verdict line at all, so the gate fails open with no +# verdict. It fails open via one of *two* branches, depending on timing: +# the Job controller deletes the active pod on ``DeadlineExceeded`` +# rather than leaving it terminal, so ``_wait_for_runner_pod`` normally +# never sees ``Succeeded``/``Failed`` and times out ("runner pod did not +# reach a terminal state"); a poll that catches the pod reporting +# ``Failed`` mid-termination instead reads partial output and lands on +# "no parseable verdict from runner". Tracked in #3622; text here +# describes what the code does today, not what it should do. _POD_SCHEDULING_GRACE_SECONDS = 120 # Per-check output tail retained in the verdict (runner side) and the @@ -349,26 +461,61 @@ def copy_if_missing(src, dst, **kwargs): def green_gate_mode() -> Literal["off", "log", "on"]: """Resolve the operator switch to one of ``off`` / ``log`` / ``on``. - Unknown values resolve to ``off``: during rollout an operator typo - must degrade to "gate does nothing", never to "gate blocks slices". + Unset resolves to ``_DEFAULT_MODE`` (``on``). Weakening the gate + requires an explicit, correctly-spelled value — ``off`` / ``0`` / + ``false`` / ``no`` to skip it, ``log`` / ``log-only`` / ``log_only`` + to run it without blocking. Any other value resolves to ``on`` and + logs a warning, so a typo cannot silently drop the deployment below + the product default. """ - raw = os.environ.get(GREEN_GATE_ENV_VAR, "off").strip().lower() + raw = os.environ.get(GREEN_GATE_ENV_VAR, "").strip().lower() + if not raw: + return _DEFAULT_MODE if raw in _ENABLED_VALUES: return "on" if raw in _LOG_ONLY_VALUES: return "log" - return "off" + if raw in _DISABLED_VALUES: + return "off" + logger.warning( + "Unrecognised green-gate switch value; falling back to the default mode", + env_var=GREEN_GATE_ENV_VAR, + value=raw, + mode=_DEFAULT_MODE, + ) + return _DEFAULT_MODE def _infra_fail_open_enabled() -> bool: """Resolve the #3417 infra-red fail-open switch (default on). Only the exact disabled values turn it off; anything else degrades - to the default. Mirrors ``green_gate_mode``'s posture: an operator - typo resolves to the documented default behavior. + to the default *and logs a warning*. Mirrors ``green_gate_mode``'s + posture on both counts: an operator typo resolves to the documented + default behavior, and it never does so silently. The warning matters + more here than on the mode switch, because the two typos point in + opposite directions. A mistyped ``EGG_SLICE_GREEN_GATE`` resolves to + ``on``, the strictest mode, so it can only over-verify. A mistyped + value here resolves to fail-open, so an operator reaching for ``off`` + and typing ``offf`` gets the *lenient* posture — silently, without + the warning. """ raw = os.environ.get(GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, "on").strip().lower() - return raw not in _INFRA_FAIL_OPEN_DISABLED_VALUES + if not raw or raw in _INFRA_FAIL_OPEN_ENABLED_VALUES: + return True + if raw in _INFRA_FAIL_OPEN_DISABLED_VALUES: + return False + logger.warning( + "Unrecognised green-gate infra-fail-open switch value; " + "falling back to the default (fail open on all-infra reds)", + env_var=GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, + value=raw, + # Structured resolved value, mirroring green_gate_mode's + # ``mode=`` kwarg, so both typo warnings are greppable the + # same way rather than carrying the resolution in prose only. + fail_open=True, + ) + return True def _gate_checks(repo: str) -> list[dict[str, str]]: @@ -504,9 +651,12 @@ def _build_runner_job_manifest( # only extends lifetime when the orchestrator crashed before # reaching it (probe precedent). "ttlSecondsAfterFinished": 300, - # Give the in-pod checks the full budget; the orchestrator's - # wait loop enforces the same ceiling, and the deadline - # guarantees a hung check terminates rather than lingering. + # Job-level deadline: counted from the Job's ``startTime``, + # i.e. from before the pod is bound, so scheduling and pull + # latency come out of the check budget (#3622 — see + # ``_POD_SCHEDULING_GRACE_SECONDS``). It bounds a hung check + # rather than sizing it; the orchestrator's wait loop is the + # outer ceiling. "activeDeadlineSeconds": timeout_seconds + 60, "backoffLimit": 0, "template": { @@ -550,11 +700,23 @@ def _build_runner_job_manifest( def _submit_runner_job(k8s: Any, namespace: str, manifest: dict[str, Any]) -> None: - """Convert the manifest dict to V1 objects and create the Job.""" + """Convert the manifest dict to V1 objects and create the Job. + + Every field is read *from the manifest* rather than restated here. + Three of them used to be hardcoded (``automountServiceAccountToken``, + ``allowPrivilegeEscalation``, ``capabilities.drop``) while the + manifest also declared them: the values agreed, so there was no live + bug, but the dict was not the source of truth it looks like, and a + test asserting on the submitted body could not tell the two apart. + Keep new fields flowing through the dict — + ``test_every_manifest_field_reaches_the_body`` fails on any manifest + key this function does not copy. + """ from kubernetes import client as k8s_client_pkg container = manifest["spec"]["template"]["spec"]["containers"][0] pod_spec = manifest["spec"]["template"]["spec"] + container_security = container["securityContext"] body = k8s_client_pkg.V1Job( api_version=manifest["apiVersion"], kind=manifest["kind"], @@ -572,7 +734,7 @@ def _submit_runner_job(k8s: Any, namespace: str, manifest: dict[str, Any]) -> No ), spec=k8s_client_pkg.V1PodSpec( restart_policy=pod_spec["restartPolicy"], - automount_service_account_token=False, + automount_service_account_token=pod_spec["automountServiceAccountToken"], security_context=k8s_client_pkg.V1PodSecurityContext( run_as_user=pod_spec["securityContext"]["runAsUser"], run_as_group=pod_spec["securityContext"]["runAsGroup"], @@ -589,8 +751,12 @@ def _submit_runner_job(k8s: Any, namespace: str, manifest: dict[str, Any]) -> No for e in container["env"] ], security_context=k8s_client_pkg.V1SecurityContext( - allow_privilege_escalation=False, - capabilities=k8s_client_pkg.V1Capabilities(drop=["ALL"]), + allow_privilege_escalation=container_security[ + "allowPrivilegeEscalation" + ], + capabilities=k8s_client_pkg.V1Capabilities( + drop=container_security["capabilities"]["drop"], + ), ), volume_mounts=[ k8s_client_pkg.V1VolumeMount( @@ -900,7 +1066,14 @@ def run_slice_green_gate( if verdict is None: # Covers pod Failed (runner harness crashed — the verdict is # printed even when checks are red, so a missing verdict is - # never a check failure) and unparseable output. + # never a *check* failure), unparseable output, and a Job + # ``activeDeadlineSeconds`` kill caught mid-termination, where + # the checks may have been about to pass and the budget was + # simply cut short by scheduling delay (#3622 — see + # ``_POD_SCHEDULING_GRACE_SECONDS``). A deadline kill more + # often lands on the "did not reach a terminal state" branch + # above, since the Job controller deletes the pod; check both + # when diagnosing a missing verdict. logger.warning( "Green gate skipped: no parseable verdict from runner (#3398)", pipeline_id=pipeline_id, diff --git a/orchestrator/tests/test_review_findings_verdict.py b/orchestrator/tests/test_review_findings_verdict.py index b794abe63..e323b9ef2 100644 --- a/orchestrator/tests/test_review_findings_verdict.py +++ b/orchestrator/tests/test_review_findings_verdict.py @@ -16,7 +16,9 @@ - mechanism-level dedup attaches >=2 producing lenses to a merged finding and raises its confidence; - ``review_findings_mode`` resolves a flag typo to ``off`` (never silently to - ``on``), exactly like ``slice_green_gate.green_gate_mode``; + ``on``) — the shared staged ``off``/``log``/``on`` shape, but with an + ``off``-default, unlike ``slice_green_gate.green_gate_mode`` which now + defaults to ``on`` and degrades a typo to ``on`` + a warning; - ``verdict_log_record`` records the computed verdict against the legacy one. The matrix-integration half (``record_findings_verdict`` routing + the diff --git a/orchestrator/tests/test_risk_router_wiring.py b/orchestrator/tests/test_risk_router_wiring.py index 0f190ce7c..3b2af5a0b 100644 --- a/orchestrator/tests/test_risk_router_wiring.py +++ b/orchestrator/tests/test_risk_router_wiring.py @@ -3,9 +3,10 @@ Slice-5 (``orchestrator/risk_router.py``) is the PURE half — a deterministic function from a changed-file set to (lenses, tier, stance). Slice-6 is the WIRING half: it threads that pure decision into the live review machinery behind -the ``EGG_RISK_ROUTER`` staged flag (``off`` default / ``log`` / ``on``, resolved -exactly like ``slice_green_gate.green_gate_mode()``). The wiring lives in three -seams: +the ``EGG_RISK_ROUTER`` staged flag (``off`` default / ``log`` / ``on`` — the +shared staged shape ``slice_green_gate.green_gate_mode()`` uses, but keeping an +``off``-default, unlike that resolver which now defaults to ``on``). The wiring +lives in three seams: * ``review_graph`` — ``risk_router_mode`` / ``resolve_risk_decision`` / ``apply_risk_router`` / ``risk_route_log_record`` and the ``changed_files`` diff --git a/orchestrator/tests/test_slice_green_gate.py b/orchestrator/tests/test_slice_green_gate.py index 66b2be9de..91d8d722e 100644 --- a/orchestrator/tests/test_slice_green_gate.py +++ b/orchestrator/tests/test_slice_green_gate.py @@ -2,8 +2,8 @@ Covers: -* ``green_gate_mode`` — the three-state operator switch (off default, - log soak mode, on; unknown values degrade to off). +* ``green_gate_mode`` — the three-state operator switch (on default, + explicit off/log; unrecognised values degrade to the default). * ``_gate_checks`` / ``_repo_requires_prebuilt`` — config-driven check selection (skip set, default ``security``) and the prebuilt-toolchain requirement derived from ``build_commands.persist_dirs``. @@ -16,6 +16,13 @@ * ``_build_runner_job_manifest`` — labels (NetworkPolicy component label present; monitor/agent-supervision labels absent), env, mounts, deadline. +* ``_submit_runner_job`` — the manifest-dict -> V1 object translation, + asserted on the ``create_namespaced_job`` body: the function copies a + fixed field list, so a field present in the manifest but absent from + that list is dropped silently (including the pod-level deadline + #3622 tracks). A reflection pass over the whole manifest closes that + class of drop generally, with a negative control for the dict-only + shape of #3622's fix. * ``run_slice_green_gate`` — gate wiring: kill switch, fail-open on every infrastructure failure (worktree, session, submit, timeout, unparseable verdict), fail-closed only on a definitive red verdict, @@ -81,8 +88,16 @@ def gate_env(monkeypatch: pytest.MonkeyPatch) -> pytest.MonkeyPatch: class TestGreenGateMode: - def test_default_is_off(self, gate_env: pytest.MonkeyPatch) -> None: - assert sgg.green_gate_mode() == "off" + def test_default_is_on(self, gate_env: pytest.MonkeyPatch) -> None: + """Unset resolves to the product default: a blocking gate. + + The #3398 switch shipped defaulting to ``off`` and was never set + in any deployment, so the check-runner path never executed. The + default is the rollout, and #3602 — a task marked ``complete`` + over five failing tests on the slice tip — is a shape only a + *blocking* gate prevents. + """ + assert sgg.green_gate_mode() == "on" @pytest.mark.parametrize("value", ["on", "ON", " true ", "1", "yes"]) def test_enabled_values(self, gate_env: pytest.MonkeyPatch, value: str) -> None: @@ -94,11 +109,87 @@ def test_log_values(self, gate_env: pytest.MonkeyPatch, value: str) -> None: gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, value) assert sgg.green_gate_mode() == "log" - @pytest.mark.parametrize("value", ["off", "0", "false", "", "banana", "enabled"]) - def test_everything_else_is_off(self, gate_env: pytest.MonkeyPatch, value: str) -> None: + @pytest.mark.parametrize("value", ["off", "OFF", " off ", "0", "false", "no"]) + def test_disabled_values(self, gate_env: pytest.MonkeyPatch, value: str) -> None: + """Turning the gate off now takes an explicit disable value.""" gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, value) assert sgg.green_gate_mode() == "off" + @pytest.mark.parametrize( + ("value", "logged"), + [ + ("banana", "banana"), + ("enabled", "enabled"), + ("tru", "tru"), + ("onn", "onn"), + # The disable-side typo — the direction that matters now + # that the default blocks. + ("offf", "offf"), + # Padded/uppercase inputs pin what the resolver actually + # logs — the post-strip/lower value, not the raw env string. + # Without these the assertion below passes either way. + (" Onn ", "onn"), + ("BANANA", "banana"), + ], + ) + def test_unrecognised_values_degrade_to_default_with_a_warning( + self, gate_env: pytest.MonkeyPatch, value: str, logged: str + ) -> None: + """A typo cannot silently weaken the gate. + + ``"tru"`` / ``"onn"`` are the realistic shapes: an operator + reaching for ``on`` must not land on "gate does nothing". + ``"offf"`` is the other direction and the reason the resolved + value is pinned to ``on`` rather than merely "not off": someone + reaching for the escape hatch and missing gets the product + default, loudly, instead of a silently ungated deployment. The + warning is asserted alongside the return value because a + misconfiguration that resolves silently is the failure mode this + default flip exists to close. + """ + gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, value) + with patch.object(sgg.logger, "warning") as warn: + assert sgg.green_gate_mode() == sgg._DEFAULT_MODE == "on" + assert warn.call_count == 1 + assert warn.call_args.kwargs["value"] == logged + assert warn.call_args.kwargs["env_var"] == sgg.GREEN_GATE_ENV_VAR + # Resolved value as a structured field, matching the shape + # ``_infra_fail_open_enabled``'s warning uses (``fail_open=``), + # so both typo warnings are greppable the same way. + assert warn.call_args.kwargs["mode"] == "on" + + @pytest.mark.parametrize( + ("value", "expected"), + [ + ("on", "on"), + ("log", "log"), + ("off", "off"), + ("", "on"), + (" ", "on"), + ], + ) + def test_recognised_values_do_not_warn( + self, gate_env: pytest.MonkeyPatch, value: str, expected: str + ) -> None: + """Only genuinely unrecognised values warn. + + ``""`` / ``" "`` are deliberately in this list rather than the + one above: both resolvers short-circuit on a falsy value + (``green_gate_mode``'s ``if not raw`` / ``_infra_fail_open_enabled``'s + ``if not raw or ...``), so an unset-equivalent value resolves to + the default *silently* on both sides. Pinning it here keeps that + symmetry from drifting — an empty value is "unset", not a typo, + and should not page an operator. + + The resolved mode is asserted alongside the warn count so that + the ``""`` / ``" "`` rows still pin *what* the falsy + short-circuit returns, not merely that it is quiet about it. + """ + gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, value) + with patch.object(sgg.logger, "warning") as warn: + assert sgg.green_gate_mode() == expected + assert warn.call_count == 0 + # ---------------------------------------------------------------------- # _gate_checks / _repo_requires_prebuilt / _gate_timeout_seconds @@ -165,11 +256,53 @@ def test_disabled_values(self, gate_env: pytest.MonkeyPatch, value: str) -> None gate_env.setenv(sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, value) assert sgg._infra_fail_open_enabled() is False - @pytest.mark.parametrize("value", ["on", "1", "true", "", "banana"]) - def test_everything_else_is_on(self, gate_env: pytest.MonkeyPatch, value: str) -> None: + @pytest.mark.parametrize("value", ["on", "ON", " 1 ", "true", "yes", "", " "]) + def test_enabled_values(self, gate_env: pytest.MonkeyPatch, value: str) -> None: gate_env.setenv(sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, value) assert sgg._infra_fail_open_enabled() is True + @pytest.mark.parametrize( + ("value", "logged"), + [ + ("banana", "banana"), + ("offf", "offf"), + ("disabled", "disabled"), + ("fals", "fals"), + # Padded/uppercase inputs pin what the resolver actually + # logs — it warns with the post-strip/lower value, not the + # raw env string. Without these the assertion below passes + # for either choice. + (" Offf ", "offf"), + ("BANANA", "banana"), + ], + ) + def test_unrecognised_values_degrade_to_default_with_a_warning( + self, gate_env: pytest.MonkeyPatch, value: str, logged: str + ) -> None: + """A typo resolves to the lenient default, but never silently. + + ``"offf"`` / ``"fals"`` are the realistic shapes: an operator + reaching for the strict every-red-blocks posture lands back on + fail-open, so the resolution needs a log line — same posture + ``green_gate_mode`` takes on its own unrecognised values. + """ + gate_env.setenv(sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, value) + with patch.object(sgg.logger, "warning") as warn: + assert sgg._infra_fail_open_enabled() is True + assert warn.call_count == 1 + assert warn.call_args.kwargs["value"] == logged + assert warn.call_args.kwargs["env_var"] == sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR + # Resolved value carried as a structured field, mirroring + # ``green_gate_mode``'s ``mode=`` kwarg. + assert warn.call_args.kwargs["fail_open"] is True + + @pytest.mark.parametrize("value", ["on", "off", ""]) + def test_recognised_values_do_not_warn(self, gate_env: pytest.MonkeyPatch, value: str) -> None: + gate_env.setenv(sgg.GREEN_GATE_INFRA_FAIL_OPEN_ENV_VAR, value) + with patch.object(sgg.logger, "warning") as warn: + sgg._infra_fail_open_enabled() + assert warn.call_count == 0 + class TestGateTimeout: def test_default(self, gate_env: pytest.MonkeyPatch) -> None: @@ -571,6 +704,199 @@ def test_runs_as_host_uid(self) -> None: assert ctx == {"runAsUser": 1234, "runAsGroup": 5678, "fsGroup": 5678} +# ---------------------------------------------------------------------- +# _submit_runner_job +# ---------------------------------------------------------------------- + + +def _real_k8s_sdk_available() -> bool: + """True only for the genuine kubernetes SDK, not the conftest stub. + + ``conftest`` installs a hand-rolled stub package under + ``sys.modules`` when ``kubernetes`` is absent, so + ``pytest.importorskip("kubernetes")`` would *succeed* and then fail + on the first missing class. The stub defines nine V1 classes; + ``_submit_runner_job`` needs six more, and the reflection test below + needs ``attribute_map``, which only the real SDK models carry. + """ + try: + from kubernetes import client as k8s_client_pkg + except ImportError: # pragma: no cover — SDK is in the ``dev`` extra + return False + return hasattr(getattr(k8s_client_pkg, "V1Job", None), "attribute_map") + + +def _assert_manifest_value_reaches(submitted: Any, expected: Any, path: str) -> None: + """Assert every key of ``expected`` survives into ``submitted``. + + Walks the manifest dict and the submitted V1 object in lockstep, + translating camelCase keys through each model's ``attribute_map`` + (present on every kubernetes SDK model, so no hand-maintained name + table can drift). A manifest key that ``_submit_runner_job`` does + not copy shows up as a ``None`` attribute or a value mismatch. + """ + if isinstance(expected, dict): + attribute_map = getattr(type(submitted), "attribute_map", None) + if attribute_map is None: + # A plain-dict field (e.g. ``labels``) — compare wholesale. + assert submitted == expected, f"{path} != {expected!r}" + return + inverse = {camel: attr for attr, camel in attribute_map.items()} + for camel, value in expected.items(): + assert camel in inverse, f"{path}.{camel} is not a field of {type(submitted).__name__}" + got = getattr(submitted, inverse[camel]) + assert got is not None, f"{path}.{camel} dropped by _submit_runner_job" + _assert_manifest_value_reaches(got, value, f"{path}.{camel}") + elif isinstance(expected, list): + assert isinstance(submitted, list), f"{path} is not a list" + assert len(submitted) == len(expected), f"{path} length {len(submitted)} != {len(expected)}" + for i, item in enumerate(expected): + _assert_manifest_value_reaches(submitted[i], item, f"{path}[{i}]") + else: + assert submitted == expected, f"{path} is {submitted!r}, manifest says {expected!r}" + + +@pytest.mark.skipif( + not _real_k8s_sdk_available(), + reason="needs the real kubernetes SDK (the conftest stub lacks V1PodSecurityContext " + "and attribute_map); it ships in the dev extra CI installs", +) +class TestSubmitRunnerJob: + """Pin the manifest-dict -> V1 object translation. + + ``_submit_runner_job`` hand-copies a *fixed* field list out of the + manifest dict; anything it does not name is silently dropped, and + every ``run_slice_green_gate`` test patches it out. So a manifest + assertion alone cannot prove a field reaches the apiserver — the + concrete trap being #3622's cheapest option ("move + ``activeDeadlineSeconds`` onto ``spec.template.spec``"), which would + pass ``test_one_shot_job_shape`` while this function dropped the + field and the submitted Job carried no pod deadline at all. + + ``test_every_manifest_field_reaches_the_body`` is what actually + closes that trap, and closes it for the whole class of fields rather + than one field: it walks the manifest and the submitted body in + lockstep. The per-field tests below stay because they name the + values that matter (deadline arithmetic, the security posture) in + terms a reader recognises; the reflection test is the one that goes + red when a *new* key is added to the dict and not to the whitelist. + """ + + @staticmethod + def _submit(manifest: dict[str, Any] | None = None, **overrides: Any) -> Any: + manifest = _manifest(**overrides) if manifest is None else manifest + k8s = MagicMock() + sgg._submit_runner_job(k8s, "egg-ns", manifest) + k8s.batch_api.create_namespaced_job.assert_called_once() + kwargs = k8s.batch_api.create_namespaced_job.call_args.kwargs + assert kwargs["namespace"] == "egg-ns" + return kwargs["body"] + + def test_job_spec_fields_reach_the_apiserver(self) -> None: + body = self._submit(timeout_seconds=600) + assert body.api_version == "batch/v1" + assert body.kind == "Job" + assert body.metadata.labels[sgg._GATE_ID_LABEL] == "abc123def456" + assert body.spec.active_deadline_seconds == 660 + assert body.spec.backoff_limit == 0 + assert body.spec.ttl_seconds_after_finished == 300 + + def test_pod_spec_fields_reach_the_apiserver(self) -> None: + body = self._submit(host_uid=1234, host_gid=5678) + pod = body.spec.template.spec + assert pod.restart_policy == "Never" + assert pod.automount_service_account_token is False + assert pod.security_context.run_as_user == 1234 + assert pod.security_context.run_as_group == 5678 + assert pod.security_context.fs_group == 5678 + assert body.spec.template.metadata.labels[sgg._GATE_ID_LABEL] == "abc123def456" + + def test_container_and_volumes_reach_the_apiserver(self) -> None: + body = self._submit() + container = body.spec.template.spec.containers[0] + assert container.name == "green-gate" + assert container.image_pull_policy == "IfNotPresent" + assert container.command[:2] == ["python3", "-c"] + assert container.security_context.allow_privilege_escalation is False + assert container.security_context.capabilities.drop == ["ALL"] + env = {e.name: e.value for e in container.env} + assert env["EGG_GREEN_GATE_REPO_DIR"] == "/home/egg/repos/egg" + assert json.loads(env["EGG_GREEN_GATE_CHECKS"]) == CHECKS + volume = body.spec.template.spec.volumes[0] + assert volume.host_path.path == "/home/host/.egg-worktrees/x/egg" + mount = container.volume_mounts[0] + assert mount.mount_path == "/home/egg/repos/egg" + assert mount.name == volume.name + + def test_pod_level_deadline_is_not_set_today(self) -> None: + """The deadline is Job-level only — the defect #3622 tracks. + + This records *where the deadline is today*; it does not enforce + that #3622's fix arrives at the apiserver. It cannot: the + assertion below stays true whether the fix is complete or the + manifest dict grew a pod-level deadline that + ``_submit_runner_job`` then dropped on the floor. That trap is + closed by ``test_every_manifest_field_reaches_the_body``, which + goes red on the dict-only version. + + When #3622 lands, flip this to ``== ``. + """ + body = self._submit(timeout_seconds=600) + assert body.spec.active_deadline_seconds == 660 + assert body.spec.template.spec.active_deadline_seconds is None + + def test_every_manifest_field_reaches_the_body(self) -> None: + """No manifest key may be dropped by the copy whitelist. + + The generalised form of the #3622 trap: add a key to + ``_build_runner_job_manifest`` and forget the matching line in + ``_submit_runner_job``, and the manifest tests stay green while + the apiserver never sees the field. + """ + manifest = _manifest() + _assert_manifest_value_reaches(self._submit(manifest), manifest, "body") + + def test_a_dict_only_pod_deadline_is_caught(self) -> None: + """Negative control for #3622 option 1, landed dict-only. + + This is the exact edit the follow-up is most likely to make — + pod-level ``activeDeadlineSeconds`` in the manifest, no matching + line in ``_submit_runner_job``. Without this the suite has no + way to distinguish it from the complete fix. + """ + manifest = _manifest(timeout_seconds=600) + manifest["spec"]["template"]["spec"]["activeDeadlineSeconds"] = 660 + with pytest.raises(AssertionError, match="activeDeadlineSeconds dropped"): + _assert_manifest_value_reaches(self._submit(manifest), manifest, "body") + + @pytest.mark.parametrize( + ("path", "value"), + [ + (("automountServiceAccountToken",), True), + (("containers", 0, "securityContext", "allowPrivilegeEscalation"), True), + (("containers", 0, "securityContext", "capabilities", "drop"), []), + ], + ) + def test_security_fields_follow_the_manifest_not_a_constant( + self, path: tuple[Any, ...], value: Any + ) -> None: + """The three fields that used to be hardcoded in the submitter. + + They agreed with the manifest, so there was no live bug — but + the assertions on them passed against a constant and would have + held with the manifest saying the opposite, which is the + non-discriminating shape ``TestSubmitRunnerJob`` exists to + replace. Loosening the manifest here must loosen the submitted + pod; if it does not, the dict is decorative again. + """ + manifest = _manifest() + target: Any = manifest["spec"]["template"]["spec"] + for key in path[:-1]: + target = target[key] + target[path[-1]] = value + _assert_manifest_value_reaches(self._submit(manifest), manifest, "body") + + # ---------------------------------------------------------------------- # run_slice_green_gate # ---------------------------------------------------------------------- @@ -653,6 +979,13 @@ class TestRunSliceGreenGate: def test_kill_switch_off_skips_everything( self, gate_env: pytest.MonkeyPatch, configured_checks: None ) -> None: + """``off`` still short-circuits before any spawn side effect. + + This must set the switch explicitly: the default is ``on``, so + an implicit-off assertion here would silently start exercising + the runner. + """ + gate_env.setenv(sgg.GREEN_GATE_ENV_VAR, "off") spawner = _spawner() assert _run_gate(spawner) is None spawner.gateway.create_worktrees.assert_not_called() @@ -787,6 +1120,31 @@ def test_red_verdict_blocks_with_actionable_message( spawner.gateway.delete_session_by_container.assert_called_once() spawner.gateway.delete_worktrees.assert_called_once() + def test_unset_switch_blocks_on_a_red_verdict( + self, gate_env: pytest.MonkeyPatch, configured_checks: None + ) -> None: + """The default deployment — nothing set — withholds the PR. + + Every other blocking test here sets the switch to ``on`` + explicitly, so without this one the resolver's ``on`` default + could regress to ``log`` and the whole suite would stay green. + This is the seam that makes the default load-bearing. + """ + spawner = _spawner() + log = _verdict_line([{"name": "test", "ok": False, "exit_code": 2, "output_tail": "x"}]) + with ( + patch.object(sgg, "_submit_runner_job"), + patch.object(sgg, "_wait_for_runner_pod", return_value=_terminal_pod()), + patch.object(sgg, "_read_runner_log", return_value=log), + patch.object(sgg, "_delete_runner_job"), + ): + failure = _run_gate(spawner) + assert failure is not None + assert "test" in failure + # The bypass is quoted in the message itself: an operator who + # hits an unexpected red must not have to find the env var. + assert sgg.GREEN_GATE_ENV_VAR in failure + def test_red_verdict_in_log_mode_does_not_block( self, gate_env: pytest.MonkeyPatch, configured_checks: None ) -> None: