Part 1: Run-cohort safety (run_id, purge command, staleness guard) - #28
Merged
Conversation
Supersedes the plan-mode scratch file (~/.claude/plans/, session-local) after a mid-session rate-limit interruption required reconstructing plan state from the conversation transcript. The lesson driving this commit: the plan lives in git, not in a scratch file, so a future interruption can recover from git log/git status alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
Additive migration (0061) - one AddField wave covers CardPrintingTag/ CardArtistVote/CardTagVote via the shared abstract base, plus a new PilotRunLedger model in the same migration (no functional reason to split it from the field-addition wave, unlike the historical 0059/0054 precedent this was originally going to mirror). Registered in admin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
generate_run_id() (timestamp + short random suffix, deliberately not the git SHA - that's logged separately). run_pilot()/run_name_frequency_ elimination() each generate their own run_id if not passed explicitly and thread it through every CardPrintingTag/CardTagVote construction site, including cluster-vote propagation and the three attribute-vote casters in local_fallback.py. Human-submitted votes are untouched (views.py never passes a run_id kwarg, stays NULL). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
get_baked_git_sha reads a GIT_SHA file baked into the image at build time (Dockerfile ARG + docker-compose build.args, both now require the documented GIT_SHA=$(git rev-parse --short HEAD) prefix on the rebuild command) - best-effort visibility only, logged at startup, never the gate itself. find_stale_applied_migrations is the actual hard gate: compares what this image's own migrations/ directory knows about against what the DB reports as applied (MigrationLoader vs MigrationRecorder) - pure DB+code introspection, automates the PR #24/#26 stale-image lesson instead of relying on someone remembering to check docker images timestamps. Caught a real bug via the new test: MigrationRecorder.applied_migrations() returns a dict, not a set - the first draft's `applied - disk` would have raised TypeError at the first real invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
purge_machine_votes --run-id <id> [--dry-run]: refuses without --run-id, deletes exactly one run's printing/artist/tag votes, re-resolves every affected card via the persisting consensus resolvers. Corrected post-purge invariant (the task's literal "assert status returns to pre-run state" would false-positive on the first real purge - a mixed human+machine vote legitimately un-resolves when its machine votes are purged): asserts any card still RESOLVED has a surviving human-backed vote behind that specific outcome, not that nothing changed. Wired find_stale_applied_migrations + get_baked_git_sha into both existing pilot commands' handle() - staleness check first, before any other work; a PilotRunLedger row created after that passes, updated to COMPLETED/FAILED on the run's actual outcome (never left dangling on an exception). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
New section in docs/features/printing-tags.md pointing to the tracked plan doc for detail, stating all four safety properties this module now guarantees (2 pre-existing: never-resolve gate, restart-safety; 2 new: revocability, staleness guard). Updated Key files and Known gaps (deductive_backfill.py's votes deliberately not in scope for run_id threading this pass). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ
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
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
Part 1 of the six-part catalog-completion plan tracked in
docs/features/catalog-completion-plan.md— makes the owner's aggressive stop → merge → rebuild → restart iteration style safe. HOLD #A per the plan: held for review before Parts 2/3 proceed, and before the live full-catalog run is next restarted with this code.run_idadded toAbstractWeightedVote(coversCardPrintingTag/CardArtistVote/CardTagVotein one migration) — a separate field, not ananonymous_idsuffix. Confirmed via direct investigation (two research passes) thatanonymous_id's exact-match reuse across invocations is load-bearing for the existing idempotence/resume mechanism, and itsmax_length=40would hard-block a stamped value for at least two engine names anyway. NewPilotRunLedgermodel for the durable, queryable run record.local_identify_printing_tags.py/local_fallback.pynow stamps its vote with the invoking run'srun_id. Human-submitted votes are untouched (stayNULL).find_stale_applied_migrations— compares what this image's own migrations know about against what the DB reports as applied, refuses to start if the image is older than a previously-deployed one. Automates the PR Pre-scale program: source exclusion, yield reconciliation, future-work note #24/Two pilot fast-follows: expansion_hint narrowing, name-frequency elimination #26 stale-image lesson instead of relying on manually checkingdocker imagestimestamps. Caught a real bug via its own test (MigrationRecorder.applied_migrations()returns a dict, not a set — the first draft would have raisedTypeErroron first real use).ARG+docker-composebuild.args) — logged at startup, never itself the gate. Rebuild command now requires aGIT_SHA=$(git rev-parse --short HEAD)prefix (documented).purge_machine_votes --run-id <id> [--dry-run]: deletes exactly one invocation's votes, re-resolves every affected card via the persisting consensus resolvers.The post-purge invariant, stated precisely
"Corrected" isn't reviewable without the exact statement, so here it is verbatim against the implementation (
purge_machine_votes.verify_no_machine_only_resolutions):After a real (non-dry-run) purge, every affected card is re-resolved from scratch via the persisting consensus resolvers (
resolve_and_persist_printing/resolve_and_persist_artist/resolve_and_persist_tag_votes), using whatever votes actually remain — not a diff against pre-purge state. The command then asserts: for every affected card whoseprinting_tag_statusisRESOLVED, at least one survivingCardPrintingTagvote for that resolved printing has a human-backedsource(notVoteSource.DEDUCTION/VoteSource.OCR); identically forartist_vote_statusagainst the resolved artist, and per-tag fortag_vote_statusesentries that areRESOLVED_APPLY/RESOLVED_REJECT.A card is NOT required to return to its pre-purge status — un-resolving as a consequence of losing machine-only weight is the expected, correct outcome, reported separately (
cards_unresolved_by_purge), never a violation. Only aRESOLVEDoutcome with zero surviving human-backed votes behind it is a violation (gate_violations, raisesCommandError) —resolve_weighted_consensus's own human-backed gate should make that structurally impossible, so if it ever fires it means something upstream broke, not that the purge did anything wrong.(With the real default weights —
PRINTING_TAG_MIN_VOTES=2,PRINTING_TAG_AI_WEIGHT=0.5, human weight1.0— 1 human vote + 2 agreeing machine votes sums to 2.0 and resolves; purging those 2 machine votes correctly drops the weight below threshold and legitimately un-resolves the card. The original task framing, "assert statuses return to pre-run state," would have false-positived on exactly this case.)Merge + migrate sequencing with the live run in flight
The migration is additive-only (
AddField(run_id, null=True, db_index=True)on three small tables — max 41,571 rows, confirmed viapg_stat_user_tables) — a metadata-onlyALTER TABLEin Postgres 11+, plus a near-instantCREATE INDEXgiven the column starts 100% NULL. Safe to apply live under the running job's concurrent writes.The running job keeps writing votes with
run_idNULL for the rest of its current invocation once this merges and migrates — that's correct and meaningful, not a gap.NULLrun_ididentifies the pre-safety-era cohort: still fully governed by the never-resolve gate and restart-safety (neither was ever conditional onrun_idexisting), just not individually purgeable by run the way anything stamped going forward is. The run is not restarted solely to gain stamping — it folds in naturally at whatever iteration the run next gets restarted for anyway. See the "Cohort convention" note indocs/features/printing-tags.md.Expected side effect: once this merges and migrates, any restart from an older (pre-migration) image trips the staleness guard and refuses to start, forcing a rebuild first. That's the guard doing its job, not a bug.
docs/features/printing-tags.mdstating the complete safety-property set (2 pre-existing: never-resolve gate, restart-safety; 2 new: revocability, staleness guard), the precise post-purge invariant, and the NULL-run_id cohort convention.Test plan
pytest cardpicker/tests/— 756 passed (plus the same pre-existing, unrelated moxfield failures documented in task Document Part 4's missing batch-flush checkpointing #122)pre-commit run(ruff, isort, black, mypy, prettier) — all cleanmakemigrations --check— migration matches model state exactlyrun_idthreading (5 tests),find_stale_applied_migrations/get_baked_git_sha(5 tests),purge_machine_votes(11 tests covering dry-run, real purge with the corrected invariant, multi-run isolation, orphaned-ledger tolerance, and a deliberately-constructed "impossible" gate-violation state),PilotRunLedgerlifecycle (4 tests: dry-run creates nothing, real run completes, mid-run exception fails cleanly not danglingly, staleness refusal creates nothing)docs/lessons.md) in the new test file before it could silently break unrelatedtest_views.pysnapshotsDoes not touch the live full-catalog run (branched from master, independent of it).
🤖 Generated with Claude Code
https://claude.ai/code/session_016i9S7LQsCL3FGaih3ZTRBJ