fix(config): enforce reference wiring contract - #2016
Conversation
|
@ouroboros-agent please perform a fresh formal review of exact remote HEAD b5ed5b4, based directly on current protected main 9d23b21. This candidate addresses #1998 by making the config-reference relationship mechanically enforceable:
Exact-head evidence:
Please inspect both soundness directions: inert fields must not be mislabeled as wired, and real reads must not be lost through alias/collision handling. Merge remains frozen until exact-head official approval, fully green same-head CI, no human changes requested, latest-main ancestry, and independent verification all hold. |
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | b5ed5b49d226edbc388413a312f1c6e8bf326496 |
| Request ID | req_1786333016_1399-retry-1-retry-2 |
| Review record | c3f49ac1-fe9f-4880-a77b-44135994ba86 |
What Improved
- Adds a schema-driven audit for all
EvaluationConfigandConsensusConfigfields. - Documents the currently inert YAML controls and their effective direct-Python alternatives.
- Adds a CI gate plus extensive alias, mapping-mutation, collision, documentation, and model-default regression tests.
Issue Requirements
| Requirement | Status |
|---|---|
| Make configuration/reference drift mechanically detectable | Partially met — the gate exists, but unreachable references can create false wiring and section-typed helpers are missed. |
Require every EvaluationConfig and ConsensusConfig field to be runtime-read, documented inert, or allowlisted |
Partially met — all current schema fields receive a disposition, but runtime-read classification is not reliable for the cases above. |
| Preserve receiver mutation strength while conservatively handling aliases and dynamic mapping-key collisions | Met — implementation and focused mutation/collision tests cover the stated patterns. |
| Add the contract checker and CI lint gate | Met. |
| Add regression coverage for aliasing, collision, evaluation order, and model-default contracts | Met for the listed scenarios, but missing critical reachability and section-parameter coverage. |
| Keep inert and effective controls explicit in the config reference | Met for the current evaluation and consensus schemas. |
Prior Findings Status
No prior ouroboros-agent review rounds were present. No contributor comments or inline review discussions required reconciliation.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:903 | BLOCKING | visit_If unconditionally visits both branches, so reads inside statically unreachable code—including if TYPE_CHECKING: and if False:—are classified as production runtime wiring. A focused probe containing only if TYPE_CHECKING: return config.evaluation.stage1_enabled and if False: return config.consensus.min_models reported both fields as runtime reads. This allows an actually inert field to lose its inert marker and still pass the new CI contract. Exclude known non-runtime branches and add regression coverage proving type-only/dead references cannot satisfy a field disposition. |
Follow-up Findings
scripts/check-config-reference-contract.py:1191[warning]_argument_valuetreats any annotation containing “config” as the top-level config root. Consequently, the natural wiring patterndef build_pipeline(evaluation: EvaluationConfig): return evaluation.stage1_enabled, called withbuild_pipeline(config.evaluation), produces no runtime read: the section-typed parameter is assigned root provenance and call arguments are not propagated. This makes the mandatory CI gate reject valid helper-based wiring for either tracked section. RecognizeEvaluationConfig/ConsensusConfigas section provenance or propagate argument provenance across local calls, with a regression test covering this boundary.
| # | File:Line | Priority | Confidence | Suggestion |
|---|-----------|----------|------------|------------|
None.
Non-blocking Suggestions
None.
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python scripts/check-config-reference-contract.py: passed, reporting the expected six current production reads.- Contract-checker unit suite: 34 passed.
- Configuration model and loader suites: 336 passed.
- Ruff lint and format checks for the changed checker/tests passed.
- Additional focused probes reproduced both scanner defects described above.
Design Notes
The schema/docs classification design is directionally sound, but the AST analyzer is now a mandatory correctness gate and must distinguish executable wiring from dead/type-only syntax while supporting ordinary section-typed helper boundaries.
Design / Roadmap Gate
All fourteen fields in the two underlying Pydantic schemas were enumerated: six are currently read through runtime model resolvers and eight are explicitly documented inert. Current documentation matches the inspected runtime. The blockers are at the newly introduced CI contract boundary: it can both silently accept non-runtime references and reject valid helper-based runtime wiring, so it cannot yet serve as the authoritative drift gate claimed by the PR.
Directional Notes
Review focused on runtime reality rather than the checker’s self-authored passing matrix. Maintainer memory was used only to prioritize complete schema enumeration and model-default boundaries; both blocking findings were independently reproduced against the current source.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python scripts/check-config-reference-contract.py: passed, reporting the expected six current production reads.- Contract-checker unit suite: 34 passed.
- Configuration model and loader suites: 336 passed.
- Ruff lint and format checks for the changed checker/tests passed.
- Additional focused probes reproduced both scanner defects described above.
Merge Recommendation
Request changes. Fix the analyzer’s reachability handling and section-typed parameter provenance, then add focused regressions for both cases. The current field classifications and documentation otherwise appear consistent.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: b5ed5b4
request_id: req_1786333016_1399-retry-1-retry-2
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
b5ed5b4 to
2bbaddf
Compare
|
@ouroboros-agent please perform a fresh formal review of exact remote HEAD 2bbaddf against protected main 9b48ebe. Supersede all prior change-request reviews and do not reuse stale evidence. Exact-head evidence:
Please bind REST commit_id, body HEAD checked, and Review-Metadata head_sha to 2bbaddf, state blocker count explicitly, and give an explicit Approve/Merge or Request changes recommendation. Merge stays frozen until no human CHANGES_REQUESTED, all same-head required CI green, latest-main ancestry, and exact official approval all hold. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 2bbaddf1cc3347f0d0b22a71be17ff68cb5381d6 |
| Request ID | req_1786349299_1460 |
| Review record | 12a6999d-e4d9-400f-ac66-24d161f470fa |
What Improved
- Adds exhaustive schema enumeration for all fourteen
EvaluationConfigandConsensusConfigfields. - Documents the eight currently inert fields and their effective runtime controls more explicitly.
- Adds a CI contract gate plus substantial regression coverage for local aliasing, mutation, collision, control-flow, exception, and model-default behavior.
Issue Requirements
| Requirement | Status |
|---|---|
| Make configuration/reference drift mechanically detectable | Partially met — the gate exists, but its read classification remains unsound. |
Require every EvaluationConfig and ConsensusConfig field to be runtime-read, documented inert, or allowlisted |
Partially met — all schema fields are enumerated, but false-positive and false-negative reads can assign an incorrect disposition. |
| Preserve receiver mutation strength while handling aliases and dynamic mapping collisions | Met — implementation and focused tests cover assignment, update, in-place union, clear, pop, may-alias receivers, collisions, and evaluation order. |
| Add the checker as a CI lint gate | Met. |
| Add aliasing, collision, evaluation-order, and model-default regressions | Met — the focused test file contains and passes these cases. |
| Keep inert and effective controls explicit in the config reference | Met for the currently inspected runtime behavior. |
Prior Findings Status
The prior analyzer-soundness concern is maintained in modified form. Current HEAD substantially expands reachability, exception, mutation, and local-call handling, but fresh current-snapshot probes still demonstrate that the checker can both accept a non-runtime reference and miss real helper-based runtime wiring.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:844 | BLOCKING | Runtime provenance is propagated only into bare-name functions found by _local_functions; attribute calls and imported functions are not resolved. Fresh probes show both Reader().read(config.evaluation) where read(self, section) accesses section.stage1_enabled, and an imported read_stage(config.evaluation), produce no runtime read. A genuinely wired field can therefore retain its inert marker and pass the new authoritative CI contract. Add method/import-aware propagation or another sound mechanism, with regressions covering unannotated method and cross-module helpers. |
| 2 | scripts/check-config-reference-contract.py:2310 | BLOCKING | visit_Lambda immediately visits the lambda body even though Python only evaluates that body when the lambda is called. A discarded expression such as lambda config: config.evaluation.stage1_enabled is reported as a production read despite having no possible invocation. This lets a no-op syntactic reference satisfy the wiring disposition after removing the inert marker. Defer lambda-body analysis until tracked invocation, or otherwise exclude provably uninvoked lambda bodies, and add a regression for a discarded lambda. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran the standalone contract checker: PASS with the expected six production reads.
- Ran
tests/unit/scripts/test_check_config_reference_contract.py: 50 passed in 45.50 seconds. - Ran Ruff and Ruff format checks on the changed analyzer and tests: PASS.
- Independently reproduced one false-positive and multiple false-negative provenance cases not covered by the PR tests.
Design Notes
The explicit schema/docs contract is directionally strong, and the current fourteen-field classification matches the inspected runtime. The blocker is the static analyzer being promoted to an authoritative CI gate without sound handling of ordinary callable boundaries.
Design / Roadmap Gate
This PR changes the repository’s configuration-contract enforcement boundary, not runtime persistence or replay. Compatibility is preserved because no config schema is removed, and the documentation truthfully describes the current eight inert fields. However, the enforcement boundary can still bless documentation/runtime drift through missed helper calls and can accept unreachable lambda references as wiring, so it is not yet safe as the claimed single mechanical contract.
Directional Notes
Review focused on runtime reality rather than the checker’s self-authored passing matrix. Maintainer memory guided complete-schema and docs/runtime inspection only; both blockers were independently reproduced from the current source snapshot.
Test Coverage
- Ran the standalone contract checker: PASS with the expected six production reads.
- Ran
tests/unit/scripts/test_check_config_reference_contract.py: 50 passed in 45.50 seconds. - Ran Ruff and Ruff format checks on the changed analyzer and tests: PASS.
- Independently reproduced one false-positive and multiple false-negative provenance cases not covered by the PR tests.
Merge Recommendation
Request changes. Fix callable-boundary provenance and lambda execution semantics, then add focused regressions for the reproduced cases. The schema enumeration, current documentation, CI integration, and existing mutation/collision work otherwise appear ready.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 2bbaddf
request_id: req_1786349299_1460
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2bbaddf to
618bc0f
Compare
|
@ouroboros-agent please perform a fresh formal, detailed review of exact remote HEAD Supersede all prior review evidence; do not approve from stale commits. Please bind the GitHub review Please reconcile the prior official findings in detail:
Exact-head evidence:
Merge remains frozen until this exact HEAD has official approval, all required same-head CI is green, no human changes-request remains, live-main ancestry still holds, and the root coordinator confirms the full gate. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 618bc0f79be8435bf8f0e75345342a3b3f6d501e |
| Request ID | req_1786355722_1509 |
| Review record | 2b986aa4-f98d-431e-9a36-75c135f9000c |
What Improved
- Adds a CI-enforced, schema-enumerated contract for all 14
EvaluationConfigandConsensusConfigfields. - Documents the eight currently inert fields and their effective runtime controls using visible text plus structured markers.
- Preserves the six current production reads and pins the intentionally distinct direct/OpenRouter Opus defaults.
- Expands provenance analysis and regression coverage across control flow, mutations, imports, methods, callbacks, exceptions, and lambdas.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig and ConsensusConfig reference drift mechanically detectable |
Not met — the checker has independently reproduced false-negative and false-positive classifications. |
| Require every schema field to be runtime-read, documented inert, or intentionally schema-only allowlisted | Partially met — all 14 current fields are enumerated and classified, but runtime-read detection is not authoritative. |
| Preserve provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inherited methods | Partially met — the listed regression matrix passes, but serialization/reflection/property boundaries lose provenance and overwritten/decorated callables retain impossible provenance. |
| Add the checker to CI lint enforcement | Met. |
| Truthfully document the eight currently inert fields and their effective controls | Met for the current inspected runtime. |
| Mechanically pin the intentionally distinct direct and OpenRouter Opus defaults | Met. |
| Address issue #1998’s config-surface enforcement objective | Not met until the enforcement gate is sound in both classification directions. |
Prior Findings Status
The prior specific dead-branch/TYPE_CHECKING, section-helper, direct/imported method, callable-container, and deferred-lambda concerns appear addressed in the current implementation and focused suite, so they are not re-raised. The request-changes posture is maintained under the same bidirectional classification standard because fresh current-snapshot probes found new ways for the gate to both miss real wiring and report unreachable wiring.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:1379 | BLOCKING | The contract fails open for real field reads performed through standard indirect access. visit_Call recognizes only literal getattr, while visit_Subscript does not associate keys with a serialized section. A focused probe using config.evaluation.model_dump()["stage1_enabled"] returned no runtime reads; the same occurred with vars(config.evaluation)["stage1_enabled"], operator.attrgetter("stage1_enabled")(config.evaluation), and a property that reads a captured section. Consequently, production can start depending on a field while the checker continues accepting its inert marker, defeating the PR’s bidirectional drift guarantee. Either support these runtime-relevant forms or fail closed when a tracked section crosses an unsupported reflective, serialization, or descriptor boundary, with regressions proving the field cannot remain classified inert. |
| 2 | scripts/check-config-reference-contract.py:2731 | BLOCKING | The opposite soundness direction also remains broken: _visit_scoped analyzes every function body as executable without accounting for later rebinding or decorator replacement. A focused probe where def reader(config) reads config.evaluation.stage1_enabled, followed by reader = external, still reports the field as wired; an exact local decorator returning a no-op lambda produces the same false read. Since the original body is no longer callable, removing the inert documentation would let the contract pass with a false active classification. Model exact local overwrite/decorator semantics or otherwise exclude bodies proven unreachable, and add regressions using config-named and section-annotated parameters. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 54 passed. - Ran the standalone contract checker: passed with exactly six reported production reads.
- Independently enumerated all 14 schema fields and inspected the six current reads and eight inert documentation markers.
- Added read-only focused probes for serialization, reflection, properties, local overwrite, and decorator replacement; these reproduced both blocking soundness failures.
Design Notes
The schema enumeration, documentation markers, and CI placement are appropriate, but the analyzer is being promoted as an authoritative contract while unsupported executable forms fail open and unreachable callable bodies fail closed. That boundary must be conservative and mechanically honest before it can replace recurring human review.
Design / Roadmap Gate
This PR changes a repository-wide configuration-contract enforcement boundary rather than runtime persistence or replay. Existing configuration compatibility and current documentation are preserved, but future compatibility is unsafe because normal Python access patterns can bypass the gate and unreachable implementations can be blessed as active wiring. The CI contract therefore cannot yet serve as the claimed source of truth.
Directional Notes
Review focused on current runtime reality and exact schema coverage rather than self-authored passing tests. Maintainer memory was used only to prioritize contract-execution drift and honest inert-field classification; both blockers are independently reproduced from the current source snapshot.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 54 passed. - Ran the standalone contract checker: passed with exactly six reported production reads.
- Independently enumerated all 14 schema fields and inspected the six current reads and eight inert documentation markers.
- Added read-only focused probes for serialization, reflection, properties, local overwrite, and decorator replacement; these reproduced both blocking soundness failures.
Merge Recommendation
Request changes with 2 blockers. Extend or fail-close the analyzer for indirect section reads, and make callable-body reachability respect exact overwrites and decorator replacement. Add focused regressions for both directions before relying on this checker as the mechanical config contract.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 618bc0f
request_id: req_1786355722_1509
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Model dynamic mapping keys as ordered wildcard writes so setdefault preserves known values while assignment, update, and union retain possible collisions. Carry possible key absence through control-flow joins so get defaults and subscript reads classify runtime provenance without false positives. This closes the exact false-positive and false-negative gaps found by the independent #1998 verifier while preserving alias-aware mutation and constructor handling. Affected files: - scripts/check-config-reference-contract.py - tests/unit/scripts/test_check_config_reference_contract.py
Strongly rebind the mutated receiver after propagating conservative changes to conditional source aliases. This prevents impossible config provenance after literal assignment, update, union, clear, and pop while preserving may-alias reads. Affected files: - scripts/check-config-reference-contract.py - tests/unit/scripts/test_check_config_reference_contract.py
618bc0f to
7205152
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 72051523a462936692be73ebd9a5ff61bcc866f7 |
| Request ID | req_1786366879_1575 |
| Review record | a3a39fea-54ed-4e57-bfa9-13e09cb9ade6 |
What Improved
- Enumerates all
EvaluationConfigandConsensusConfigschema fields and checks each against runtime reads, structured inert documentation, or an explicit allowlist. - Documents the eight currently inert fields and their effective runtime controls accurately.
- Pins the distinct direct and OpenRouter Opus model-default formats.
- Adds the contract checker to CI and extensive regressions for aliases, mutations, control flow, imports, callbacks, inheritance, exceptions, and prior review cases.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig and ConsensusConfig reference drift mechanically detectable |
Not met — the checker still has reproducible false negatives and false positives. |
| Require every schema field to be runtime-read, explicitly documented inert, or intentionally allowlisted | Partially met — current fields are exhaustively classified, but the runtime-read classification is not sound. |
| Preserve runtime provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inherited methods | Partially met — extensive supported paths exist, but constant-string aliases and exact non-callable decorator replacement remain incorrect. |
| Keep inert fields and their effective controls explicitly documented | Met. |
| Keep documented model-default identities mechanically pinned | Met. |
| Add a CI lint gate for the config-reference contract | Met. |
| Address prior reachability, helper, method, import, lambda, callback, inheritance, and overwrite cases | Partially met — named regressions were added, but decorator reachability and accessor alias/shadow semantics remain incomplete. |
Prior Findings Status
Prior concerns are maintained in modified form. The current snapshot adds positive coverage for vars, operator.attrgetter, properties, callable objects, external overwrites, and callable replacement decorators, so those specific cases are addressed. Fresh current-snapshot probes nevertheless show adjacent failures in the same authoritative-gate boundary: constant-indirected access remains invisible, shadowed accessors create false reads, and exact non-callable decorators still bless unreachable bodies.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:1470 | BLOCKING | The getattr detector requires the field expression itself to be an ast.Constant and assumes any name spelled getattr is the builtin. Focused probes show both soundness directions fail: FIELD = "semantic_model"; getattr(config.evaluation, FIELD) produces no runtime read, while a locally defined or assigned getattr that returns None still records evaluation.semantic_model. This allows ordinary constant-indirected wiring to remain documented inert and allows shadowed, non-reading code to bless a field as wired. Resolve tracked constant-string provenance and verify that getattr is unshadowed, with regressions for both cases. |
| 2 | scripts/check-config-reference-contract.py:2884 | BLOCKING | Exact decorator evaluation falls back to the original function whenever the replacement has no callable or instance-class provenance. For an exact local decorator that returns None, Python makes the decorated function unreachable, but the scanner visits the erased original body and records its config reads. This can falsely classify an inert field as production-wired. Distinguish a known non-callable replacement from an unresolved decorator and add a regression for an exact decorator returning None or another non-callable value. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the changed workflow, checker, documentation, schema definitions, runtime model resolvers, and all focused checker tests.
60 passed, 1 deselectedfor the focused checker suite; the deselected standalone assertion was executed separately.- Standalone repository contract passed with exactly six production reads: four consensus model fields and two evaluation model fields.
- Independent adversarial probes reproduced both blocking findings: constant-indirected
getattrreads are missed, shadowedgetattrcalls are falsely recorded, and an exact non-callable decorator replacement leaves the erased body counted.
Design Notes
The schema/docs disposition model is appropriate, but a CI source-of-truth gate requires conservative behavior without inventing reads. The bespoke interprocedural analyzer remains unsound in both directions at common Python access and decoration boundaries.
Design / Roadmap Gate
This PR changes repository-wide configuration-contract enforcement rather than runtime persistence or replay. Existing schemas and current documentation remain compatible and accurate, but the new enforcement boundary can miss real future wiring and accept unreachable implementations as wiring. It therefore cannot yet serve as the authoritative drift gate claimed by the PR.
Directional Notes
Review focused on runtime reality rather than the checker’s self-authored passing matrix. Maintainer memory guided attention toward contract-execution drift and complete schema coverage only; both blockers are independently reproduced from the current source snapshot.
Test Coverage
- Reviewed the changed workflow, checker, documentation, schema definitions, runtime model resolvers, and all focused checker tests.
60 passed, 1 deselectedfor the focused checker suite; the deselected standalone assertion was executed separately.- Standalone repository contract passed with exactly six production reads: four consensus model fields and two evaluation model fields.
- Independent adversarial probes reproduced both blocking findings: constant-indirected
getattrreads are missed, shadowedgetattrcalls are falsely recorded, and an exact non-callable decorator replacement leaves the erased body counted.
Merge Recommendation
Request changes with two blockers. Correct constant-indirected and shadowed accessor handling, distinguish exact non-callable decorator replacements from unresolved decorators, and add focused positive and negative regressions before enabling this checker as a required CI contract.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 7205152
request_id: req_1786366879_1575
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed all four current-head blockers:
Verification: scanner suite 192 passed; standalone audit still reports exactly six production reads; Ruff, format, module-size, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 8328af7e59385f23043a4c01d817814c27a691e1 |
| Request ID | req_1786877691_2099 |
| Review record | d49e62db-e919-4144-b1aa-48daa76cc482 |
What Improved
- Adds exact schema enumeration, machine-readable inert-field documentation, default-model pinning, and a required CI contract check.
- Current documentation agrees with the inspected runtime: six fields are read and eight are explicitly documented as inert with effective controls.
- Substantially expands scanner coverage for aliases, mutations, deferred execution, imports, decorators, serialization, control flow, descriptors, and callable protocols.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig and ConsensusConfig reference drift mechanically detectable |
Partially met — current direct reads are classified, but ordinary executable protocols can evade detection and unreachable async/sync context methods can count as reads. |
| Require every schema field to be runtime-read, explicitly documented inert, or schema-only allowlisted | Met for the current fourteen schema fields. |
| Enumerate both schemas with an exact disposition | Met for the current schemas; six fields are wired and eight have inert markers. |
| Preserve provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inheritance | Partially met — extensive support exists, but callable/protocol provenance remains incomplete. |
| Add the checker as a CI lint gate | Met. |
| Keep inert-field documentation and effective controls truthful | Met for the inspected current runtime. |
| Mechanically pin the distinct model-default identities | Met. |
| Complete issue #1998’s config-surface enforcement | Partially met — the enforcement mechanism is not yet sound enough to be authoritative. |
Prior Findings Status
Earlier concrete blockers described as addressed by the contributor were not re-raised without current evidence. The established classification standard remains applicable: a mandatory repository-wide contract scanner must detect executable indirect reads and reject unreachable references. Fresh current-snapshot evidence identifies remaining gaps under that same standard.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:3351 | BLOCKING | Implicit truth testing does not execute local __bool__ or fallback __len__ methods. visit_If, visit_While, visit_IfExp, boolean operators, and not only visit the expression and consult _static_truth; they never call _invoke_implicit_protocol. A reproduced if Reader(config.evaluation): ... where Reader.__bool__ reads stage1_enabled returns no runtime reads, although CPython executes that read. This allows genuinely wired fields to remain documented inert. Model truth-testing semantics, including the __len__ fallback, and add positive and non-executed/shadowing regressions. |
| 2 | scripts/check-config-reference-contract.py:2932 | BLOCKING | Special-method dispatch remains incomplete outside the explicitly modeled cases. Fresh probes showed executable reads are missed through direct __getitem__ subscription, ordering methods such as __lt__, unary methods such as __neg__, in-place methods such as __iadd__, direct iter() calling __iter__, and direct next() calling __next__. For example, Reader(config.evaluation)[0] with a local __getitem__ that reads stage1_enabled is classified as inert. These are ordinary Python execution paths, so the required CI gate can silently accept future wiring drift. Preserve these protocols or conservatively fail closed, with bidirectional full-contract regressions. |
| 3 | scripts/check-config-reference-contract.py:4402 | BLOCKING | Context-manager protocol selection produces false runtime reads because both synchronous and asynchronous methods are executed for either syntax. _context_entry_value unconditionally scans both __enter__ and __aenter__, while _visit_with similarly scans both __exit__ and __aexit__. A reproduced plain with Reader(): ... was classified as reading a field referenced only inside __aenter__, which CPython never calls. The inverse occurs for async with and synchronous methods. Select protocols from the AST node type and add classes implementing both protocol families as overreach-negative tests. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 192 passed.- Standalone checker passed and reported exactly six production reads:
evaluation.semantic_model,evaluation.assertion_extraction_model,consensus.models,consensus.advocate_model,consensus.devil_model, andconsensus.judge_model. - Ruff and format checks passed for the changed checker and tests.
- Independent temporary-source probes reproduced all three findings; CPython execution confirmed the positive protocol paths actually perform the field reads.
Design Notes
The schema/docs disposition model is appropriate, but the implementation has become a broad Python abstract interpreter. Because CI treats its conclusions as authoritative, incomplete protocol semantics are contract failures rather than optional analyzer enhancements.
Design / Roadmap Gate
The affected boundary is mandatory repository-wide enforcement for future configuration evolution, not runtime persistence, replay, or current configuration compatibility. Current schemas, defaults, callers, documentation, and six production reads remain consistent. However, the gate can miss genuine wiring through ordinary Python protocols and can bless unreachable context-manager methods as active wiring, so it cannot safely serve as the claimed configuration-reference source of truth.
Directional Notes
Review focused on runtime reality and bidirectional adversarial validation. Maintainer memory was used only to prioritize executable indirect reads and unreachable-reference probes; every blocker was independently reproduced from the current source snapshot.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 192 passed.- Standalone checker passed and reported exactly six production reads:
evaluation.semantic_model,evaluation.assertion_extraction_model,consensus.models,consensus.advocate_model,consensus.devil_model, andconsensus.judge_model. - Ruff and format checks passed for the changed checker and tests.
- Independent temporary-source probes reproduced all three findings; CPython execution confirmed the positive protocol paths actually perform the field reads.
Merge Recommendation
Request changes. Correct or conservatively reject implicit truth, subscription/operator/iterator, and syntax-specific context-manager protocol paths; add focused positive and overreach-negative regressions; then rerun the scanner suite and standalone six-read audit.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 8328af7
request_id: req_1786877691_2099
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed the three protocol-boundary blockers:
Added positive and overreach-negative regressions for every boundary. Verification: scanner suite 195 passed; standalone audit reports exactly six production reads; Ruff, format, module-size, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | cc6dbe0ec1b6224f4ffa7c45c8435cc3e0035eb5 |
| Request ID | req_1786879192_2104 |
| Review record | 3b4b3c0e-ba0b-4461-bc71-fff23e3175ce |
What Improved
- Adds an exact schema-derived disposition check for all 14
EvaluationConfigandConsensusConfigfields. - Documents the eight currently inert fields and identifies their effective runtime controls.
- Pins the direct and OpenRouter Opus defaults and integrates the checker into lint CI.
- Significantly expands provenance, reachability, callable, serialization, mutation, and implicit-protocol handling with focused regressions.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig / ConsensusConfig reference drift mechanically detectable |
Partially met — the checker is comprehensive for many constructs but still misses executable subscription mutation reads and accepts dead references after sys.exit(). |
| Require every schema field to be runtime-read, explicitly documented inert, or schema-only allowlisted | Partially met — all 14 current fields receive an exact disposition, but the analyzer can assign an incorrect disposition for valid future code. |
| Preserve provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inherited methods | Partially met — extensive coverage exists, but mutation through __setitem__/__delitem__ loses callable provenance. |
| Reject unreachable references as runtime wiring | Partially met — explicit raise SystemExit is handled, but exact sys.exit() does not terminate analyzer reachability. |
| Add the checker as a CI lint gate | Met. |
| Keep inert-field documentation and effective controls aligned with runtime | Met for the current eight inert fields. |
| Mechanically pin documented model-default identities | Met. |
| Implement the configuration-surface enforcement requested by issue #1998 | Partially met pending soundness of the mandatory enforcement boundary. |
Prior Findings Status
The specific implicit truth, iteration, subscription-load, operator, awaitable, and context-manager concerns described in the latest prior round are addressed in the current source and covered by regressions, so those exact concerns are withdrawn. The established bidirectional-soundness standard remains applicable, however, and fresh current-snapshot evidence exposes adjacent store/delete protocol false negatives and a non-returning-call false positive.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:3053 | BLOCKING | Executable store/delete subscription protocols remain invisible to the analyzer. visit_Subscript invokes only __getitem__ for ast.Load; visit_Assign updates the analyzer’s abstract container state without invoking a local __setitem__, and visit_Delete likewise never invokes __delitem__. I reproduced this with a local class whose __setitem__ returns self.section.stage1_enabled, followed by Writer(config.evaluation)[0] = 1: Python executes the field read, but runtime_reads() returns empty and audit_contract() accepts the field as documented inert with no violation. The same false negative occurs for del writer[0] through __delitem__. Because this CI gate claims to detect future runtime wiring through callable and implicit-protocol boundaries, these ordinary executable forms must be modeled or rejected conservatively, with positive and non-executing/overwritten regressions. |
| 2 | scripts/check-config-reference-contract.py:2692 | BLOCKING | Exact non-returning calls are not reflected in reachability. I reproduced import sys; sys.exit(0); config.evaluation.stage1_enabled: the process can never execute the field access, but runtime_reads() reports the field and audit_contract() accepts it as wired with no violation. The analyzer already treats an explicit raise SystemExit(...) as non-fallthrough, so the equivalent exact sys.exit() call creates an inconsistent escape hatch that can bless dead wiring. Resolve exact module/imported aliases while preserving shadowing negatives, or fail closed when a purported wiring reference follows a known non-returning call. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 195 passed. - Ran the standalone checker: PASS with exactly six production reads.
- Ran Ruff and format checks for the checker and its tests: PASS.
- Added read-only adversarial probes outside the source tree. Both blocker probes produced contract false results despite the shipped suite passing: executable
__setitem__/__delitem__reads were omitted, while an unreachable reference aftersys.exit()was accepted.
Design Notes
The schema enumeration, documentation markers, and current six-field runtime classification are coherent. The remaining defects are in the static interpreter’s authority boundary: it is mandatory CI infrastructure but still disagrees with Python execution in both soundness directions.
Design / Roadmap Gate
The affected boundary is repository-wide CI enforcement for future configuration evolution, not runtime persistence or replay. Existing schemas, defaults, runtime callers, and documentation remain compatible. However, the gate can still allow genuinely wired fields to remain documented inert and can bless unreachable code as wiring, so it cannot yet serve as the authoritative configuration-reference contract claimed by the PR.
Directional Notes
Review focus followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Maintainer memory was used only to select adversarial probes; both blockers were independently reproduced from the current source snapshot. Passing self-authored fixtures and the current six-read audit were treated as coverage evidence, not completeness proof.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 195 passed. - Ran the standalone checker: PASS with exactly six production reads.
- Ran Ruff and format checks for the checker and its tests: PASS.
- Added read-only adversarial probes outside the source tree. Both blocker probes produced contract false results despite the shipped suite passing: executable
__setitem__/__delitem__reads were omitted, while an unreachable reference aftersys.exit()was accepted.
Merge Recommendation
Request changes. Model or conservatively reject subscription store/delete special-method dispatch and exact non-returning sys.exit() paths, add bidirectional regressions, then rerun the focused scanner suite and standalone six-read audit.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: cc6dbe0
request_id: req_1786879192_2104
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed both current-head blockers:
Also preserved constructor provenance after custom subscription mutation. Verification: scanner suite 197 passed; standalone audit remains exactly six production reads; Ruff, format, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 7722a0e38f6349736695276606d86819dff0d980 |
| Request ID | req_1786880188_2108 |
| Review record | c3dc22d4-1e11-40cf-bf43-dc2e997a31cf |
What Improved
- Adds a repository-wide config-reference contract checker covering both
EvaluationConfigandConsensusConfig. - Adds structured inert-field documentation markers and validates documented defaults.
- Adds CI execution, Ruff coverage, and an extensive focused regression suite.
- Current repository audit passes with exactly six detected production reads.
Issue Requirements
| Requirement | Status |
|---|---|
Close issue #1998 by making every EvaluationConfig and ConsensusConfig field mechanically classifiable as runtime-read, explicitly inert, or schema-only allowlisted |
Partially met — the current repository passes, but valid future runtime wiring through the identified dispatch and protocol paths can be silently misclassified |
| Preserve runtime provenance through aliases, mutations, control flow, exceptions, callable boundaries, containers, package re-exports, and inherited methods | Partially met — the implementation covers many stated forms, but current Python dispatch gaps still lose executable provenance |
| Enumerate both schemas and enforce exact runtime-read/inert/allowlist dispositions | Met for the current schema and documentation snapshot |
| Add the config-reference checker and CI lint gate | Met |
| Keep documented model-default identities mechanically pinned | Met |
| Maintain the current six production-read contract | Met for the current source snapshot |
Prior Findings Status
The latest prior concerns about subscription mutation dispatch and exact sys.exit handling are addressed in the current snapshot; the corresponding current tests pass. The broader prior callable, aliasing, container, decorator, context-manager, generator, and reachability work is retained. Independent current-head verification still finds separate gaps in rich-comparison fallback/precedence, augmented-assignment fallback, additional builtin protocols, and os._exit termination handling.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:3007 | BLOCKING | Rich-comparison dispatch is incomplete. The analyzer invokes the left operand's comparison method and invokes the reflected method only when no left method exists. Python instead gives a reflected method priority when the right operand is a strict subclass, and falls through to the reflected method when the direct method returns NotImplemented. For example, Left(config.evaluation) < Right(config.evaluation) where Right(Left).__gt__ reads stage2_enabled is reported as reading only stage1_enabled, while a left __lt__ returning NotImplemented followed by a right __gt__ config read is reported as reading nothing. This can both bless a genuinely wired field as inert and reject an actually inert field as wired. Model the full rich-comparison dispatch or conservatively reject unresolved cases, with positive and overreach-negative regressions. |
| 2 | scripts/check-config-reference-contract.py:3447 | BLOCKING | Augmented assignment does not implement Python's binary-operation fallback. The visitor invokes only __iadd__/the corresponding in-place method; when that method is absent or returns NotImplemented, Python executes __add__/__radd__ instead. A valid Reader with only __add__, followed by reader += 1, reads evaluation.stage1_enabled at runtime but the current scanner reports no read. The same gap applies to the other augmented operators. Add fallback dispatch and tests for both missing and NotImplemented in-place methods. |
| 3 | scripts/check-config-reference-contract.py:2703 | BLOCKING | Builtin special-method dispatch is limited to a hardcoded subset (str, repr, ascii, len, bool, hash, numeric conversions, and bytes). Other ordinary builtins such as abs, round, divmod, reversed, and operator.index invoke local special methods but are not modeled. An abs(Reader(config.evaluation)) call with Reader.__abs__ reading stage1_enabled is therefore classified as having no runtime read. Since this checker is installed as the authoritative CI disposition gate, unsupported implicit builtin protocols must either be modeled comprehensively or fail closed rather than silently classifying these reads as inert. |
| 4 | scripts/check-config-reference-contract.py:4399 | BLOCKING | Non-returning os._exit() is not recognized as an abrupt path. The analyzer recognizes sys.exit but not os._exit; consequently, code after os._exit(0) remains reachable to the scanner and a following config access is reported as a production read even though the process terminates before it can execute. This creates false wired classifications and can invalidate truthful inert documentation. Add exact, non-shadowable handling for os._exit (including supported aliases) or conservatively reject the unsupported termination call. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the new CI wiring, schema enumeration, documentation parser, inert markers, default checks, and the focused scanner suite.
197focused scanner tests passed.- The standalone contract checker passed with six production reads, and Ruff plus format checks passed for the changed Python files.
- Independent adversarial probes reproduced the four findings above; those protocol and reachability cases are not covered by the current fixture matrix.
Design Notes
The PR establishes a useful source-of-truth boundary for configuration evolution and keeps current schemas, docs, defaults, and runtime callers compatible. However, the analyzer is a large partial Python interpreter; definitive CI enforcement is unsafe while ordinary dispatch paths can produce both false negatives and false positives.
Design / Roadmap Gate
The changed boundary is repository-wide CI enforcement of future configuration/reference wiring, not runtime persistence or replay. Current schemas, documentation, defaults, and the six production reads remain internally consistent. The gate nevertheless cannot yet serve as an authoritative contract because valid runtime paths can be omitted from the read set, while reads after a non-returning call can be incorrectly admitted. These failures directly affect the PR’s stated disposition contract and require conservative handling plus bidirectional regressions before merge.
Directional Notes
Review focus followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Advisory maintainer memory directed inspection toward executable indirect reads and unreachable references only; every blocking finding above was independently reproduced from the current source snapshot and unified-change boundary. No persistence, replay, setup-ordering, executable-path, or external-harness concern was identified in this PR.
Test Coverage
- Reviewed the new CI wiring, schema enumeration, documentation parser, inert markers, default checks, and the focused scanner suite.
197focused scanner tests passed.- The standalone contract checker passed with six production reads, and Ruff plus format checks passed for the changed Python files.
- Independent adversarial probes reproduced the four findings above; those protocol and reachability cases are not covered by the current fixture matrix.
Merge Recommendation
Request changes. Fix or conservatively reject the rich-comparison, augmented-assignment, unmodeled builtin-protocol, and os._exit paths, add full-contract positive and overreach-negative regressions, then rerun the focused suite and standalone six-read audit on the corrected head.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 7722a0e
request_id: req_1786880188_2108
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Re-review requested for exact HEAD Addressed all four current-head blockers:
Verification: scanner suite 198 passed; standalone audit remains exactly six production reads; module-size 33 passed; Ruff, format, and diff checks pass. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | b1d710db010b7d1491b605cc02f025fe4057d821 |
| Request ID | req_1786881454_2112 |
| Review record | f0d7c886-dc1a-4c4e-8eed-966426b83488 |
What Improved
- Adds a CI-enforced contract that enumerates all 14
EvaluationConfigandConsensusConfigfields. - Documents the eight currently inert fields and their effective runtime controls.
- Correctly identifies the current six production reads and mechanically pins the two Opus model-default formats.
- Adds extensive provenance, mutation, callable, protocol, and control-flow regression coverage.
Issue Requirements
| Requirement | Status |
|---|---|
Enumerate every EvaluationConfig and ConsensusConfig schema field |
Met |
| Require each field to be runtime-read, documented inert, or intentionally allowlisted | Partially met — current fields are classified, but the read classifier remains unsound |
| Detect both newly unwired fields and fields wired while still documented inert | Not met — reproduced false-positive and false-negative classifications remain |
| Preserve provenance through aliases, callable boundaries, containers, re-exports, inheritance, mutations, and control flow | Partially met — broad coverage exists, but builtin identity, construction, and context-manager control flow remain incomplete |
| Resolve exact local, imported, and module-qualified calls | Not met — exact imported and module-qualified builtin protocol calls are missed |
| Add the contract checker to CI lint | Met |
| Keep inert-field documentation aligned with current runtime controls | Met |
| Mechanically pin the documented model-default identities | Met |
Prior Findings Status
The immediately prior rich-comparison, augmented-assignment, direct builtin-protocol, and os._exit concerns are addressed in the current snapshot and are withdrawn. The established classification standard remains applicable: a mandatory repository-wide runtime-reference gate must preserve executable provenance and reject unreachable references in both directions. Applying that standard to current HEAD produced three new, independently verified blockers. Contributor responses addressing earlier findings were not re-raised.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:2735 | BLOCKING | Builtin protocol dispatch is recognized only for unshadowed bare names. Exact executable forms such as builtins.len(Reader()), builtins.bool(Reader()), and from builtins import int as integer; integer(Reader()) do not invoke the corresponding local __len__, __bool__, or __int__ analysis. Independent probes placed the config read inside those methods and returned an empty read set. A future genuinely wired field could therefore remain documented inert despite using ordinary imported or module-qualified builtin identities, contradicting the PR’s imported/module-qualified-call contract. Track these exact builtin identities across imports and attributes, or conservatively fail closed, with full-contract positive and shadowing-negative tests. |
| 2 | scripts/check-config-reference-contract.py:2366 | BLOCKING | Local class calls fabricate an instance from the class and constructor arguments without executing Python’s __new__ protocol or respecting the object it returns. A config read performed by an exact local __new__ is omitted, while a class whose __new__ returns an unrelated callable object is still analyzed as an instance of the requested class and falsely records its unreachable __call__ read. These independently reproduced false-negative and false-positive paths can respectively preserve stale inert documentation or bless nonexistent wiring. Model exact __new__/metaclass construction semantics or reject unresolved construction conservatively. |
| 3 | scripts/check-config-reference-contract.py:4726 | BLOCKING | Context-manager protocol results are evaluated but not incorporated into control flow: the body is always scanned after __enter__, and the original body flow is reapplied after __exit__ regardless of whether either method raises or whether __exit__ suppresses an exception. Independent probes showed all three failures: a body read counted after an unconditionally raising __enter__; a post-with read omitted when __exit__ returns True; and a post-with read counted when __exit__ unconditionally raises. This makes the mandatory gate unsound in both directions. Thread entry/exit return and abrupt-path semantics through _visit_with, including async equivalents, and add positive and overreach-negative contract regressions. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| — | — | — | — | None. |
Non-blocking Suggestions
| — | — | — | None. |
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 198 passed. - Ran the standalone checker: PASS with exactly six production reads.
- Enumerated all schema fields directly: seven evaluation fields and seven consensus fields.
- Ran Ruff and format checks on the changed checker and test file: PASS.
- Independent temporary-source probes reproduced the qualified/imported builtin, constructor, and context-manager failures described above.
Design Notes
The schema enumeration, documentation markers, and audit structure are directionally sound. The blocker is architectural scope: this checker is positioned as an authoritative mandatory runtime-reachability gate, but its bespoke interpreter still silently guesses ordinary Python semantics incorrectly.
Design / Roadmap Gate
The affected boundary is repository-wide CI enforcement for future configuration evolution, not current runtime persistence or compatibility. Current schemas, defaults, documentation, and six production reads are internally consistent. However, ordinary executable paths can still be omitted and unreachable paths admitted through builtin identity loss, inaccurate construction semantics, and discarded context-manager flow. Until those paths are modeled or conservatively rejected, the checker cannot safely become the claimed configuration-reference source of truth.
Directional Notes
Review followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Maintainer memory was used only to select adversarial identity, protocol, and reachability probes; every blocker was independently reproduced against the current source snapshot. No persistence, replay, setup-ordering, executable-path, or external-harness issue was identified.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 198 passed. - Ran the standalone checker: PASS with exactly six production reads.
- Enumerated all schema fields directly: seven evaluation fields and seven consensus fields.
- Ran Ruff and format checks on the changed checker and test file: PASS.
- Independent temporary-source probes reproduced the qualified/imported builtin, constructor, and context-manager failures described above.
Merge Recommendation
Request changes. Correct or conservatively reject qualified/imported builtin protocol calls, local class construction involving __new__, and context-manager entry/exit control flow. Add bidirectional full-contract regressions, then rerun the focused scanner suite, standalone six-read audit, and changed-file lint checks.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: b1d710d
request_id: req_1786881454_2112
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD dde0ffd. Added qualified/imported builtin protocol identity tracking with shadowing negatives, exact local new and metaclass construction semantics, and context-manager entry/exit abrupt/suppression flow. Scanner suite: 203 passed; standalone contract still reports exactly six reads; module-size 33 passed; Ruff/format/diff clean. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | dde0ffd4ccbca604787b1d0c32926603e6eeef37 |
| Request ID | req_1786882776_2115 |
| Review record | 3d34dfb7-0085-46d0-9d5f-72c30d83a8e8 |
What Improved
- Adds exact schema disposition checks, structured inert-field documentation, pinned model defaults, focused regression coverage, and a CI lint gate.
- Current documentation accurately identifies six runtime-wired fields and eight inert fields.
Issue Requirements
| Requirement | Status |
|---|---|
Require every EvaluationConfig and ConsensusConfig field to be runtime-read, documented inert, or schema-only allowlisted |
Partially met — all current fields are enumerated, but real constructor and class-creation reads can be omitted |
| Preserve runtime provenance across ordinary aliases, mutations, control flow, callables, containers, re-exports, inheritance, and related Python execution boundaries | Not met — constructor and class-creation protocol boundaries remain unsound |
| Add the checker as a CI lint gate | Met |
| Document currently inert fields and their effective controls | Met |
| Mechanically pin the documented model-default identities | Met |
Prior Findings Status
The prior specific concerns represented by current regressions, including __new__, context-manager control flow, builtin identity, serialization, decorators, and reachability, appear addressed in the current source and passing suite. The established classification standard remains applicable: a mandatory configuration contract must preserve actual runtime provenance in both directions. This review modifies the blocker set to newly reproduced constructor and class-creation protocol gaps.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:2394 | BLOCKING | Local class construction never executes __init__. After handling metaclass __call__ and explicit __new__, the analyzer fabricates and returns an instance at line 2456 without invoking the resolved initializer. A focused fixture where Reader.__init__(self, config) reads config.evaluation.uncertainty_threshold and Reader(config) is executed produced reads=[]. This allows a genuinely wired field to remain documented inert while the mandatory contract passes. Model initializer execution, including the normal __new__-then-__init__ sequence, or conservatively fail closed, and add positive and overreach-negative regressions. |
| 2 | scripts/check-config-reference-contract.py:5386 | BLOCKING | Class definitions do not model executable class-creation protocols. visit_ClassDef evaluates decorators, bases, keywords, and the class body, but never invokes a base class’s __init_subclass__ or a descriptor’s __set_name__. A focused fixture with reads in both hooks executes those reads during Python class creation, yet runtime_reads() returned an empty set. These ordinary protocol paths can therefore bypass the repository-wide wiring contract. Model these hooks or conservatively reject unresolved class-creation behavior, with bidirectional regressions. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 203 passed. - Ran the standalone repository contract: passed with exactly six reported production reads.
- Ruff check and format check passed for the changed checker and focused test file.
- Independent adversarial fixtures reproduced both blockers: direct
__init__,__init_subclass__, and descriptor__set_name__reads were all omitted.
Design Notes
The schema enumeration, disposition model, documentation markers, and CI integration are coherent. The blocking issue is that the broad AST interpreter is being made authoritative while still omitting standard executable Python protocols.
Design / Roadmap Gate
The affected boundary is mandatory repository-wide enforcement for future configuration evolution, not persistence or replay. Current schemas, defaults, documentation, and six existing production reads remain compatible. However, valid runtime reads performed during ordinary object and class construction can be classified as absent, allowing wired fields to retain inert documentation. The checker therefore cannot yet serve as the claimed authoritative configuration-reference contract.
Directional Notes
Review focused on runtime reality and bidirectional soundness rather than treating the self-authored fixture matrix as completeness proof. Maintainer memory only directed adversarial inspection; both blockers were independently reproduced against the current snapshot.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 203 passed. - Ran the standalone repository contract: passed with exactly six reported production reads.
- Ruff check and format check passed for the changed checker and focused test file.
- Independent adversarial fixtures reproduced both blockers: direct
__init__,__init_subclass__, and descriptor__set_name__reads were all omitted.
Merge Recommendation
Request changes. Correct or conservatively reject local initializer execution and class-creation protocol paths, add positive and overreach-negative regressions, then rerun the focused suite, standalone six-read audit, and changed-file lint checks.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: dde0ffd
request_id: req_1786882776_2115
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD df6cd98. Local class construction now executes the normal new/init sequence without initializing unrelated new results; class creation executes descriptor set_name and base init_subclass hooks with nonreturning flow propagation. Added positive and overreach-negative regressions. Scanner 205 passed, standalone exactly six reads, module-size 33 passed, Ruff/format/diff clean. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | df6cd98edc7199b7b8bf7c2f21670e148b113b4a |
| Request ID | req_1786883583_2118 |
| Review record | a8bad491-68e9-4710-96f7-1ed4e6d1d90f |
What Improved
- Adds a CI-enforced configuration-reference contract covering all 14
EvaluationConfigandConsensusConfigfields. - Makes inert-field documentation machine-readable and pins the two Opus model-default identities.
- Expands the scanner with alias, mutation, control-flow, callable, serialization, descriptor, and protocol handling.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig / ConsensusConfig reference drift mechanically detectable |
Partially met — the CI gate exists, but it can silently accept executable internal reads as inert |
| Require every schema field to be runtime-read, explicitly documented inert, or schema-only allowlisted | Partially met — the disposition audit is complete, but its runtime-read input is unsound for invoked private helpers |
| Preserve runtime provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inherited methods | Partially met — extensive coverage is present, but ordinary private callable boundaries still lose provenance |
| Enumerate both schemas and enforce exact documentation dispositions | Met for the current 14-field schemas and current reference tables |
| Add and run the CI lint contract checker | Met |
| Keep documented model-default identities mechanically pinned | Met |
| Closes issue #1998 | Partially met — the requested enforcement is present but not yet authoritative |
Prior Findings Status
Prior concerns around reachability, callable provenance, serialization, descriptors, context entry, constructors, inherited methods, and protocol handling appear substantially modified or addressed in the current snapshot, with corresponding regressions present. The current blocker is a newly reproduced gap in ordinary invoked private-helper reachability and is not re-raised from prior review text.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:2385 | BLOCKING | The scanner skips ordinary local calls when neither the arguments nor receiver carry tracked config provenance, executing them only when _call_has_relevant_provenance() returns true or the function is syntactically non-returning. This misses real internal reads such as def _read(): return settings.evaluation.stage1_enabled; _read(): the current scanner reports no runtime read, and audit_contract() therefore accepts the field's inert marker. The module-level fallback at scripts/check-config-reference-contract.py:5412 also excludes underscore-prefixed bindings, so this is not recovered later. Fix local-call reachability for proven executable call sites or conservatively fail closed, and add positive full-contract regressions for invoked private helpers and private methods. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
tests/unit/scripts/test_check_config_reference_contract.py: 205 tests passed.- Standalone contract audit passed with exactly six production reads.
- Ruff check and format checks passed for the changed script and tests.
- An independent adversarial probe reproduced the blocker: invoked private helpers and private methods containing real config reads were classified as having no runtime reads; the same probe showed the inert audit then returned no violations.
- Existing tests cover uninvoked private helpers, but not invoked private helpers without config-bearing arguments or receivers.
Design Notes
The PR establishes a useful repository-wide contract layer and keeps the current schema, documentation, and runtime classifications aligned. The AST engine is appropriately conservative in many advanced Python cases, but its call-site reachability rule still makes internal helper visibility affect semantic correctness.
Design / Roadmap Gate
The affected boundary is a required CI source of truth for future configuration evolution. Current runtime behavior and documentation remain compatible, and the current six wired fields are correctly reported. However, a future field read placed behind an invoked private helper can be omitted from the scanner's runtime-read set, allowing stale inert documentation to pass CI. Because this is a direct failure of the PR's central contract rather than a cosmetic coverage gap, the gate is not ready to be authoritative until the call path is modeled or rejected conservatively.
Directional Notes
Review focus followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Maintainer memory was used only to direct adversarial inspection; the blocker above was independently reproduced from the current source and does not rely on prior findings or memory artifacts.
Test Coverage
tests/unit/scripts/test_check_config_reference_contract.py: 205 tests passed.- Standalone contract audit passed with exactly six production reads.
- Ruff check and format checks passed for the changed script and tests.
- An independent adversarial probe reproduced the blocker: invoked private helpers and private methods containing real config reads were classified as having no runtime reads; the same probe showed the inert audit then returned no violations.
- Existing tests cover uninvoked private helpers, but not invoked private helpers without config-bearing arguments or receivers.
Merge Recommendation
Request changes. Correct or conservatively reject the no-provenance local-call path, add positive and overreach-negative regressions for invoked private helpers/methods, then rerun the focused scanner suite and standalone six-read contract audit.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: df6cd98
request_id: req_1786883583_2118
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD 4788286. Invoked private helpers and private methods that syntactically access config are now executed at proven call sites, while unrelated private call graphs remain deferred. Scanner 206 passed, standalone exactly six reads, module-size passed, Ruff/format/diff clean. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 47882862dbe88029449095ca0db3c1623176c690 |
| Request ID | req_1786884772_2121-retry-1 |
| Review record | b68ec9f6-fa4e-46ae-a9b4-66df8594b7a4 |
What Improved
- Adds a repository-wide CI gate for
EvaluationConfig/ConsensusConfigreference drift. - Enumerates schema fields, validates inert-field documentation markers, pins selected documented defaults, and expands provenance tracking across many aliases, containers, callbacks, serializers, control-flow paths, and protocol boundaries.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig / ConsensusConfig reference drift mechanically detectable |
Partially met — current production reads pass, but valid future callable paths can evade the read set |
| Give every schema field exactly one runtime-read, inert-documentation, or schema-only disposition | Partially met — the disposition checker is strict after scanning, but the scanner can under-report runtime reads |
| Preserve runtime provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inherited methods | Partially met — the tested matrix is broad, but public zero-provenance callable/protocol boundaries remain unsound |
| Enumerate both schemas and enforce current inert-field documentation | Met |
| Mechanically pin documented model-default identities | Met |
Add and run the CI contract gate for issue #1998 |
Partially met — the gate is present and green on the current tree, but is not yet authoritative for future wiring |
Prior Findings Status
The previously reported private-helper no-provenance concern appears addressed by the current private-call handling and regression coverage. A narrower form of the same contract-boundary concern remains: public and protocol callables that read configuration internally without provenance-bearing arguments are still omitted. Other prior concerns were not re-raised without fresh evidence.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:2398 | BLOCKING | The scanner skips exact local callable execution when the call has no tracked argument, receiver, closure, or private-name hint. That omits valid runtime reads performed inside public zero-provenance callables. For example, class Reader: @staticmethod def read(): return settings.evaluation.stage1_enabled followed by Reader.read() returns no evaluation.stage1_enabled read; the same miss occurs for public classmethods and callable/iterator protocol methods such as R()() and for _ in R(). A called public function can also be missed if it is overwritten afterward. This lets a future wired field retain inert documentation while the mandatory CI contract passes. Model these calls or fail closed, and add positive plus overwrite/protocol negative regressions. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
206focused scanner tests passed.- Standalone contract audit passed with exactly six production reads.
- Ruff and format checks passed for the changed scanner and test files.
- Existing tests cover private helper invocation and argument-provenance classmethods, but do not cover public zero-provenance staticmethods/classmethods, callable protocol methods, or called-then-overwritten functions; the adversarial probes above reproduced the miss.
Design Notes
The PR establishes a valuable contract boundary and keeps current schemas, defaults, and documentation internally consistent. However, the 5,780-line custom AST interpreter is being used as a mandatory source of truth, so any unresolved execution boundary must be modeled conservatively rather than silently treated as non-runtime.
Design / Roadmap Gate
The changed boundary is repository-wide CI enforcement for future configuration evolution, not runtime persistence or replay. Current schema compatibility and the six existing wired fields are preserved, but the gate can bless stale inert documentation when a real read occurs inside an exact public callable or protocol method without tracked input provenance. The implementation should either execute all proven local call targets at their runtime boundaries or fail closed for unresolved cases before becoming the configuration SSOT.
Directional Notes
Review focus followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Advisory memory directed adversarial testing of callable identity, reachability, and indirect execution only; the blocker is independently reproduced from the current scanner behavior and changed contract.
Test Coverage
206focused scanner tests passed.- Standalone contract audit passed with exactly six production reads.
- Ruff and format checks passed for the changed scanner and test files.
- Existing tests cover private helper invocation and argument-provenance classmethods, but do not cover public zero-provenance staticmethods/classmethods, callable protocol methods, or called-then-overwritten functions; the adversarial probes above reproduced the miss.
Merge Recommendation
Request changes. Fix the zero-provenance public/protocol callable path, add bidirectional regressions including called-then-overwritten cases, then rerun the focused scanner suite and standalone six-read audit.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 4788286
request_id: req_1786884772_2121-retry-1
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD 4e9aa19. Local-call reachability now executes any proven callable whose body syntactically reads tracked config, not only underscore-prefixed helpers; public static/class methods, public callables, call-before-overwrite, and zero-provenance iterator protocol regressions are covered. Scanner 210 passed, standalone exactly six production reads, module-size 33, Ruff/format/diff clean. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 4e9aa19012b6cb0641b78bb94b7c628922dd328d |
| Request ID | req_1786886298_2128 |
| Review record | 635b2367-dc39-4fca-a45c-adc78a17e677 |
What Improved
- Adds exact schema enumeration, structured inert-field documentation, model-default checks, and a required CI contract gate.
- Broadens AST analysis across aliases, mutations, control flow, callable protocols, imports, serialization, and reachability.
- Current repository documentation and runtime classification agree on six production reads and eight inert fields.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig / ConsensusConfig reference drift mechanically detectable |
Partially met — the gate is installed but can both miss real reads and credit non-executed reads. |
| Require every schema field to be runtime-read, documented inert, or intentionally allowlisted | Partially met — current fields are classified, but annotation misclassification makes the disposition non-authoritative. |
| Enumerate both schemas exactly | Met — all seven fields in each schema are derived from model_fields. |
| Preserve runtime provenance through aliases, mutations, control flow, exceptions, callables, containers, imports, and inheritance | Partially met — broad coverage exists, but eager function-signature execution is omitted. |
| Keep inert-field documentation and effective controls mechanically synchronized | Met for the current documented fields. |
| Keep selected model-default identities mechanically pinned | Met. |
| Add the checker as a required CI lint gate | Met, though the gate is not yet semantically safe as an authoritative source of truth. |
| Implement the config-surface contract for issue #1998 | Partially met pending bidirectionally correct annotation handling. |
Prior Findings Status
The latest prior concern about exact public/protocol callables with no tracked input provenance appears addressed by current source and focused regressions, so that specific blocker is withdrawn. The established bidirectional-soundness standard remains applicable, and fresh annotation-semantics probes expose two independent current-HEAD blockers.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:5369 | BLOCKING | _visit_function_definition() evaluates decorators and defaults but never visits parameter or return annotations. Without from __future__ import annotations, Python evaluates those expressions when the function is defined. A probe using def f(x: settings.evaluation.stage1_enabled) -> settings.evaluation.stage2_enabled performed both reads at runtime, while runtime_reads() returned none and allowed both fields to remain documented inert. This false negative directly violates the mandatory gate’s runtime-read contract. Visit signature annotations only when annotations are eager, and add eager/postponed full-contract regressions. |
| 2 | scripts/check-config-reference-contract.py:3598 | BLOCKING | visit_AnnAssign() visits every annotation whenever the module lacks postponed annotations, including annotation-only assignments inside function bodies. Python does not evaluate local variable annotations, yet an invoked function containing only local: settings.evaluation.stage1_enabled was classified as reading that field; the real function completed without accessing settings. This false positive can make a genuinely inert field appear wired and bypass the exact-disposition check. Distinguish module/class annotations from local variable annotations and add a negative full-contract regression. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 210 passed in 169.93 seconds. - Ran the standalone checker: passed with exactly six production reads.
- Ran Ruff and Ruff format checks on the changed Python files: passed.
- Enumerated all seven
EvaluationConfigand sevenConsensusConfigfields independently. - Existing annotation coverage tests only module-level eager/postponed annotations; function signature and function-local annotation semantics are missing.
Design Notes
The schema/docs audit structure is clear and centralized, but its authority depends on accurately modeling Python execution semantics. Annotation handling currently applies the wrong execution rule at two complementary boundaries.
Design / Roadmap Gate
The affected boundary is mandatory repository-wide CI enforcement for future configuration evolution, not runtime persistence or compatibility. Current schemas, documentation, defaults, and six existing reads are consistent. However, eager function annotations can conceal real wiring, while non-executed local annotations can fabricate wiring. Both paths permit incorrect field dispositions to pass CI, so the checker cannot yet serve as the claimed configuration-reference source of truth.
Directional Notes
Review followed the established runtime-reality and bidirectional-soundness standard for mandatory static enforcement. Advisory memory directed attention toward unmodeled language boundaries only; both findings were independently reproduced against the current source snapshot.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 210 passed in 169.93 seconds. - Ran the standalone checker: passed with exactly six production reads.
- Ran Ruff and Ruff format checks on the changed Python files: passed.
- Enumerated all seven
EvaluationConfigand sevenConsensusConfigfields independently. - Existing annotation coverage tests only module-level eager/postponed annotations; function signature and function-local annotation semantics are missing.
Merge Recommendation
Request changes. Model annotation execution according to scope and postponed-annotation semantics, add bidirectional full-contract regressions for function signatures and local annotations, then rerun the focused scanner suite and standalone six-read audit.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 4e9aa19
request_id: req_1786886298_2128
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD 8743bf4. Eager function parameter/return annotations are now evaluated while postponed annotations remain inert; bare function-local variable annotations are excluded without suppressing module/class annotations. Added bidirectional full-contract regressions. Scanner 213 passed, standalone exactly six production reads, module-size 33 passed, Ruff/format/diff clean. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 8743bf4609c0c43570a4dac6f511ab32dae40648 |
| Request ID | req_1786887087_2131 |
| Review record | bccf8f7d-7d67-4f07-94ce-157780aea7ff |
What Improved
- Enumerates all fourteen
EvaluationConfigandConsensusConfigfields and accurately documents the current six wired and eight inert fields. - Adds structured inert-field markers, pinned model-default identities, extensive scanner regressions, and a required CI contract check.
- Broadly improves provenance handling across aliases, control flow, containers, imports, protocols, and callable boundaries.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig / ConsensusConfig reference drift mechanically detectable |
Not met — valid decorator-mediated reads can pass as inert without a violation. |
| Require every field to be runtime-read, documented inert, or schema-only allowlisted | Partially met — all current fields are enumerated, but runtime-read classification is not authoritative. |
| Preserve provenance across aliases, mutations, control flow, exceptions, callable boundaries, containers, re-exports, and inheritance | Partially met — broad coverage exists, but class decorators and unresolved preserving function decorators lose executable provenance. |
| Add the contract checker as a CI lint gate | Met — .github/workflows/lint.yml runs the checker. |
| Keep current inert-field documentation aligned with runtime behavior | Met — the inspected eight inert fields and six production reads match current runtime wiring. |
| Mechanically pin documented model-default identities | Met. |
| Complete issue #1998’s config-surface implementation | Partially met — the enforcement mechanism remains bypassable at ordinary decorator boundaries. |
Prior Findings Status
Prior findings were not reused as facts. The current focused suite indicates the previously discussed annotation, callable, protocol, and reachability cases are now covered. The established bidirectional-soundness standard remains applicable, and the two decorator-boundary blockers above were independently reproduced against the current source snapshot.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:5444 | BLOCKING | visit_ClassDef visits class-decorator expressions but never applies their result to created_class, so executable reads introduced by class decorators are silently missed. I reproduced this with an exact local _install decorator that adds a method reading settings.evaluation.stage1_enabled; Reader().read() returned the field at runtime, while runtime_reads() returned an empty set and audit_contract() accepted the field as inert with no violations. The reverse direction also fails: a decorator returning None leaves the original class and its methods visible to the scanner. Model exact class decorators or emit an unresolved-semantics contract violation instead of treating the undecorated class as authoritative. |
| 2 | scripts/check-config-reference-contract.py:5355 | BLOCKING | An unresolved function decorator returns _UNKNOWN_VALUE, but this uncertainty is discarded rather than failing the audit. I reproduced this with the standard-library @functools.singledispatch: calling the decorated default implementation read settings.evaluation.stage1_enabled at runtime, while the scanner reported no reads and accepted inert documentation without violations. Thus the stated “fail closed” behavior is actually fail-open for real reads behind valid preserving decorators. Either preserve provenance for such decorators or propagate unresolved callable semantics into an audit failure. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 213 passed. - Ran the standalone repository contract: passed with the expected six production reads.
- Ruff and Ruff format checks passed for both changed Python files.
- Added read-only adversarial probes for exact local class decorators and
functools.singledispatch; both executed the field read under Python but produced no scanner read and no inert-documentation violation. - Existing tests contain no class-decorator or
singledispatchcoverage.
Design Notes
The schema enumeration, documentation contract, and CI integration are directionally sound, but the checker currently conflates “unresolved semantics” with “no runtime read.” A mandatory source-of-truth analyzer must instead model those boundaries or reject them conservatively.
Design / Roadmap Gate
This PR affects mandatory repository-wide configuration enforcement, not runtime persistence or replay. Existing schemas, defaults, current documentation, and current runtime compatibility remain intact. The gate is nevertheless unsafe for future configuration evolution because ordinary decorators can conceal real reads or preserve erased reads without producing uncertainty, allowing CI to certify an incorrect disposition.
Directional Notes
Review focused on runtime reality and bidirectional analyzer soundness rather than treating the large self-authored fixture matrix as completeness proof. Maintainer memory was used only to prioritize adversarial decorator and callable-identity probes; both blockers rely on current-source behavior and direct runtime reproduction.
Test Coverage
- Ran
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 213 passed. - Ran the standalone repository contract: passed with the expected six production reads.
- Ruff and Ruff format checks passed for both changed Python files.
- Added read-only adversarial probes for exact local class decorators and
functools.singledispatch; both executed the field read under Python but produced no scanner read and no inert-documentation violation. - Existing tests contain no class-decorator or
singledispatchcoverage.
Merge Recommendation
Request changes. Correct or conservatively reject class-decorator transformations and unresolved function decorators, add full-contract positive and overreach-negative regressions, then rerun the focused scanner suite, standalone six-read audit, and changed-file lint checks.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 8743bf4
request_id: req_1786887087_2131
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
@ouroboros-agent please re-review exact HEAD 904c68f. Exact local class decorators now apply their returned class transformations, including installed methods and erasing decorators. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2016 |
| HEAD checked | 904c68f2afe42095399281c2b8d3b8d276b7a422 |
| Request ID | req_1786888917_2135 |
| Review record | 8ef2990a-dd7e-4781-8d58-69bb716e49b8 |
What Improved
- Adds CI enforcement for the evaluation/consensus configuration-reference contract.
- Accurately enumerates the current schemas and documents the eight currently inert fields with effective controls.
- Provides extensive tests for aliases, control flow, serialization, callbacks, descriptors, and other Python execution boundaries.
Issue Requirements
| Requirement | Status |
|---|---|
Make EvaluationConfig and ConsensusConfig reference drift mechanically detectable |
Not met — the checker still accepts both a missed executable read and an incorrectly credited replaced read. |
| Require every field to be runtime-read, documented inert, or intentionally allowlisted | Partially met — the disposition mechanism is implemented, but its runtime-read input is unsound in both directions. |
| Enumerate both schemas with exact dispositions | Met for the current source snapshot. |
| Preserve runtime provenance across aliases, mutations, control flow, callable boundaries, containers, re-exports, and inheritance | Partially met — broad coverage exists, but unresolved decorator boundaries still lose indirect provenance. |
| Fail closed for unresolved replacement decorators | Not met — uncertainty depends on shallow syntactic config mentions and is bypassed for name-allowlisted decorators. |
| Add the contract checker to CI lint | Met. |
| Document current inert fields and their effective controls | Met. |
| Mechanically pin the intended direct/OpenRouter Opus default identities | Met. |
| Complete issue #1998’s mechanically enforceable config surface | Partially met pending sound decorator-boundary handling. |
Prior Findings Status
Prior direct decorator, property, serialization, callback, annotation, and control-flow concerns appear substantially addressed. The established bidirectional-soundness concern remains, but the current blockers are freshly reproduced variants: indirect reads hidden by unresolved decorators and false reads credited through name-only decorator preservation.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | scripts/check-config-reference-contract.py:1157 | BLOCKING | Unresolved decorators fail open when the decorated node reaches configuration through an indirect private helper. _record_unresolved_decorator() only records uncertainty when ast.walk(node) directly finds a config-like name or tracked section. A reproduced @functools.total_ordering class whose invoked __eq__ calls _read(), where _read() loads settings.evaluation.stage1_enabled, produces no runtime read and no unresolved-semantics violation. Supplying inert documentation then passes audit_contract() with zero violations even though Reader() == Reader() executes the read. This allows real future wiring to remain documented inert. Unresolved reachable decorators must conservatively fail closed based on transitive callable relevance, or the decorated execution path must be modeled. |
| 2 | scripts/check-config-reference-contract.py:5565 | BLOCKING | Decorators are treated as semantics-preserving solely by their final textual name. When an unresolved imported decorator is named dataclass, command, callback, field_validator, or retry_async, the scanner preserves the original class/function without establishing the decorator’s identity. A reproduced from external import dataclass case records a read from the original decorated class and reports no uncertainty, although that external decorator can validly replace the class with an implementation that performs no read; active-field documentation then passes the audit incorrectly. Resolve the exact decorator provenance, or fail closed rather than trusting an unqualified basename. The equivalent function path at line 5423 has the same issue. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| None. |
Non-blocking Suggestions
None.
Test Coverage Notes
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 216 passed.- Standalone repository contract passed and reported the expected six production reads.
- Ruff check passed for the changed script and test; Ruff format check reported both files formatted.
- Independent adversarial probes reproduced both blockers and demonstrated that
audit_contract()returns no violations for the incorrect dispositions.
Design Notes
The schema/docs disposition model is clear and the current classifications appear truthful. The remaining problem is that the mandatory CI authority attempts semantic Python interpretation while failing open at decorator boundaries.
Design / Roadmap Gate
This PR changes a repository-wide CI contract for future configuration evolution, not runtime persistence or compatibility. Current schemas, documentation, defaults, and six existing reads remain consistent. However, ordinary decorator transformations can still hide a reachable read or preserve a replaced read without triggering uncertainty, so the checker cannot safely act as the configuration-reference source of truth.
Directional Notes
Review followed the established requirement that mandatory static enforcement be sound in both directions. Maintainer memory was used only to prioritize adversarial decorator and callable-provenance probes; both blockers are independently reproduced from the current source.
Test Coverage
SETUPTOOLS_SCM_PRETEND_VERSION=0.0.0 /root/.local/bin/uv run python -m pytest tests/unit/scripts/test_check_config_reference_contract.py -q: 216 passed.- Standalone repository contract passed and reported the expected six production reads.
- Ruff check passed for the changed script and test; Ruff format check reported both files formatted.
- Independent adversarial probes reproduced both blockers and demonstrated that
audit_contract()returns no violations for the incorrect dispositions.
Merge Recommendation
Request changes. Make unresolved decorators conservatively fail closed across indirect callable dependencies, remove basename-only preservation in favor of proven decorator identity, and add positive and overreach-negative regressions for both reproduced cases.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: 904c68f
request_id: req_1786888917_2135
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1 participant
Summary
EvaluationConfig/ConsensusConfigreference drift mechanically detectable instead of relying on repeated human review.Changes
finally, exception domains, andTryStarsubgroups.Prior review blockers
TYPE_CHECKINGreachability and section-typed helper findings are covered.__init__re-exports, and inherited unannotated methods. Exact HEAD618bc0f79be8435bf8f0e75345342a3b3f6d501eaddresses all five with positive and overreach-negative regressions.Direction and overlap
maincbedda5e5a5b79e97a2d4dc0a9222a2c21fd19c9.Verification
618bc0f79be8435bf8f0e75345342a3b3f6d501eoncbedda5e5a5b79e97a2d4dc0a9222a2c21fd19c9.90a462cde60bb322417f7e44fe29c7e7c4d5148afe9fdb87717cf2033710d8f3.Closes #1998