[codex] Fix Pydantic model_fields access#100
Conversation
tavateva
left a comment
There was a problem hiding this comment.
Draft Promotion Criteria — FAIL on criterion 4 (fixable documentation gap)
Fix is correct and minimal. Blocking promotion on the same missing-pass-count issue we hit on den-sq/mctutil#78 and, in parallel, on #99.
Passes:
-
CI & mergeability — all 3 checks green (`coverage`, `server-image`, `test-coverage`); `mergeable: MERGEABLE`, `mergeStateStatus: CLEAN`. ✓
-
Issue Acceptance Criteria — issue #98's 3-item checklist:
- `models.py:117` uses class-level `model_fields` access → `type(self).model_fields.keys()` at models.py:117. ✓
- Coverage workflow no longer emits the `PydanticDeprecatedSince211` warning → `@pytest.mark.filterwarnings("error::pydantic.PydanticDeprecatedSince211")` on `test_model_with_json` enforces this at the unit level; CI coverage run passed. ✓
- Ripgrep confirms no other instance-level `model_fields` remains → PR body explicitly reports `rg "self\.model_fields|self\.model_fields_set|self\.model_computed_fields" python/ouroboros` returned no matches. ✓
-
Test Plan checkboxes — no checkbox section. N/A.
-
Tests run, not just present — Validation section reads:
`poetry run pytest test/helpers/test_models.py -W error::pydantic.PydanticDeprecatedSince211`
`rg "self\.model_fields|..." python/ouroboros` returned no matchesIssue #98's AC includes both a test requirement and a verification command that names a specific pytest invocation. Criterion 4 requires concrete pass/fail counts ("X passed / Y skipped") in the PR body. `rg` result is reported concretely; pytest result is not. Failure.
-
Scope — models.py -1/+1 (single-line fix); test_models.py +2 (`pytest` import + filterwarnings marker). Trivially tight to #98. ✓
-
Regressions & patterns — `type(self).model_fields` preserves subclass polymorphism (Pydantic resolves `model_fields` on the leaf class, so subclass fields are still iterated); no behavior change for callers. Existing `test_model_with_json` still passes on the coverage run, so the marker doesn't spuriously fire on unrelated warnings. ✓
-
Judgement calls — used `type(self).model_fields` rather than `self.class.model_fields` (both work, both were listed as acceptable in #98). No explicit reasoning in PR body, but this is a stylistic call within the sanctioned set. ✓
Fix to promote: append pass counts to the Validation section, e.g.:
```
poetry run pytest test/helpers/test_models.py -W error::pydantic.PydanticDeprecatedSince211
→ N passed, M skipped
```
Also worth including a full-suite `pytest` run with its count. Then all 7 criteria hold.
tavateva
left a comment
There was a problem hiding this comment.
Re-review — Draft Promotion Criteria PASS ✅
Validation section updated with concrete pass counts. All 7 criteria hold.
- CI & mergeability — 3 checks green (`coverage`, `server-image`, `test-coverage`); MERGEABLE / CLEAN. ✓
- Issue #98 AC — `type(self).model_fields.keys()` at models.py:117; `filterwarnings("error::pydantic.PydanticDeprecatedSince211")` marker on `test_model_with_json`; `rg` reported no other instance-level `model_fields` access. ✓
- Test Plan checkboxes — N/A.
- Tests run with counts — targeted `pytest test/helpers/test_models.py -W error::pydantic.PydanticDeprecatedSince211` → 7 passed in 0.08s; `rg` → no matches; full `pytest` → **168 passed, 1 warning in 7.75s`. The remaining warning is explicitly identified as the sibling `SharedNPArray.del` unraisable warning tracked in #97 / PR #99 — no hidden regressions. ✓
- Scope — models.py -1/+1; test_models.py +2. ✓
- Regressions & patterns — 168 matches the baseline from PR #96; no test count delta expected (no new tests, just a stricter marker on an existing one). ✓
- Judgement calls — reasoned inline. ✓
Approving. Safe to promote and merge.
Fixes #98
Summary
copy_values_from_otherto class-level Pydanticmodel_fieldsaccess throughtype(self)PydanticDeprecatedSince211warnings fail that test pathRoot Cause
Pydantic 2.11 deprecated accessing
model_fieldson a model instance. The helper copied fields withself.model_fields, which still works today but warns and is scheduled for removal in Pydantic 3.0.Validation
poetry run pytest test/helpers/test_models.py -W error::pydantic.PydanticDeprecatedSince211rg "self\\.model_fields|self\\.model_fields_set|self\\.model_computed_fields" python/ouroborospoetry run pytestSharedNPArray.__del__unraisable warning tracked by SharedNPArray.__del__ AttributeError on partially-constructed instance (mem.py:76) #97 / PR [codex] Fix SharedNPArray partial init cleanup #99