Skip to content

Pre-register the gpt-5.6-sol -> gpt-6-astra Codex reviewer campaign (PR-1: harness re-pin, no model swap) - #849

Merged
igerber merged 1 commit into
mainfrom
codex-upgrade
Oct 17, 2026
Merged

igerber merged 1 commit into
mainfrom
codex-upgrade

Conversation

@igerber

@igerber igerber commented Oct 16, 2026

Copy link
Copy Markdown
Owner

Summary

  • PR-1 of 2 for the Codex reviewer upgrade gpt-5.6-sol -> gpt-6-astra: pre-registration + harness re-pin, NO production model change. The candidate arm's first corpus run is the campaign itself (which starts only after this PR is frozen); PR-2 carries the verdict and, on GO, the swap of the CI reviewer, the local review script's default model and the plan-review engine.
  • tools/reviewer-eval: new pre-registered DECISION_RULE.md (B = gpt-5.6-sol@xhigh control vs E = gpt-6-astra@xhigh candidate, k=2; symmetric regression gate over every ground-truth bug, FP gate, downgrade rule, verifier-confirmed leaks void a cell and contest the gate, precedence (0)-(7), stop conditions, campaign ledger + frozen git ref, candidate-exposure gate); the 2026-07 rule is archived under campaigns/. Grader-facing GRADING_RUBRIC.md split off (marker-based LEAK? flag whose list renders from reviewer_audit.LEAK_RULES; graders never see gates or history).
  • New engines: reviewer_verdict.py (gates, void/contest handling, binding to the sealed artifacts, protocol guards, PROVISIONAL/DEFINITIVE, informational campaign reads) and reviewer_audit.py (post-unblinding session-log audit: literal + regex + computed markers, three coverage classes, informational and over-matching by design).
  • run_eval.py: audit and verdict subcommands; every codex call runs under an isolated CI-faithful CODEX_HOME (credential + sandbox/approval only; no MCP, hooks, memories); seven-key git provenance per artifact and in the manifest with aggregates; artifacts sealed at run time (artifacts_sha); blinded bundle bound to the artifacts (bundle_id, run_ids_sha, rubric/prompt sha, deterministic regeneration at verdict); tree-identity and hidden-input guards on every post-run stage; provenance preflight, append-only attempts log (3-rerun cap, consecutive-timeout stop, resume integrity), campaign ledger anchored by refs/campaign/<control>-<candidate>/frozen; review worktree leaves are salted opaque hashes.
  • Harness CLI re-pinned to codex-cli 0.162.1 on every arm (arm E added); CI codex-version: 0.162.1 pinned and lockstep-tested with DEFAULT_MODEL, the plan-review CODEX_MODEL, the control arm and the prose pins in CLAUDE.md/SKILL.md.
  • .claude/scripts/openai_review.py: call_codex parses codex's stderr session events into session_commands (+ start/exec/other-event counts, truncation flag) on every exit path and prints a one-line summary; --timeout now also caps the codex backend when passed explicitly (0 = no timeout on both backends, negatives rejected); PRICING corrected for gpt-5.6-* and extended with gpt-6-*; gpt-6 is a reasoning family; blinding scrubs gpt-6/astra and Unicode-dash spellings.
  • Probes recorded in config/configs.json: gpt-6-astra @ xhigh accepted on codex-cli 0.162.1 through the exact _build_codex_cmd argv; control harness smoke on 0.162.1 OK (8/8 exec blocks parsed); off-corpus astra latency probe on PR fix(synthetic_control): put the treated unit in every in-space placebo's donor pool #841 = 325 s wall-clock, so CALL_TIMEOUT_S stays 3600. Under codex defaults BOTH models web-search routinely, so most audit cells will be unaudited (accepted; see the rule).
  • Tracking: TODO.md row (answer-key isolation for corpus worktrees), DEFERRED.md row (gpt-6-sol cost probe needs an API-key login); rust-test.yml gains a 3.9 in-checkout harness step and path filters for the lockstep surfaces.

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no estimator or methodology changes (reviewer-evaluation infrastructure only; docs/methodology/REGISTRY.md untouched)
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: tests/test_openai_review.py, tests/test_evals_runtime.py, tests/test_evals_engine.py, tests/test_plan_review_skill.py, new tests/test_reviewer_verdict.py, tests/test_reviewer_audit.py, fixtures tests/fixtures/codex_stderr_0_162_1*.txt (sanitized event-structure transcripts). Targeted suites pass on Python 3.14 and 3.9 (uv). Pre-freeze rehearsal (control arm, k=1, the two S1 cases) run on this HEAD: provenance gate OK, blinded compare + audit OK, verdict refused exactly at the protocol guard, both tamper tests refused.
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes (committed stderr fixtures redact prompts, outputs, review text and absolute paths; rotated-credential copies are written owner-only)

Changelog

  • changelog.d/ fragment added (or N/A - no user-visible change): Yes (changelog.d/20261016-reviewer-eval-gpt6-prereg.md, ### Internal)

@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 2cdd09c0342cdeb6eb137de2a28181e8cb864a13


Overall assessment

⛔ Blocker — one P0 campaign-integrity defect and one P1 resume-integrity defect.

Executive summary

  • No estimator, identification, weighting, variance, SE, or default-methodology changes.
  • Concurrent runs sharing a subdirectory can silently mix artifacts from different invocations.
  • Aborted attempts omit already-persisted successful cells, permitting selective resampling.
  • Bare run uses incompatible defaults and always refuses.
  • Changelog coverage is present; documented deferred items are non-blocking.

Methodology

No findings. No causal-inference methods are affected, and docs/methodology/REGISTRY.md is unchanged.

Code Quality

Concurrent invocations can corrupt the campaign artifact set

  • Severity: P0
  • Location: tools/reviewer-eval/run_eval.py:L1050-L1111; tools/eval_core/runner.py:L298-L311
  • Impact: Two processes using the same --subdir generate identical run IDs and write the same artifact files without exclusive ownership. Last-writer behavior can leave a mixture of cells from both invocations, which the final process can seal and verdict successfully. A direct concurrency probe reproduced a store containing one cell from each invocation.
  • Concrete fix: Acquire a cross-process, per-subdirectory lock before preflight and hold it through manifest sealing. Add a two-process regression test asserting the second invocation refuses and artifacts come exclusively from the lock owner.

Bare run is configured to fail

  • Severity: P2
  • Location: tools/reviewer-eval/run_eval.py:L1693-L1704
  • Impact: Defaults select B,E but retain --k 1; the candidate-exposure gate requires k=2, so python run_eval.py run always refuses.
  • Concrete fix: Default this campaign to k=2, or default to a non-candidate configuration. Add a test covering the complete default argument combination.

Performance

No findings.

Maintainability

Aborted attempts lose successful-cell integrity records

  • Severity: P1
  • Location: tools/reviewer-eval/run_eval.py:L1112-L1131, tools/reviewer-eval/run_eval.py:L454-L478
  • Impact: The runner persists completed cells incrementally, but a post-dispatch exception records ok_cells: []. Resume validation skips such END records, so an artifact completed before interruption may be deleted or changed and silently rerun, violating the pre-registered no-resampling rule.
  • Concrete fix: Track each persisted result through a callback or attempt journal and include its hash in aborted END records. Resume validation should protect the cumulative successful-cell set. Test interruption after one successful save, followed by deletion or modification.

Tech Debt

  • Severity: P3 informational — answer-key isolation is explicitly tracked in TODO.md; no PR action required.
  • Severity: P3 informational — the gpt-6-sol cost probe is explicitly tracked in DEFERRED.md; no PR action required.

Security

No accidental secrets were found in the added fixtures or reviewed credential paths.

Documentation/Tests

No release-note defect: changelog.d/20261016-reviewer-eval-gpt6-prereg.md is present.

All changed Python modules parse successfully. The focused pytest suites could not be executed because pytest is unavailable in this environment. Official OpenAI documentation was also unavailable under the restricted network, so the revised pricing table could not be independently verified.

Path to Approval

  1. Add exclusive per-subdirectory run ownership and a concurrent-invocation regression test.
  2. Preserve and validate successful artifact hashes from aborted attempts, with interruption/resume tests.

@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: 940e178da27c651d1910d700c379a28babedf5b6


Overall assessment

⚠️ Needs changes — three P1 findings remain, including a partially unresolved prior finding.

Executive summary

  • The prior concurrency blocker and broken default run configuration are fixed.
  • Aborted-attempt journaling still has a post-save interrupt gap.
  • rank_control_units can silently mishandle NaN/Inf inputs.
  • The tutorial overstates SCM identification and CWZ coverage without their required assumptions.
  • Estimator behavior changes lack an accurate changelog fragment.

Methodology

[Newly identified] Tutorial overstates identification and conformal coverage

  • Severity: P1
  • Location: docs/tutorials/25_synthetic_control_policy.ipynb:L317-L320, L927-L995
  • Impact: The tutorial states that SCM “identifies” the counterfactual path despite non-unique weights, and describes CWZ intervals as calibrated while listing only a long pre-period as necessary. The cited sources require the linear-factor identifying conditions, good predictor/pre-outcome balance with sufficiently large T0 or small transitory shocks, and—under moving-block conformal inference—stationary weakly dependent residuals whose distribution is invariant to treatment. Non-unique pre-fit weights alone do not guarantee a unique post-treatment counterfactual.
  • Concrete fix: Qualify the SCM statement with the ADH factor-model conditions and sensitivity to alternative admissible weights. Add the CWZ stationarity, dependence, heteroscedasticity, and intervention-invariance assumptions beside the interval interpretation and comparison table.

The affected methods are classic Synthetic Control, in-space diagnostics, and CWZ conformal inference. The change from NNLS to Frank–Wolfe is itself a valid numerical implementation choice: it solves the same registered simplex-constrained objectives and is not a methodology defect.

Code Quality

[Newly identified] Non-finite ranking inputs silently produce invalid scores or fallback weights

  • Severity: P1
  • Location: diff_diff/prep.py:L989-L1075
  • Impact: rank_control_units passes matrices containing NaN/Inf directly into noise estimation and Frank–Wolfe, suppresses non-convergence warnings, and then normalizes the result. NaN solver output can silently become uniform weights, while infinite treated outcomes can make every RMSE and quality_score NaN, leaving an arbitrary ranking rather than rejecting invalid data.
  • Concrete fix: Validate finite pre-period treated/control matrices, or implement an explicit documented missing-data policy before optimization. Request the convergence flag and warn or fail closed on non-convergence. Add tests for partial missing cells, all-missing donor cells, and ±Inf in treated and control outcomes.

Performance

No findings.

Maintainability

Prior P1 remains: save and abort journaling are not atomic

  • Severity: P1
  • Location: tools/eval_core/runner.py:L316-L322; tools/reviewer-eval/run_eval.py:L1163-L1182
  • Impact: A result is atomically persisted before on_result adds it to seen. An interrupt immediately after store.save()—reproducible with a store that calls super().save() and then raises—leaves a successful artifact on disk but absent from the aborted END record. It can subsequently be deleted and re-dispatched, violating the registered no-resampling rule.
  • Concrete fix: On abort, reconstruct the current attempt’s successful cells from the expected run IDs in the store and union them with seen, or otherwise make persistence and durable journaling one operation. Add a regression test that raises after the first successful os.replace, before the callback.

Tech Debt

  • Severity: P3 informational
  • Impact: Answer-key isolation and the gpt-6-sol cost probe remain limited.
  • Concrete fix: No PR action required; they are tracked in TODO.md:L107 and DEFERRED.md:L126.

Security

No findings. Credential copies are owner-only, and no apparent secrets were found in the fixtures.

Documentation/Tests

Estimator behavior changes are missing from release notes

  • Severity: P2
  • Location: diff_diff/synthetic_control.py:L114-L123; changelog.d/20261016-reviewer-eval-gpt6-prereg.md:L1-L28
  • Impact: Public solver behavior and active inner_max_iter/inner_min_decrease parameters changed, but the only added fragment documents reviewer infrastructure.
  • Concrete fix: Add an accurate behavioral fragment describing the Frank–Wolfe solver, active convergence controls, and possible numerical/output changes.

All changed Python files parsed successfully. Tests could not be executed because pytest, NumPy, and uv are unavailable. Official OpenAI pricing documentation was also unavailable in this restricted environment.

Path to Approval

  1. Protect artifacts saved immediately before an abort and test the post-save/pre-callback interrupt.
  2. Restore fail-closed NaN/Inf and convergence handling in rank_control_units.
  3. Add the missing ADH and CWZ validity assumptions to the tutorial.

@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: daf888672c931af775083d24f5496eaf2be511d9


Overall assessment

⚠️ Needs changes — one newly identified P1 remains.

Executive summary

  • No causal estimator, weighting, variance, identification, or default behavior changed; the Methodology Registry is unaffected.
  • The prior post-save abort-journaling issue is fixed.
  • A coordinator interruption can still discard completed reviews and selectively rerun them.
  • Prior SCM ranking/tutorial findings are resolved or no longer in this PR’s scope.
  • Changelog coverage is present; tracked isolation limitations are non-blocking.

Methodology

[Newly identified] Completed reviews can be selectively resampled after interruption

  • Severity: P1
  • Location: tools/eval_core/runner.py:L312-L325; tools/reviewer-eval/run_eval.py:L1163-L1184; tools/reviewer-eval/DECISION_RULE.md:L247-L262
  • Impact: Workers return successful reviews, but artifacts are persisted only while the coordinator consumes futures. If KeyboardInterrupt or another coordinator exception occurs, ThreadPoolExecutor waits for remaining workers to finish, yet their results are never saved. Abort recovery scans only the store, so those completed cells are omitted and rerun later, violating the pre-registered no-resampling rule. A reproduction with two successful reviews and interruption after the first done: callback persisted only one artifact.
  • Concrete fix: Persist each RunResult inside _execute() immediately before returning, then let abort recovery scan the planned keys as it does now. Add a deterministic test where two workers complete, the coordinator is interrupted after consuming one future, and both artifacts appear in the aborted END record and are reused on resume.

No causal-inference methods are affected.

Code Quality

No additional findings.

Performance

No findings.

Maintainability

No additional findings. The prior post-save/pre-callback gap is now covered by on_plan plus store reconstruction.

Tech Debt

  • Severity: P3 informational
  • Impact: Answer-key isolation and the gpt-6-sol cost probe remain limited.
  • Concrete fix: No PR action required; tracked in TODO.md:L109 and DEFERRED.md:L126.

Security

No findings. Credential rotations are written owner-only, and no apparent secrets remain in the sanitized fixtures.

Documentation/Tests

No separate findings. The changelog fragment follows the project convention and describes the user-visible tooling changes.

Static validation passed for all 15 changed Python files, configs.json, and rubric-rule rendering. Pytest was unavailable in this environment. Official OpenAI pricing documentation was also unreachable, so pricing claims could not be independently confirmed.

Path to Approval

  1. Persist worker results before coordinator consumption and add the coordinator-interruption regression test described above.

…x reviewer campaign

PR-1 of 2: no production model change. Pre-registers the 2026-10 campaign
(DECISION_RULE.md; the 2026-07 rule is archived under campaigns/), splits the
grader-facing GRADING_RUBRIC.md off it, adds the reviewer_verdict.py and
reviewer_audit.py engines plus run_eval.py audit/verdict, runs every harness
codex call under an isolated CI-faithful CODEX_HOME, records seven-key git
provenance per artifact and in the manifest, seals artifacts at run time and
binds the blinded bundle, and makes review worktree leaves salted opaque hashes.
Harness CLI re-pinned to codex-cli 0.162.1 (arm E = gpt-6-astra @ xhigh added),
CI codex-version pinned to the same and lockstep-tested with DEFAULT_MODEL, the
plan-review pin, the control arm and the prose pins.

openai_review.py: call_codex parses the codex stderr session events into
session_commands (+ counts, truncation flag) on every exit path and prints a
summary line; --timeout caps the codex backend too (0 = none, negatives
rejected); PRICING corrected for gpt-5.6-* and extended with gpt-6-*; gpt-6 is
a reasoning family; blinding scrubs gpt-6/astra and Unicode-dash spellings.
@github-actions

Copy link
Copy Markdown

🔁 AI review rerun (requested by @igerber)

Head SHA: ec6226ae6335691df4c602a6783d1278ea3c47d9


Overall assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive summary

  • No causal estimator, weighting, variance, identification, or default behavior changed.
  • The prior interruption/resampling P1 is resolved: workers persist results before coordinator consumption.
  • A targeted regression test covers coordinator interruption and cached resume.
  • Changelog coverage is present; remaining isolation limitations are explicitly tracked.

Methodology

No findings. The Methodology Registry is unaffected because this PR changes reviewer-evaluation infrastructure only.

Code Quality

No findings. The prior P1 is fixed in tools/eval_core/runner.py:L276-L330, with abort recovery in tools/reviewer-eval/run_eval.py:L1147-L1187.

Performance

No findings.

Maintainability

No findings.

Tech Debt

Tracked reviewer isolation limitations

  • Severity: P3 informational
  • Impact: Corpus answer-key isolation and the cheaper-model cost probe remain incomplete.
  • Concrete fix: No PR action required; tracked in TODO.md:L109 and DEFERRED.md:L126.

Security

No unmitigated findings. Rotated credentials are created owner-only, and the committed stderr fixtures appear sanitized.

Documentation/Tests

No findings.

The changelog fragment follows the repository convention. The exact interruption regression is at tests/test_evals_runtime.py:L3394. All 15 changed Python files parse successfully. Pytest was unavailable, and the read-only environment provided no writable temporary directory, so tests could not be executed. Official OpenAI documentation was also unavailable, so the future-dated pricing claims could not be independently verified.

@igerber igerber added the ready-for-ci Triggers CI test workflows label Oct 17, 2026
@igerber
igerber merged commit 225a4af into main Oct 17, 2026
52 of 54 checks passed
@igerber
igerber deleted the codex-upgrade branch October 17, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci Triggers CI test workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant