fix(agent-team): P3-flip — resolve /sh-security-review findings (LOGIC-1/2/3)
High-recall fan-out (injection/logic/iac+secrets) + proof-or-kill on the LIVE apply path found 3 real issues the cross-review missed; all fixed: - LOGIC-2 (HIGH, was a live hole): build-test ran `ruff check . || echo` / `pytest -q || echo`, swallowing failures so the job was always 'success' and the gate would open draft PRs on RED builds. ruff/pytest now run authoritatively under set -e (pytest exit 5 'no tests' is the only non-fatal case); the exit code IS the build-test conclusion the gate keys on. - LOGIC-1 (verified!=shipped): the dispatcher used `git apply` + `git add -A`, staging stray untracked content into the pushed PR head. Now `git apply --index` stages exactly the diff, so the head tree is precisely base+diff — bound to the bytes CI hash-verified. - LOGIC-3 (§4.5 on the live path): gate-weakening was enforced only box-side; added a gate-weakening check to the guard job so the live PR-opening path rejects a diff that adds suppressions/skips, even on a green build. Injection / secrets / least-privilege / flip-correctness / no-untrusted-checkout all came back clean. 1044 tests, ruff clean, YAML valid.
This commit is contained in:
parent
a388b9c813
commit
c4a77e14b2
3 changed files with 100 additions and 11 deletions
|
|
@ -253,14 +253,16 @@ def _default_branch_pusher() -> BranchPusher:
|
||||||
check=True,
|
check=True,
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
)
|
)
|
||||||
|
# --index applies AND stages exactly the diff's changes (incl. new
|
||||||
|
# files) and NOTHING else — so the committed head tree is precisely
|
||||||
|
# base+diff, never stray untracked worktree content. This keeps the
|
||||||
|
# PR head bound to the same bytes CI hash-verified (LOGIC-1). No
|
||||||
|
# separate `git add -A` (which would stage unrelated content).
|
||||||
subprocess.run(
|
subprocess.run(
|
||||||
["git", "-C", str(clone), "apply", str(diff_path)],
|
["git", "-C", str(clone), "apply", "--index", str(diff_path)],
|
||||||
check=True,
|
check=True,
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
)
|
)
|
||||||
subprocess.run(
|
|
||||||
["git", "-C", str(clone), "add", "-A"], check=True, capture_output=True
|
|
||||||
)
|
|
||||||
subprocess.run(
|
subprocess.run(
|
||||||
[
|
[
|
||||||
"git",
|
"git",
|
||||||
|
|
|
||||||
|
|
@ -531,6 +531,52 @@ jobs:
|
||||||
sys.exit(main())
|
sys.exit(main())
|
||||||
PY
|
PY
|
||||||
|
|
||||||
|
- name: Gate-weakening check (§4.5; a diff cannot disable its own checks)
|
||||||
|
env:
|
||||||
|
DIFF_PATH: ./_incoming/candidate.diff
|
||||||
|
run: |
|
||||||
|
set -euo pipefail
|
||||||
|
# SECURITY (§4.5): enforce gate-weakening on the LIVE PR-opening path,
|
||||||
|
# not only box-side. A diff that ADDS a lint/type/coverage/security
|
||||||
|
# suppression or a test skip/xfail could make build-test pass falsely;
|
||||||
|
# such a diff is escalated to a human, never auto-built. Mirrors
|
||||||
|
# agent_team.ci_gate._GATE_WEAKENING_MARKERS. Reads the diff as DATA.
|
||||||
|
python3 - <<'PY'
|
||||||
|
import os
|
||||||
|
import re
|
||||||
|
import sys
|
||||||
|
|
||||||
|
markers = (
|
||||||
|
"noqa",
|
||||||
|
"type: ignore",
|
||||||
|
"type:ignore",
|
||||||
|
"pragma: no cover",
|
||||||
|
"pragma: no-cover",
|
||||||
|
"nosec",
|
||||||
|
"nosemgrep",
|
||||||
|
"--no-verify",
|
||||||
|
)
|
||||||
|
skip_re = re.compile(
|
||||||
|
r"@(?:pytest\.mark\.(?:skip|xfail)|unittest\.skip\w*)\b"
|
||||||
|
r"|\bpytest\.(?:skip|xfail)\s*\("
|
||||||
|
r"|\.skipTest\s*\("
|
||||||
|
)
|
||||||
|
text = open(os.environ["DIFF_PATH"], encoding="utf-8", errors="replace").read()
|
||||||
|
viol = []
|
||||||
|
for line in text.splitlines():
|
||||||
|
if line.startswith("+") and not line.startswith("+++"):
|
||||||
|
content = line[1:]
|
||||||
|
low = content.lower()
|
||||||
|
if any(m in low for m in markers) or skip_re.search(content):
|
||||||
|
viol.append(content.strip()[:120])
|
||||||
|
if viol:
|
||||||
|
print("::error::gate-weakening: a diff cannot add suppressions/skips that disable its own checks")
|
||||||
|
for v in viol[:20]:
|
||||||
|
print(f" + {v}")
|
||||||
|
sys.exit(1)
|
||||||
|
print("no gate-weakening markers in the diff")
|
||||||
|
PY
|
||||||
|
|
||||||
# ───────────────────────────────────────────────────────────────────────────
|
# ───────────────────────────────────────────────────────────────────────────
|
||||||
# JOB 2 — build-test (boundary 1). UNTRUSTED execution. This is the ONLY job
|
# JOB 2 — build-test (boundary 1). UNTRUSTED execution. This is the ONLY job
|
||||||
# that applies + runs the patch. It is credential-less: contents:read only, no
|
# that applies + runs the patch. It is credential-less: contents:read only, no
|
||||||
|
|
@ -637,20 +683,32 @@ jobs:
|
||||||
with:
|
with:
|
||||||
python-version: "3.12"
|
python-version: "3.12"
|
||||||
|
|
||||||
- name: Install + build + test (untrusted; result is non-authoritative)
|
- name: Install + build + test (untrusted; exit code IS authoritative)
|
||||||
id: run
|
id: run
|
||||||
run: |
|
run: |
|
||||||
set -euo pipefail
|
set -euo pipefail
|
||||||
# Placeholder build/test for the narrowest task class (dep bump /
|
# Build/test for the narrowest task class (dep bump / single-file fix).
|
||||||
# single-file fix). At deploy time this is parameterized per target
|
# SECURITY (boundary 4): this step's EXIT CODE is the authoritative
|
||||||
# repo. Exit code is what matters; any file the patch writes is
|
# build-test conclusion the pure-code gate keys on
|
||||||
# ignored by the authoritative gate (boundary 4).
|
# (needs.build-test.result). It must therefore FAIL on a red build/test
|
||||||
|
# — a previous `|| echo` swallowed ruff/pytest failures, which (now that
|
||||||
|
# the apply path is flipped live) would open draft PRs on red builds.
|
||||||
|
# The tool installs are best-effort (|| true), but ruff + pytest run
|
||||||
|
# AUTHORITATIVELY under `set -e` so a real failure fails the job.
|
||||||
|
#
|
||||||
|
# DEPLOY: the build/test invocation is parameterized per target repo at
|
||||||
|
# provisioning (a repo with no pytest suite needs its own command). Until
|
||||||
|
# then this generic ruff+pytest is the gate; a target whose tests need a
|
||||||
|
# different runner MUST be wired before it is dispatched.
|
||||||
if [ -f requirements.txt ]; then
|
if [ -f requirements.txt ]; then
|
||||||
python3 -m pip install --quiet -r requirements.txt || true
|
python3 -m pip install --quiet -r requirements.txt || true
|
||||||
fi
|
fi
|
||||||
python3 -m pip install --quiet ruff pytest || true
|
python3 -m pip install --quiet ruff pytest || true
|
||||||
ruff check . || echo "ruff non-zero (recorded, non-authoritative)"
|
ruff check .
|
||||||
pytest -q || echo "pytest non-zero (recorded, non-authoritative)"
|
# pytest exit 5 == "no tests collected"; treat ONLY that as non-fatal
|
||||||
|
# (a fix with no test suite), every other non-zero is an authoritative
|
||||||
|
# failure. Never blanket-swallow.
|
||||||
|
pytest -q || { rc=$?; [ "$rc" -eq 5 ] || exit "$rc"; echo "no tests collected (exit 5)"; }
|
||||||
|
|
||||||
- name: Post-build denied-path check (build hook may not write the trust surface)
|
- name: Post-build denied-path check (build hook may not write the trust surface)
|
||||||
if: always()
|
if: always()
|
||||||
|
|
|
||||||
|
|
@ -528,3 +528,32 @@ def test_materialize_bounds_decoded_diff_size() -> None:
|
||||||
assert steps
|
assert steps
|
||||||
for s in steps:
|
for s in steps:
|
||||||
assert "too large" in s["run"], "materialize must bound the decoded diff size"
|
assert "too large" in s["run"], "materialize must bound the decoded diff size"
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_test_step_does_not_swallow_failures() -> None:
|
||||||
|
# LOGIC-2: the build/test exit code is the authoritative build-test
|
||||||
|
# conclusion the gate keys on, so ruff/pytest must NOT be swallowed by a
|
||||||
|
# `|| echo ... non-authoritative` (which would open draft PRs on red builds).
|
||||||
|
steps = [
|
||||||
|
s for _, s in _iter_steps(_doc()) if s.get("run") and "ruff check" in s["run"]
|
||||||
|
]
|
||||||
|
assert steps, "expected the build/test step"
|
||||||
|
for s in steps:
|
||||||
|
run = s["run"]
|
||||||
|
assert 'echo "ruff non-zero' not in run, "ruff failure must not be swallowed"
|
||||||
|
assert 'echo "pytest non-zero' not in run, (
|
||||||
|
"pytest failure must not be swallowed"
|
||||||
|
)
|
||||||
|
# ruff runs authoritatively under set -e (bare invocation).
|
||||||
|
assert re.search(r"^\s*ruff check \.\s*$", run, re.MULTILINE), (
|
||||||
|
"ruff check must run authoritatively (no failure swallow)"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_guard_runs_gate_weakening_check_on_live_path() -> None:
|
||||||
|
# LOGIC-3: §4.5 enforced on the LIVE PR-opening path (guard), not only box-side.
|
||||||
|
guard_steps = _doc()["jobs"]["guard"]["steps"]
|
||||||
|
names = [s.get("name", "").lower() for s in guard_steps]
|
||||||
|
assert any("gate-weakening" in n for n in names), (
|
||||||
|
"guard must run a gate-weakening check so the live path enforces §4.5"
|
||||||
|
)
|
||||||
|
|
|
||||||
Reference in a new issue