Skip to content

chore: use one pre-push Codex review and disable automatic review disposition - #72

Open
doublewhy wants to merge 2 commits into
devfrom
60-pre-push-review-caps
Open

doublewhy wants to merge 2 commits into
devfrom
60-pre-push-review-caps

Conversation

@doublewhy

@doublewhy doublewhy commented Oct 9, 2026 •

Copy link
Copy Markdown

Summary

Brings .ground-control.yaml's pre-push review settings in line with the current reference, OpenRAE/rae's .ground-control.yaml on dev:

  • one pre-push Codex review cycle;
  • automatic review disposition off: shadow mode, no auto-overrides, judge off.

Per #60, reaching the review cap then always escalates to a human, instead of letting the authoritative auto path grant an extra cycle. Configuration only: no code, skill or CI change. Unlike the issue text, this PR does not declare test_quality_review; see "Deviation from the issue text".

Stacked on #74, so merge it after #74. The branch now carries #74's uv.lock fix (commit 6ee2564; #74 is the PR for #76) under this PR's own commit 143ba96, which is unchanged. That clears CI's audit job, which failed here only on dev's lock. Once #74 is merged, the squash of this PR changes only .ground-control.yaml.

Requirement UIDs

Related Issues

Refs #60

ADR Impact

  • None.

Changes

.ground-control.yaml, workflow: block only (+6/−4):

  • added codex_review.pre_push_cap: 1;
  • review_disposition.enabled true → false;
  • review_disposition.mode authoritative → shadow;
  • review_disposition.max_auto_overrides 1 → 0;
  • review_disposition.judge.enabled true → false.

test_command, completion_command, lint_command, format_command and every other top-level key are unchanged.

uv.lock is carried from #74 (6ee2564) and not edited by this PR's own commit; #74 has the details.

Deviation from the issue text

#60 (2026-07-26) also asks for test_quality_review.pre_push_cap: 1, which is acceptance criterion 2. It names rae's .ground-control.yaml on dev as the reference, and at the time the issue matched rae commit 46cd7693 ("Configure pre-push review limits", 2026-07-26). The reference has since changed. git show origin/dev:.ground-control.yaml in OpenRAE/rae (dev at 35122105) now declares:

  codex_review:
    pre_push_cap: 1
  review_disposition:
    enabled: false
    mode: shadow
    max_auto_overrides: 0
    judge:
      enabled: false

test_quality_review was removed from the reference and from two sibling repositories:

  • rae #1317 (ad151246, 2026-09-17);
  • lilrae #1109 (e6bef3c6, 2026-09-18);
  • env-packs #382 (443248d, 2026-09-18). Its comment now reads "One pre-push Codex review cycle, with automatic disposition of the review cap off ... This matches Ground Control's current default".

The env-packs commit also added a test that keeps the key out. tests/test_ground_control_review_config.py::ReviewConfigPlacementTests::test_retired_test_quality_review_key_is_absent asserts that workflow has no test_quality_review key, with the message "Ground Control retired workflow.test_quality_review; keeping it makes .ground-control.yaml invalid". The test is still on env-packs dev (9ce449d).

Two sibling repositories still declare the key. OpenRAE/hub and OpenRAE/adapters carry test_quality_review.pre_push_cap: 1 on dev, in files last edited before the retirement: hub a203171 (2026-07-30) and adapters f05ef0c (2026-08-02). Removing the key there is tracked in OpenRAE/hub#48 and OpenRAE/adapters#103.

This PR matches the current reference and does not add the retired key, so criterion 2 is deliberately unmet. That is why this PR uses Refs #60 rather than a closing keyword. Criterion 2 is the only #60 item this PR leaves unmet, and removing it from #60 is a maintainer decision on the issue text. With that change, this PR completes #60, and its Related Issues route moves from the reference to a closing keyword.

Test Plan

  • Reference match: compare_gc_ref.py, a PyYAML script inlined in the collapsed block at the end of this section, prints PASS for:
    • workflow.codex_review, workflow.test_quality_review (absent from both) and workflow.review_disposition, each equal to rae dev's;
    • every other workflow: key unchanged;
    • every other top-level key unchanged.
    • Negative control: the unchanged dev file fails the codex_review and review_disposition comparisons (exit 1).
  • Issue criteria: check_gc60.py, a script encoding chore: use one pre-push Codex review, one pre-push test-quality review, and disable automatic review disposition #60's seven acceptance criteria (same block), passes every check except workflow.test_quality_review.pre_push_cap is 1, which fails as intended because the key is absent (see the deviation above).
  • uvx pre-commit run --files .ground-control.yaml (pre-commit 4.6.2): every commit-stage hook passed, including check-yaml and gitleaks.
  • .gc/plan-rules.md requires uv run ruff check ., uv run ruff format --check . and uv run pytest before completion. Locally, uvx ruff@0.15.21 check . (the ruff version uv.lock pins) printed All checks passed! and uvx ruff@0.15.21 format --check . printed 47 files already formatted. I did not run pytest locally because nothing Python-related changed. CI's test job runs all three (its Lint, Format check and Test with coverage steps). It passed at the earlier head 6ca4e9f in run 38075834128 and at the restacked head 143ba96 in run 38196310542 ("148 passed").
  • Not exercised: the Ground Control /implement workflow that reads these keys is not available in this environment.
  • Restack onto chore(deps): update locked cryptography, django, pyjwt and sqlparse for published advisories #74 (head 6ee2564): git rebase --onto origin/uv-lock-vulnerability-fixes origin/dev replayed the one commit, and git range-diff reports it unchanged (6ca4e9f = 143ba96). .ground-control.yaml is byte-identical to 6ca4e9f's (the raw file at both heads has the same SHA-256), so the script results below still hold. uv.lock is byte-identical to chore(deps): update locked cryptography, django, pyjwt and sqlparse for published advisories #74's, and uv lock --check resolves 93 packages (exit 0). uvx pre-commit run --files .ground-control.yaml uv.lock exits 0.
  • Audit, as CI runs it: uv export --frozen --no-dev --all-extras --no-emit-project --no-hashes --format requirements-txt, then uvx pip-audit --strict --no-deps on the export printed "No known vulnerabilities found" (exit 0). Locally I added --disable-pip, because pip-audit's temporary virtual environment aborts in ensurepip on this machine; with --no-deps it audits the same pins. On dev's lock the same commands print "Found 27 known vulnerabilities in 4 packages" (exit 1).
  • CI at 143ba96: audit passes ("No known vulnerabilities found") and test passes, both in run 38196310542. PR title guard (38196310576), CodeQL actions, javascript-typescript and python (38196308603) and GitGuardian also pass.
  • sonar is the only red check, and no code change can fix it: it stops before analysis because SonarCloud rejects the repository SONAR_TOKEN ("HTTP 403 Forbidden" in run 38196310542, as in the earlier run 38075834128). Only the maintainer can replace the secret; tracked in ci: SonarCloud rejects the repository SONAR_TOKEN #75.
Rerun both scripts (bash or zsh; needs curl and uv)

The inputs are pinned to this PR's head (143ba96), its dev base (b923768) and rae dev (35122105). The three runs end with exit 0, exit 1 (negative control) and exit 1 (criterion 2).

d=$(mktemp -d)
s=$(mktemp -d)
raw=https://raw.githubusercontent.com/OpenRAE
curl -fsSL -o "$d/new.yaml" "$raw/workbench/143ba9627b40b419ffa1665e9a95389df0954a29/.ground-control.yaml"
curl -fsSL -o "$d/dev.yaml" "$raw/workbench/b92376883a0611495cd14d28cec8b36dfce5b9aa/.ground-control.yaml"
curl -fsSL -o "$d/rae.yaml" "$raw/rae/35122105b0c1bb648754625d8e3920eafa9bc599/.ground-control.yaml"
cat > "$s/compare_gc_ref.py" <<'EOF'
"""Compare the review keys of two .ground-control.yaml files and check nothing else moved.

usage: python compare_gc_ref.py <workbench-new> <workbench-dev> <rae-dev>
"""
import sys

import yaml

new, old, ref = (yaml.safe_load(open(p, encoding="utf-8")) for p in sys.argv[1:4])
REVIEW = ("codex_review", "test_quality_review", "review_disposition")
status = 0
for key in REVIEW:
    ok = new["workflow"].get(key) == ref["workflow"].get(key)
    print(f"{'PASS' if ok else 'FAIL'} workflow.{key} == rae dev: {new['workflow'].get(key)!r}")
    status |= not ok
others_new = {k: v for k, v in new["workflow"].items() if k not in REVIEW}
others_old = {k: v for k, v in old["workflow"].items() if k not in REVIEW}
ok = others_new == others_old
print(f"{'PASS' if ok else 'FAIL'} other workflow keys unchanged")
status |= not ok
ok = {k: v for k, v in new.items() if k != "workflow"} == {k: v for k, v in old.items() if k != "workflow"}
print(f"{'PASS' if ok else 'FAIL'} top-level keys outside workflow unchanged")
status |= not ok
sys.exit(status)
EOF
cat > "$s/check_gc60.py" <<'EOF'
"""Check a .ground-control.yaml against issue #60's seven acceptance criteria.

usage: python check_gc60.py <workbench-new> <workbench-dev>
"""
import sys

import yaml

new, old = (yaml.safe_load(open(p, encoding="utf-8")) for p in sys.argv[1:3])
NAMED = ("codex_review", "test_quality_review", "review_disposition")
CRITERIA = [  # criteria 1-6: (key path under workflow, required value)
    ("codex_review.pre_push_cap", 1),
    ("test_quality_review.pre_push_cap", 1),
    ("review_disposition.enabled", False),
    ("review_disposition.mode", "shadow"),
    ("review_disposition.max_auto_overrides", 0),
    ("review_disposition.judge.enabled", False),
]
status = 0
for path, want in CRITERIA:
    got = new["workflow"]
    for key in path.split("."):
        got = got.get(key) if isinstance(got, dict) else None
    ok = type(got) is type(want) and got == want  # a YAML `true` must not satisfy a pin of 1
    print(f"{'PASS' if ok else 'FAIL'} workflow.{path} is {want!r} (got {got!r})")
    status |= not ok
others = [{k: v for k, v in doc["workflow"].items() if k not in NAMED} for doc in (new, old)]
ok = others[0] == others[1]  # criterion 7
print(f"{'PASS' if ok else 'FAIL'} no other workflow: keys changed")
status |= not ok
sys.exit(status)
EOF
uv run --no-project --with pyyaml python -I "$s/compare_gc_ref.py" "$d/new.yaml" "$d/dev.yaml" "$d/rae.yaml"; echo "exit $?"
uv run --no-project --with pyyaml python -I "$s/compare_gc_ref.py" "$d/dev.yaml" "$d/dev.yaml" "$d/rae.yaml"; echo "exit $?"
uv run --no-project --with pyyaml python -I "$s/check_gc60.py" "$d/new.yaml" "$d/dev.yaml"; echo "exit $?"

Ground Control Checks

  • No Ground Control MCP gate (gc_evaluate_quality_gates, gc_run_sweep) was run from this environment, and the repository has no make policy target.

Traceability

Checklist

Documentation

Verified unchanged: no documentation describes these keys (git grep finds them only in .ground-control.yaml).

@doublewhy
doublewhy force-pushed the 60-pre-push-review-caps branch from d910aa9 to c56955f Compare October 9, 2026 08:07
@doublewhy doublewhy changed the title chore: use one pre-push Codex review, one pre-push test-quality review, and disable automatic review disposition chore: use one pre-push Codex review and disable automatic review disposition Oct 9, 2026
@doublewhy
doublewhy marked this pull request as ready for review October 9, 2026 08:46
…or published advisories

Upgrade only these four uv.lock entries, within the existing pyproject.toml
constraints:

- cryptography 49.0.0 -> 50.0.2 (PYSEC-2026-3552)
- django 5.2.16 -> 5.2.18 (PYSEC-2026-3717, PYSEC-2026-4035)
- pyjwt 2.13.0 -> 2.15.1 (PYSEC-2026-4140 to PYSEC-2026-4152, PYSEC-2026-4183)
- sqlparse 0.5.5 -> 0.6.0 (PYSEC-2026-3696 to PYSEC-2026-3699, PYSEC-2026-3923)

CI's audit job runs pip-audit --strict over the locked runtime dependencies
and fails on dev with 27 known vulnerabilities in these packages (22 distinct
advisories: pip-audit lists PYSEC-2026-3552 and PYSEC-2026-3696 to
PYSEC-2026-3699 twice); with this lock it reports none. No other package is
added, removed or moved.

Closes #76
@doublewhy
doublewhy force-pushed the 60-pre-push-review-caps branch from c56955f to 6ca4e9f Compare October 10, 2026 18:27
…position

Declare one pre-push Codex review cycle and turn automatic review
disposition off (shadow mode, no auto-overrides, judge off), so reaching
the review cap always escalates to a human. These are the values in
OpenRAE/rae's .ground-control.yaml on dev. test_quality_review is not
declared: it was retired from rae (ad151246), lilrae (e6bef3c6) and
env-packs (443248d). Every other key is unchanged.

Refs #60

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant