External architecture review (Codex gpt-5.6-sol): 10 findings + pre-CPS refactor plan

[MaxGhenis]

External architecture review, run 2026-07-09 with Codex CLI (gpt-5.6-sol, ultra reasoning) against master 784bdf0 (candidates 1–14). Reproduced verbatim; findings unedited.

Overall: preserve the research ledger, but stop extending its execution architecture. The one-shot artifacts show unusually strong audit discipline; the candidate chain, contract file, and test layout are now too implicit for CPS deployment.

Scope correction: current clean origin/master at 784bdf0 contains candidates/artifacts 1–14, not 15; the suite itself asserts 14 artifacts at test_gates_derivations.py:1704. Also, only candidates 11–14 use types.FunctionType rebinding. Candidates 1–10 mix aliases, copied functions, and non-linear imports.

Findings

1. Major — __code__ identity does not prove unchanged behavior.

   Candidate 14 builds functions from its predecessor’s code object but supplies candidate 14’s global namespace at run_gate2_candidate14.py:606. Reused functions then resolve candidate-specific _widow_probs, simulate_holdout, fit_components, and similar names through those redirected globals at run_gate2_candidate14.py:630. It even retains an otherwise-unused hpanel global solely because inherited bytecode expects it at run_gate2_candidate14.py:129.

   The test confirms both same code object and new globals at test_gate2_candidate14.py:310. That proves instruction-stream reuse, not transitive semantic identity: any redirected global can change behavior while __code__ is remains true. The draw-by-draw outcome comparisons are the real behavioral proof.

   Recommendation: replace namespace rebinding with explicit dependency injection. “Unchanged” should mean the same immutable component implementation ID and canonical parameter snapshot, backed by behavioral equivalence tests.

2. Major — The candidate ladder is a non-linear dependency graph without a stable component interface.

   Candidate 10 combines c1, c5, c6, c8, and c9 rather than simply extending c9 at run_gate2_candidate10.py:116. Candidate 12 directly imports eight historical runners at run_gate2_candidate12.py:105. The base Components dataclass declares only the original fields at run_gate2_candidate1.py:321, but later candidates dynamically attach fields at run_gate2_candidate12.py:702 and mutate inherited bundles in place at run_gate2_candidate14.py:554.

   Recommendation: candidate 15+ should use a flattened component registry.

   src/populace_dynamics/family_transitions/
     specs.py
     fitted.py
     registry.py
     components/
       first_marriage.py
       divorce.py
       widowhood.py
       remarriage.py
       fertility.py
       initial_state.py
     simulator.py
   
   src/populace_dynamics/validation/gate2/
     runner.py
     scoring.py
     artifact.py
   
   specs/gate2/candidate15.yaml
   scripts/run_gate2_candidate15.py   # CLI only
   

   Define immutable ComponentRef(kind, implementation_id, params) and CandidateSpec(candidate_id, contract_revision, components, protocol, declared_delta_paths). Candidate inheritance may be an authoring convenience, but registration must materialize and hash the complete resolved spec; runtime must never recursively import ancestors.

   First port the actual c14 lineage as an engineering replay:

   • c3 first-marriage model
   • c1 divorce
   • c14 widowhood level with c5 NCHS trend
   • c11 remarriage
   • c5 fertility
   • c9 marriage-count initial state
   • c12 entry-widowed and spouse-age components
   • c10 mean-over-20-draw scoring protocol

   Compare the port against v14’s committed 20×46×5 rate cube, scores, pass flags, and verdict. Record a compatibility certificate; do not create a new candidate or overwrite v14. Candidate 15 can then declare a small, machine-checked diff against that flattened baseline.

3. Major — Historical runners and artifacts are not yet mechanically immutable.

   Candidate 14 records hashes for selected ancestors, but not its own runner or artifact at run_gate2_candidate14.py:1027. Its test checks only that stored hashes have 64 characters, rather than hashing the current files and comparing equality, at test_gate2_candidate14.py:854. Its CLI also unconditionally overwrites the committed artifact path at run_gate2_candidate14.py:1565.

   Recommendation: add an append-only sidecar manifest containing exact SHA-256 values for every c1–c14 runner and v1–v14 artifact, their originating commit, contract revision, environment identity, and registration. CI should recompute every digest. Leave all historical scripts and JSON files byte-for-byte at their existing paths; do not reformat them, turn them into wrappers, or redirect them to new src code. Full historical reproduction should run from the recorded commit/environment. New artifact creation should fail if the destination already exists.

4. Blocker — Gate 2 overclaims what it certifies.

   The gate description says household-composition transitions score against the listed holdouts at gates.yaml:634, and holdout_basis includes MX23REL at gates.yaml:672. The same contract later says the MX23REL household tranche is not scored and marriage×earnings is uncovered at gates.yaml:693 and gates.yaml:700. Tests merely confirm those statements exist at test_gates_derivations.py:1011.

   A future pass could therefore be interpreted as certifying household and auxiliary-benefit readiness when it certifies only marital/fertility tranches.

   Recommendation: split the contract into explicit tranches, for example:

   • gate_2a_marital_fertility: locked
   • gate_2b_relationship_household: unlocked
   • gate_2c_marriage_earnings_joint: unlocked

   Each downstream benefit provision should declare the tranche IDs it requires. Do not authorize CPS household-benefit deployment from a 2a pass alone.

5. Major — “Locked” currently means arithmetically consistent, not immutable or environment-bound.

   Derivations cite mutable artifact paths without digests at gates.yaml:218, while the test simply reads whatever file currently occupies that path at test_gates_derivations.py:29. A coordinated edit to the floor, multiplier, and threshold could therefore pass. Amendment history is largely prose and retains ambiguous pointers such as flipped_live: this pull request at gates.yaml:577.

   The environment rule is also not satisfied by the main Gate-1 floor chain. The contract requires matching recorded classifier versions at gates.yaml:569, but diagnostics explicitly identify sklearn 1.9.0 for the committed floor versus 1.8.0 for candidate scoring at c10_diagnostics_v1.json:241. Dependencies remain open-ended at pyproject.toml:40.

   Recommendation: introduce immutable contract revisions with parent hash, effective registration boundary, changed JSON pointers, artifact/schema hashes, and a derivation_context_id covering Python, NumPy, sklearn, scorer-code digest, and dependency lock. Preserve historical artifact bytes and backfill metadata through sidecars; new version-sensitive floors must use exactly the candidate scoring environment.

6. Major — The contract implementation will not scale to Gate 3.

   Gate 1 and Gate 2 have separate handwritten loaders and derivation logic at test_gates_derivations.py:24 and test_gates_derivations.py:819. Gate 3 is only an unlocked stub at gates.yaml:1210. Even within Gate 1, geometry has derivation parity, while the eight battery tolerances at gates.yaml:348 are tested only for the existence of reference keys at test_gates_derivations.py:82.

   Lifecycle drift is already visible: active Gate-2 comments still say “DRAFT” and locked stays false at gates.yaml:642, immediately before locked: true at gates.yaml:654. _gate_2_if_draft now always returns the live gate at test_gates_derivations.py:824.

   Recommendation: create a typed contracts package with generic GateSpec, ThresholdSpec, DerivationSpec, ProtocolSpec, and Amendment models plus one evaluator. Every locked numeric field should have exactly one structured source—floor formula, a-priori criterion, or identity—and explicit operator, scope, rounding mode, and artifact pointer. Use exact decimal arithmetic for contractual values. Store historical revisions separately and generate a clean current view.

7. Major — There is no source-neutral panel boundary for W1 CPS transport.

   The intended PERSON/EARNINGS/RELATIONSHIP/EVENT schema is documented at technical-specifications.md:163. Implementation instead exposes PSID-shaped frames and packed PSID identifiers at panels.py:10, with marriage-episode columns at marriage.py:259.

   data/transitions.py also crosses three boundaries: it constructs PSID panels, defines Gate-2 cells, and embeds the Social Security remarriage-at-60 rule at transitions.py:178. Its reference_moments is explicitly the union of every Gate-2 estimand at transitions.py:1093, so it is validation code, not data access.

   Recommendation: establish:

   • panel/{schema,ids,validation} for canonical tables, dtypes, keys, relationship integrity, and representation-factor semantics
   • sources/psid/ for raw PSID readers and adapters
   • adapters/populace/ for CPS/Populace inputs
   • models/family_transitions/ for fitted generators
   • validation/gate2/reference.py for observed estimands

   Use opaque, source-qualified IDs rather than treating the packed PSID integer as a universal person identifier. The existing harness is the strongest boundary in the repository and should remain a source-agnostic validation core.

8. Major — Social Security rules, empirical claiming, PolicyEngine integration, and filesystem access are intermingled.

   ss declares itself a frozen cross-engine oracle at ss/init.py:3. Yet claiming.py:5 combines a reference loader, empirical sampler, and statutory benefit adjustments; household.py:1 combines auxiliary arithmetic with PSID episode mapping. Despite its name, household.py emits couple/episode rows, not PolicyEngine household entities.

   The package is also checkout-dependent: claiming walks to repository-level data/external at claiming.py:80, while packaging includes only src at pyproject.toml:74. ss.params parses a sibling PolicyEngine-US checkout at params.py:187 and shells out to Git for provenance at params.py:309.

   Tracked documentation also conflicts with issue #100: it claims spousal and family-maximum calculations run through an existing PolicyEngine-US implementation at technical-specifications.md:243, while the source correctly states those formulas are absent upstream at params.py:20.

   Recommendation: commit an ADR capturing issue Integration design: dynamics → Populace panel → full tax-benefit incidence #100’s W1/W2/W3 ownership, then separate:

   • ss/oracle/{worker,actuarial,auxiliary,params}: pure frozen formulas
   • models/claiming/{reference,distribution}: empirical behavior
   • adapters/populace: panel mapping
   • adapters/policyengine_us: person-indexed social_security baseline/reform values or deltas—the W2 seam
   • provider interfaces for references and parameters

   Package immutable reference data with importlib.resources or require explicit artifact URIs. Keep environment/path resolution in CLIs. Preserve current imports temporarily through compatibility wrappers. W3 should upstream rules through Axiom/rulespec and cross-validate against the frozen oracle.

9. Major — Test runtime and artifact maintenance have no explicit tier policy.

   I collected 1,258 tests. Pytest defines no markers or runtime tiers at pyproject.toml:103. CI runs plain pytest -q without provisioning PSID at tests.yml:27, while local candidate tests activate expensive reproduction from an incidental home-directory path at test_gate2_candidate14.py:69.

   Historical tests also grow linearly: candidate 14 hard-codes outcome floats and moved-cell sets beginning at test_gate2_candidate14.py:155, while the global contract test hard-codes the current artifact count. Some assertions described as byte identity actually use pytest.approx(..., abs=1e-12) at test_gate2_candidate14.py:941, which is strong numeric reproduction, not byte reproduction.

   Recommendation: define unit, artifact, integration_psid, reproduction_legacy, and oracle_policyengine markers. PR CI should run unit plus static artifact/schema/digest validation with an enforced skip budget. Scheduled/private runners should shard PSID integration and historical reproduction; releases should run all tiers. Centralize data-location fixtures, cache loaded panels, and publish duration reports. Replace repeated artifact structure/verdict tests with one manifest-driven validator, while retaining candidate-specific scientific assertions only where they encode the declared delta.

10. Major — scripts/ is already the real application library.

The repository has 57 Python scripts totaling 60,389 lines, versus 7,285 lines under src. Candidate 1 mutates sys.path and imports floor builders at run_gate2_candidate1.py:99; candidate 9 makes a forensics module a production dependency at run_gate2_candidate9.py:99; replication rows imports private helpers across multiple builders and replication scripts at replication_mermin_rows.py:134.

Recommendation: promote code into src when any of these holds:

• another script or test imports it;
• it defines model, scoring, transformation, or contract semantics;
• more than one artifact reuses it;
• it needs independent unit tests.

Scripts should retain only CLI parsing, explicit configuration, API calls, and artifact I/O. Immediate promotion candidates are floor/reference computation, c2st_forensics_lib.py, inner_validation.py, weighted-statistic helpers, replication kernels, transport/career selection, and the new registry runner. Keep fetchers and one-shot entry points as scripts. Crucially, port reusable logic; do not edit or redirect the frozen historical scripts.

Prioritized pre-CPS refactor plan

1. P0 — Freeze and clarify the record. Add the c1–c14 legacy digest manifest, prevent artifact overwrite, split Gate 2 into explicit tranches, and commit the issue-100 ownership ADR.

2. P0 — Establish contract identity. Add typed contract revisions, artifact/environment sidecars, exact derivations, and a generic evaluator before defining Gate 3.

3. P1 — Replace the candidate architecture. Port the resolved c14 behavior into the component registry, issue the full compatibility certificate, and make c15 the first thin registry-driven runner.

4. P1 — Define the deployment data contract. Implement the canonical four-table panel schema, ID/weight semantics, PSID adapter, and Populace/CPS adapter before writing transport models.

5. P1 — Expose the actual PolicyEngine seam. Split the SS oracle and claiming model, package/inject resources, and implement person-level modeled social_security inputs/deltas.

6. P2 — Make validation operationally sustainable. Tier and shard the suite, consolidate artifact validation, promote reusable script code, then begin W1 CPS history imputation and its transport gates.

No files were modified; the working tree remains clean. I ran collection and targeted compatibility checks, not the full approximately 20-minute live-PSID suite.