feat: add trusted sqlite qpk-vnext N2 store#259
Conversation
🤖 Codex PR Review🚫 Merge blocked: 1 serious issue(s) found in high-risk files 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Bug in
|
Co-Authored-By: Codex <noreply@openai.com>
Pigbibi
force-pushed
the
codex/qpk-vnext-n2-sqlite-reslice
branch
from
f6cb211 to
1421edd
Compare
|
Concentrated closure (head |
![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: f6cb211218
ℹ️ 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".
| if bytes(row[0]) == payload: | ||
| conn.execute("COMMIT") | ||
| return "idempotent" |
There was a problem hiding this comment.
If an existing row has the same key and canonical payload but stale selector columns (for example after local DB corruption or a partial/manual migration), this branch commits and returns idempotent without running _validate_columns. Because list_keys filters by domain/profile/timing before it decodes rows, that “successful” retry leaves the result hidden under its real selector and only fails if queried through the corrupted selector, so the store no longer fails closed on identity-column corruption.
Useful? React with 👍 / 👎.
| with pytest.raises(StoreError): | ||
| SqliteResultStore(db).put(contract()) |
There was a problem hiding this comment.
In environments where pytest runs as root (including many Docker CI jobs), chmod(0o500) does not stop root from creating results.sqlite, so SqliteResultStore(db).put(contract()) succeeds and this pytest.raises assertion fails; I reproduced this with PYTHONPATH=src python -m pytest tests/test_qpk_vnext_n2_store.py -q in the provided container. Skip/adjust this case for root or assert permissions with a mechanism that is effective for the runner.
Useful? React with 👍 / 👎.
| conn.execute("INSERT OR IGNORE INTO store_meta(key, value) VALUES (?, ?)", | ||
| ("namespace", NAMESPACE)) |
There was a problem hiding this comment.
When db_path accidentally points at an existing SQLite database without the qpk-vnext namespace row, this INSERT OR IGNORE path creates the marker (after also creating tables) and subsequent calls use that file as an N2 store instead of failing as an incompatible/unmarked store. That violates the exact schema-marker/no-legacy-read contract and can mutate an unrelated local database before the caller notices the wrong path.
Useful? React with 👍 / 👎.
| "FROM results WHERE key=?", (key,)).fetchone() | ||
| if row is None: | ||
| _fail() | ||
| item = decode_wire(json.loads(bytes(row[1]).decode("utf-8"))) |
There was a problem hiding this comment.
If a local/corrupted database stores payload as TEXT (SQLite permits this even for a BLOB column), bytes(row[1]) raises TypeError, which is not caught here, so exact reads leak an unexpected exception instead of the documented sanitized StoreError fail-closed behavior; the selector decode path has the same issue for any selected corrupted row.
Useful? React with 👍 / 👎.
| domain, profile = _segment(domain), _segment(profile) | ||
| except ContractError: | ||
| _fail() | ||
| if timing not in {"next_open", "next_close"}: |
There was a problem hiding this comment.
list_keys accepts invalid selectors that ResultContract would reject: for example profile='soxl' passes _segment and silently returns an empty result even though N1 requires uppercase profiles, while an unhashable timing such as [] raises TypeError instead of StoreError. These cases can make case/config mistakes look like missing experiment output or break the store's sanitized fail-closed API.
Useful? React with 👍 / 👎.
|
PERMANENT_FREEZE: closure exhausted; URI/path lifecycle and payload/connection hardening indicate scope must narrow before successor reslice. Frozen evidence retained. |
1 participant
Fresh QPK-N2 SQLite reslice
Fresh base:
origin/main@a06a99f74566ce60ae91fc39317cdfc16558e325; no code from frozen PRs #257/#258 is reused.N2 is explicitly a trusted single-process local research backend using only stdlib
sqlite3:synchronous=FULL; ephemeral rejects before open/createqpk-vnext/result/v2; no legacy readsValidation
5 passed619 passed, 1 skippedReview mode: preflight then one concentrated closure maximum; remediation_count=0.