feat: add reusable offline price gap patches - #103
Conversation
protostatis
left a comment
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR adds a reusable, spec-driven offline CoinGecko price-gap patch toolkit (collection, sealed artifacts, transactional apply, manifest migration, read-only inspection, and guarded rollback). It is careful, defensive database-mutation tooling rather than the Docker/shell configs this skill typically targets, so I reviewed it for security/correctness/reliability of the write path. The safety posture is strong: collection never touches a production DB, apply/rollback require exact multi-field confirmations, writers-paused checks, BEGIN IMMEDIATE transactions, force-verified pre-mutation backups, foreign-key baseline tracking, and all-or-none commit. I found no secrets, no glaring security holes, and no production-breaking bug in the visible portion. The diff was truncated (4 files not shown, likely tests/docs/fixtures), so the 234-pass test claim and the remaining files were not independently verifiable. Verdict is comment: a handful of robustness/correctness nits worth addressing, none blocking.
Verdict: Comment
Comments
- The diff is truncated ('... 4 more file(s) truncated due to size'). The remaining files are almost certainly tests/docs/fixtures, but the 234-pass test claim, the wheel build, and the packaged legacy-spec import could not be verified from this diff alone. Please confirm the truncated files don't contain any executable/CI changes.
- rollback_patch opens a read-write connection (_readwrite_connection) and starts BEGIN IMMEDIATE before validating the confirmation flags (_guard_mutation runs after acquiring the write lock). This is not a correctness bug, but validating the confirmations before opening the write lock would avoid briefly taking the production write lock on a command that will just fail confirmation. Same ordering observation applies between classify_target and the earlier confirmation checks in apply_patch (apply_patch does confirm before opening, which is the better pattern).
- The pre-mutation backup in apply_patch is taken while holding BEGIN IMMEDIATE (RESERVED/write lock) but is produced through a separate read-only connection via the SQLite online-backup API. This is durable and consistent today, but it depends on the subtle invariant that no data mutation happens before _backup_database. A short comment asserting 'backup must be the first data-affecting step after classify' would protect that ordering from future refactors.
- _migrate_manifest_to_v2 relies on transactional DDL (ALTER TABLE ... RENAME / DROP TABLE / CREATE TABLE inside the open transaction) and asserts connection.in_transaction after each step. This is correct on modern SQLite but the module should document the minimum SQLite version requirement (>=3.25), since on older versions some DDL would implicitly commit and silently break the all-or-none guarantee.
- Exact float equality is also used implicitly in _candidate_digest (hash of canonical JSON of floats) and in artifact reproduce checks. Because the JSON bytes are serialized deterministically from the same Python floats, this is self-consistent and fine within one process/version — just avoid regenerating fixtures across Python/SQLite versions without re-baselining the pinned hashes.
- Overall the safety design (no DB path in collection, exact confirmations, BEGIN IMMEDIATE, verified backup, FK baseline, all-or-none commit, migrated manifest compatibility with the Aug 2026 v1 artifact) is excellent and clearly meets its stated Safety goals.
Reviewed by Sky — Unchained Sky engineering agent
| schema_version=schema_version, | ||
| incident_id=incident_id, | ||
| provider=provider, | ||
| source_tag=source_tag, |
There was a problem hiding this comment.
source_tag length is guarded against >50, but that 50-char limit is a bare magic number with no constant or citation to the actual price_data source column length. If the real column ever differs, this check drifts silently. Extract it to a named constant and/or derive it from the schema.
| "start_hour": self.start_hour.isoformat(), | ||
| "end_exclusive": self.end_exclusive.isoformat(), | ||
| "coins": dict(self.coins), | ||
| } |
There was a problem hiding this comment.
fetch_start/fetch_end pad one hour on each side of the gap. Worth a comment documenting that CoinGecko's market_chart/range returns data keyed to the bucket start, so the padding is required to correctly reconstruct the first/last full UTC hours. It is a subtle correctness assumption that is easy to break later.
| f"Artifact metadata mismatch for {key}: {metadata.get(key)!r} != {expected!r}" | ||
| ) | ||
|
|
||
| coin_rows = connection.execute( |
There was a problem hiding this comment.
_value_matches uses exact == on floats for price_usd/volume_24h/market_cap verification. Exact float equality on DB round-tripped REAL values is fragile — a value that survives an insert/select round-trip today could differ by 1 ULP under a different SQLite build or driver and cause a spurious 'applied row no longer matches' failure on verify/rollback of an otherwise-intact row. Consider a tolerance (math.isclose or a small relative epsilon) while still failing loudly on material divergence.
| f"{coin.lower()}-attempt-{attempt.attempt}-" | ||
| f"http-{attempt.http_status}.{suffix}" | ||
| ) | ||
| response_path = temporary / name |
There was a problem hiding this comment.
MAX_RESPONSE_BYTES is enforced only after response.content has already been fully buffered into memory (httpx reads the body before this check). The 20 MiB cap therefore bounds stored size/disk but not peak memory. If protecting memory is a goal, stream or cap via a client-side streaming read instead of buffering content first.
| f"got {len(successful_attempts)}" | ||
| ) | ||
| successful_raw_id = None | ||
| for attempt in attempts: |
There was a problem hiding this comment.
retryable status logic maps http_status None to retryable, but a timeout/HTTPError (attempt.http_status is None) uses the same exponential backoff path as transient 5xx. That is fine, but note there is no retry budget/pause on a persistent non-retryable 400/401/403 — it correctly breaks immediately, which is the right behavior for a wrong API key or bad coin id.
1 participant
Summary
Safety
BEGIN IMMEDIATEwith a verified pre-mutation backupVerification
uv run pytest -q— 234 passed, 1 skipped