Fix acquire-sky-supply-basis workflow: canonical idempotency, chronology, signing-key validation, DB safety net#116
Merged
Conversation
…real bugs The embedded-YAML Python had already gone through two incremental guess-and-patch fixes (PR #112, PR #114/#115) that only addressed the mf_result.status != "ok" typo. A full audit against the actual market_facts and value_capture contracts, plus a real end-to-end local simulation against a copy of PR #110's branch database, found further defects the incremental patches never reached: - Idempotency check hand-rolled value_capture.service._logical_id()'s hash formula instead of using the repository's own public strict_known_supply lookup. It happened to be numerically correct, but was fragile and violated the "no approximate reimplementation" principle. Replaced with strict_known_supply (also removes the only need for a hashlib import). - No precondition check that the Sky ValueCaptureRuleSnapshot already exists before attempting to build a SupplyBasisSnapshot that implicitly depends on it existing elsewhere in the evidence chain. - Signing-key env vars were decoded with a bare bytes.fromhex() with no validation, producing a raw traceback instead of an actionable error on a missing/malformed secret. - requested_fact_types included spot_price, which is never used in the supply-basis payload, and got referenced in observed_market_fact_ids anyway, creating a semantic mismatch (an unrelated price observation cited as supply-basis evidence). - No check that only "snapshot" rows changed in the canonical database before allowing a commit; the repository already suffered exactly this failure mode once (tests/test_data_ops.py's hardcoded DB path polluting data/data_ops.sqlite with unrelated automation-job rows during PR #110's own remediation). - A genuine chronology bug local simulation caught directly: the script reused one external `now` for both the market-fact request's requested_at/recorded_at AND the value-capture acquisition's effective_at/acquired_at, but CoinGeckoObservedMarketFactProvider stamps its own real result.acquired_at/known_at independently. Any real execution would non-deterministically fail authority checks depending on provider call latency. Fixed by anchoring requested_at a safety margin before `now`, using the provider's own result.known_at for recorded_at, and anchoring the supply acquisition's timestamp to max(now, *market_fact.known_at) so it can never precede what it cites. Extracted the acquisition logic to scripts/acquire_sky_supply_basis.py (ruff/black/mypy-covered, pyproject.toml files/pythonpath updated) with tests/test_acquire_sky_supply_basis.py (14 tests): signing-key validation, the real "success" status literal, missing-evidence/missing-rule preconditions, full run() end-to-end against a real synthetic DB with cross-linked evidence/rule/market-fact/supply verification, idempotent rerun (asserting the network transport is never called and row counts are byte-for-byte unchanged), and the database diff-boundary safety net. Locally simulated the corrected script end-to-end against a real copy of PR #110's branch database (data/data_ops.sqlite, configs/*.yaml) with an injected, schema-valid (not live) CoinGecko response, since api.coingecko.com is policy-denied in this session. Confirmed: market facts accepted, the existing evidence (c53f556b...) and rule (f774f946...) records found via canonical strict-known lookups, SupplyBasisSnapshot persisted and independently re-verified linked by entity/representation/evidence/ market-fact/chronology, a second run exits idempotently with zero writes and zero network calls, and the database diff was exactly the 4 expected new snapshot rows with automation-job/automation-run counts unchanged. Workflow YAML: acquisition logic now lives in scripts/ on main (lint/type/ test-covered) rather than embedded in the workflow. Checkout still targets the PR #110 branch (it owns the canonical database), with a new step that materializes the reviewed script from main via `git show origin/main:...` without committing it to the PR branch — only data/data_ops.sqlite is ever committed there, unchanged from before. Pinned action SHAs, permissions, concurrency group, and the commit/push procedure are unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZwAhhLUYJMGgJm9yyzs7H
fafa33
marked this pull request as ready for review
July 26, 2026 06:05
This was referenced Jul 26, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full end-to-end audit and fix of
.github/workflows/acquire-sky-supply-basis.yml(the one-shot escape hatch that persists PR #110's missing SkySupplyBasisSnapshot). Two prior incremental patches (PR #112, then #114/#115) only fixed thestatus != "ok"typo — this audit found and fixed everything else, including a genuine chronology bug that would have caused non-deterministic failures on any real run.Root causes found (none previously fixed)
hunter.value_capture.service._logical_id()'s hash formula instead of calling the repository's own publicstrict_known_supplylookup. Numerically correct by luck, but fragile and against the "no approximate reimplementation" principle.ValueCaptureRuleSnapshotalready existed before building aSupplyBasisSnapshotthat depends on the same evidence chain.bytes.fromhex(os.environ[...])with no pre-check — a missing/malformed secret produced a raw traceback, not an actionable error.spot_price(unused in the supply-basis payload) and referenced it inobserved_market_fact_idsanyway — an unrelated price observation cited as supply-basis evidence.snapshotrows changed before committing — the repository already suffered exactly this failure mode once (tests/test_data_ops.py's hardcoded DB path pollutingdata/data_ops.sqlitewith unrelatedautomation-jobrows during PR Issue #107 Milestone 1: entity-agnostic valuation evidence acquisition (Sky pilot) #110's own remediation).nowfor both the market-fact request'srequested_at/recorded_atand the value-capture acquisition'seffective_at/acquired_at, butCoinGeckoObservedMarketFactProviderstamps its own realresult.acquired_at/known_atindependently. Any real execution would have non-deterministically failed authority checks depending on provider call latency — this would never have shown up from reading the code, only from running it.Fix
Extracted acquisition logic to
scripts/acquire_sky_supply_basis.py(ruff/black/mypy-covered —pyproject.toml'smypy.files/pytest.pythonpathupdated to includescripts), fixing all six issues. Addedtests/test_acquire_sky_supply_basis.py(14 tests). The workflow YAML now checks out the PR #110 branch as before (it owns the canonical database) and materializes the reviewed script frommainviagit show origin/main:...into that checkout without ever committing it there — onlydata/data_ops.sqliteis committed, unchanged from before. Pinned action SHAs, permissions, and concurrency group are unchanged.Local simulation (not a live CI run — see below)
api.coingecko.comis policy-denied in this session (confirmed via the proxy status endpoint), so I could not run the live workflow. Instead, ran the corrected script directly against a real copy of PR #110's branch database and configs, with an injected schema-valid (not live) CoinGecko response:circulating_supply,total_supply).c53f556b...) and rule (f774f946...) found via canonicalstrict_known_*lookups — no hardcoded IDs.SupplyBasisSnapshotpersisted; independently re-verified linked by entity/representation/evidence/market-fact/chronology.SupplyBasisSnapshot already exists ... — idempotent no-op, zero database writes, and the injected transport'sget_json(which raisesAssertionErrorif called) was never invoked — proving no network call is attempted on a no-op rerun.snapshotrows (2 market facts + 1 receipt + 1 supply record);automation-job(10) andautomation-run(20) counts unchanged before/after.Is a live rerun required?
Yes. This PR only fixes the workflow; it does not and cannot persist the real
SupplyBasisSnapshotitself (no live CoinGecko access in this session, and no data was fabricated). After this PR merges: Actions → "Acquire Sky SupplyBasisSnapshot (Milestone 1)" → Run workflow → branchmain, withHUNTER_VALUE_CAPTURE_SIGNING_KEY_ID/HUNTER_VALUE_CAPTURE_SIGNING_KEYrepository secrets already set (unchanged requirement from PR #112).Verification
ruff check .— passblack --check .— passmypy— pass (570 files)pytest— 1134 passed, 0 failed (1120 pre-existing + 14 new, zero regression)Remaining limitations
Not merging or rerunning the live workflow — leaving both for review/authorization, per instructions.
Generated by Claude Code