Repository navigation
fix(audit): bump SCHEMA_VERSION to 2 for scorer-stamped records (#315) - #324
Conversation
Records with the post-#312 scorer_version shape now persist schema_version=2 so older v1-only readers hit their explicit unsupported-version raise instead of TypeError→corrupt-skip on pending work. New reader migrates v1 lines (including the mistaken early stamp under schema_version=1). Scorer-identity behaviour from #304/#312 is unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
LGTM (posted as COMMENT; self-approve is blocked on own PRs).
Summary
Closes #315: SCHEMA_VERSION → 2 for the scorer-stamped audit shape; new reader migrates v1 (including the mistaken early stamp); old v1-only readers hit explicit unsupported-version rejection instead of TypeError→corrupt-skip on pending work. #304/#312 scorer-identity behaviour preserved.
Verification
- Local:
test_audit_store_durability, sparse, attach, cheap-layer,test_verifier_boost, diagnose_script — green. - Changelog fragment present; squash auto-merge enabled.
No blocking changes. CI matrix still pending at review time.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e17070e9f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| d = dict(d) | ||
| version = d.get("schema_version") | ||
| if version != SCHEMA_VERSION: | ||
| if version not in _SUPPORTED_SCHEMA_VERSIONS: |
There was a problem hiding this comment.
When an existing v1 line contains valid JSON but cannot construct an AuditRecord (for example, it is missing purpose or has an invalid inclusion probability), accepting v1 here makes AuditStore._load_line call from_dict in its preflight outside the guarded construction block. The resulting KeyError/TypeError/ValueError aborts opening the entire store instead of counting and skipping that one corrupt line as documented; distinguish supported versions before construction or move migration construction under the existing exception handler.
Useful? React with 👍 / 👎.
| known = set(cls.__dataclass_fields__) | ||
| return cls(**{k: v for k, v in d.items() if k in known}) |
There was a problem hiding this comment.
Filtering to dataclass fields silently accepts malformed v1 and v2 records with unknown keys. For example, a persisted record that misspells optional stratum as strtaum is now loaded under the default "all" stratum, changing the resulting audit statistics; previously the unexpected keyword caused the line to be counted as corrupt. Since scorer_version is already a known dataclass field, migration does not require dropping every other unknown key.
Useful? React with 👍 / 👎.
…ields (#315) Addresses Codex review on #324: - P1: AuditStore._load_line preflight now only raises on versions outside SUPPORTED_SCHEMA_VERSIONS, so a malformed v1 line goes through the guarded construction and is counted corrupt instead of aborting the whole store. - P2: AuditRecord.from_dict no longer filters unknown keys; a misspelt field (e.g. strtaum) raises TypeError and is counted corrupt rather than loading under default values. Also regenerates docs/api.md (SCHEMA_VERSION=2), which failed the test matrix.
Birfy
left a comment
Birfy
left a comment
There was a problem hiding this comment.
Pushed 3e7f6ad addressing both Codex findings and the red test matrix:
- P1 (malformed v1 aborts the store):
AuditStore._load_linenow only takes the raising preflight for versions outsideSUPPORTED_SCHEMA_VERSIONS; v1 and v2 both go through the guarded construction, so a v1 line missingpurpose/task_id/verifier_scoreor withinclusion_prob=0is one corrupt line, not an unopenable store. - P2 (unknown keys silently defaulted):
AuditRecord.from_dictno longer filters to dataclass fields; a misspeltstrtaumraisesTypeErrorand is counted corrupt instead of landing in stratumall. - CI: all five
test (3.x)jobs failed only ontest_api_reference(docs/api.md still showedschema_version: int = 1); regenerated. Also merged currentmain.
New tests: malformed-v1 (3 params), invalid inclusion prob under v1, unknown field under v1 and v2, and unknown schema_version=3 still raises. The 6 corrupt-path tests fail without the fix; tests/test_audit_store_durability.py passes in full. Changelog fragment updated. LGTM once the matrix is green.
Closes #315
Problem
#312 added optional
AuditRecord.scorer_versionwhile leavingSCHEMA_VERSIONat 1. An older v0.6.0 reader accepts schema version 1 but has noscorer_versionconstructor parameter;from_dictforwards the new field →TypeError→AuditStorecounts the line corrupt and silently skips pending work.Fix
SCHEMA_VERSIONto 2. Writers always stamp the current marker; nonemptyscorer_versionis included on the line, empty is still omitted.from_dictmigrates v1 lines (unstamped and the mistaken early stamp that wrotescorer_versionunderschema_version=1) and raises explicitly on any other version.schema_version=2and take their unsupported-versionValueErrorpath instead of TypeError→corrupt-skip.Tests
tests/test_audit_store_durability.py: unstamped/stamped v2 round-trips, v1 migration (pending/resolved/buggy stamp), mixed v1+v2 load without rewriting bytes, simulated old-reader contract.Changelog
changelog.d/315-audit-schema-version.md