ci: sprint 37 sweep — asset pipeline discipline + decision coverage #136
Assignees
jpmschweitzer (Jeroen Schweitzer)
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: jpmschweitzer/settled-reach#136
Reference in New Issue
Block a user
Summary
Sprint 37 CI sweep. Seven tickets, all closed:
metatable stamping (schema_version + generator_sha + generated_at)tooling/check-systems-db-stamp/pr-pushauto-runsmake regen-dbwhen generator sources changed.claude/rules/asset-pipeline.md, CLAUDE.md hook, DEVOPS.md sectiontooling/db/decision show <D-NNN>with implementing tickets + cross-refs;decisions-coveragenow lists ticket IDs per decisionMotivation
Sprint 36 hit a binary merge conflict class when two branches both committed changes to
server/data/systems.db. The meta stamp + pre-push hook +/pr-pushregen form a systemic fix: the canonical rule is "edit sources, not the DB" and the tooling now enforces it.Verification
make regen-dbran successfully; three generators stamped inmetatablemake check-systems-dbexit 0 —check-systems-db-stamp: OK — 3 generator(s) up to dateruff check tooling/— All checks passeddonein the DBTest plan
make regen-dbruns end-to-end on a clean clonemake check-systems-dbexits 0 after a fresh regengit pushwith a modified generator but stale DB — hook rejects/pr-pushon a branch with modifiedtooling/economy-db/import_economics.pyauto-regenerates before pushingtooling/db/decision show D-159returns implementing tickets + refs🤖 Generated with Claude Code
decision show <D-NNN>with implementing tickets + refs a41f7aa06cReview: sprint-37/ci → main (PR #136)
Reviewers: Hoshe (code quality & tests) + Tyre (architecture).
Verdict: CHANGES REQUESTED
Source-canonical rule is a solid design — SHA-of-sources rather than DB-content hash sidesteps SQLite non-determinism cleanly, pre-push hook is the correct enforcement layer,
/pr-pushauto-regen is appropriately scoped. But 5 blocking items + several minor/suggestion items need addressing before merge.Hoshe — Code Quality & Tests: REQUEST_CHANGES
Exit codes are correct, error messages actionable, schema coherent,
show_decisionhandles zero-ticket cases cleanly, pre-push hook integrates without disrupting existing checks. Two real bugs + two nits.tooling/planet-gen/generate_atlas.py:~1383-1391conn.commit()at 1384 persists atlas data, then_write_stamp(conn)issues its ownconn.commit()at line 124. Inconsistent withimport_economics._write_stampwhich leaves transaction ownership to caller. Either remove the inner commit (match the economics pattern — preferred) or remove the outer commit.tooling/check-systems-db-stamp:~48-54(file_sha1)if p.exists(): h.update(...)). If a generator source file is moved/deleted, validator computes SHA-1 of zero bytes (alwaysda39a3ee...). The stored SHA won't match → push correctly rejected, but the error message says "STALE" with no hint that the file is missing. Edge case: if both stored and current SHA converge on the empty hash, check passes vacuously. Add explicit "source file not found: " error with exit 1.tooling/check-systems-db-stamp:~32-37(GENERATOR_SOURCES)generate_brandssource is listed but no stamp writer in the Rust binary — it's stamped on behalf byimport_economics._write_stamp. Pre-existing design constraint, not a regression, but should be called out in the meta table schema comment or aTODOso the next engineer doesn't add an independent stamp path that races..config/hooks/pre-push(stamp section, last ~30 lines)rc=2(no meta table) is advisory (intended grace for new DBs). But the warning message "runmake regen-dbafter next regeneration" reads like a future action. Suggest: "systems.db has no meta stamp — runmake regen-dbnow if this DB was generated by you."Tyre — Architecture: REQUEST_CHANGES
The core design is elegant where it matters. SHA-of-sources (not hash-of-DB) is the right choice because it sidesteps SQLite's non-determinism (timestamps, autoincs, rowids). Pre-push is the correct enforcement layer.
/pr-pushauto-regen is scoped tight to generator-touching branches. But three cracks need patching, plus one Makefile failure-handling bug..claude/rules/asset-pipeline.md:~95("Why direct DB edits are forbidden")meta.generated_atusesdatetime('now'), SQLite rowids/autoinc aren't stable across runs. The stamp is deterministic, not the DB binary. The rule's strength doesn't depend on byte-determinism — so soften the claim to "the stamp is deterministic (same generator source → same recorded SHA)" and remove the byte-for-byte assertion. Otherwise reviewers expect binary-identical DBs after regen and are confused when diffs appear.tooling/economy-db/import_economics.py:1194+tooling/check-systems-db-stamp:37-43generate_brandsstamped on behalf byimport_economics, not by its own Rust binary. Undocumented coupling in the rule's "Three generators write to systems.db" table. Two real failure modes: (a) someone runstooling/generate-brandsdirectly (the rule's documented invocation) withoutimport_economicsafter it → SHA drifts from stamp; (b) brands source changes, economics doesn't → any workflow invoking only the Rust binary silently skips stamping. Fix: either make the Rust binary stamp itself (~30 LOC withrusqlite), or document the coupling explicitly and remove "on behalf" as an invisible convention. Hoshe also flagged this (H3)..claude/rules/asset-pipeline.md(whole doc)MIGRATION_SQLinimport_economics.pyis the de-facto location — the rule should state that explicitly and forbid any other form of direct DB edit (no hand-runsqlite-exec, no one-off patches). Otherwise the rule has a loophole an operator can drive a truck through.Makefile:357-366(regen-dbtarget)generate_brands(step 1) fails,import_economicsstill runs and stamps → DB with stale brand data and a "fresh" stamp. Addset -esemantics or|| exit 1after each step, or use a.SHELLFLAGSguard..config/hooks/pre-push:~155check-systems-db-stampunconditionally whenserver/data/systems.dbwas modified in ANY branch commit, regardless of remote-ref state. Alternatively, document the gap.tooling/check-systems-db-stamp:61-65generator_namein meta silently skipped ("forward compat"). Wrong default for a safety gate — a future branch adding a new generator without registering it here becomes a no-op check, silently. Warn loudly on unknown names, or require explicit allowlist bump. Fail closed..claude/skills/pr-push/SKILL.mdstep 4atooling/economy-db/import_economics.py,tooling/planet-gen/generate_atlas.py,server/src/bin/generate_brands/main.rs, etc.) plus content dirs. This will drift fromGENERATOR_SOURCESincheck-systems-db-stamp. Two sources of truth in a system whose purpose is source-canonical discipline is exactly what this PR is trying to prevent. Shell out to a registry script, or at minimum add mutual cross-reference comments.tooling/db/decisions_sync.py:417-457(show_decision)tickets.decision_refis the right direction (no two-way sync). One note:decision_refis free-text without a foreign key, so typo'd or deleted D-IDs silently don't show up anywhere. Not blocking; worth a follow-updecisions-orphan-ticketsCLI to surface mismatches.server/data/systems-schema.sql(meta table)schema_version TEXTuses SHA-1 of the schema file. The rule doc names "savegame migration lineage" as a future use. When that arrives, a content-hash is harder to order than a monotonic version number — you cannot tell from two SHAs which is newer. Don't change now; flag for the migration phase:v1.0-style semver would serve lineage better.Process note
PR test plan has 5 unchecked boxes:
make regen-dbruns end-to-end on a clean clonemake check-systems-dbexits 0 after a fresh regengit pushwith modified generator but stale DB — hook rejects/pr-pushon a branch with modified generator auto-regenerates before pushingtooling/db/decision show D-159returns implementing tickets + refsThese are NOT merge-path per skill 0b-i (no pre-game/scene/save/connection code). No automatic verdict downgrade. But they ARE the pipeline's own smoke tests — the features this PR ships. Worth running before the next push so the verification is concrete, not aspirational.
Summary of blocking issues
5
error/major-severity fixes required:generate_atlas._write_stamp(inconsistent with economics pattern)generate_brandssilently stamped "on behalf" by economics (also H3). Either self-stamp or document the coupling explicitly.Makefile:regen-dbdoesn't halt on generator failure → can produce stale-data-fresh-stamp state.Other items are minor/suggestion; address before merge or push back with rationale.
Review: sprint-37/ci → main (PR #136) — Round 2
Reviewers: Hoshe (code quality & tests) + Tyre (architecture).
Verdict: APPROVED
3 → 0 blockers across 2 rounds. All round-1 items correctly addressed. The T2 pivot (merging the brand pipeline into
import_economicsinstead of documenting the stamp-on-behalf coupling) is architecture, not paper.Hoshe — Code Quality & Tests: APPROVE
All 4 round-1 items fixed. H2 error-handling path is consistent across
import_economics,generate_atlas, andcheck-systems-db-stamp. The H3 architectural pivot is clean.Round-1 status
generate_atlas._write_stampdouble-commitconn.commit()removed; caller wraps stamp + data intry/exceptwith rollback. Data and stamp land atomically or not at all_file_sha1now raisesFileNotFoundErrorwith formatted path.check()loop catches it and returns rc=1 with"BROKEN — {generator_name}: {exc}"on stderr. Not swallowed. Consistent across all three filesgenerate_brandsno longer stamps at all.import_economics.regenerate_brands()shells out to the Rust binary as step zero of itsmain(), checks return code, raises_ImportAbortedon non-zero exit (stderr printed before raising).IMPORT_ECONOMICS_SOURCESincludes the Rust.rsfiles + the wrapper script. MigrationDELETE FROM meta WHERE generator_name='generate_brands'cleans the orphan row on pre-PR DBs"pre-push: WARNING — systems.db has no meta stamp — run 'make regen-db' now if this DB was generated by you"New observation (non-blocking)
The stamp-write failure path in
import_economics(around line 1271) catchesException, prints a warning, and continues — but does not roll back the data commit (which has already landed). Intentional: the stamp is advisory, and a missing-meta state is caught bycheck-systems-db-stampas rc=2 (warn-not-block). That's a coherent deliberate choice. Worth a one-line comment "stamp failure is non-fatal; DB data is already committed" so the next reader doesn't wonder if the missing rollback is a bug. Not required before merge.Tyre — Architecture: APPROVE
The T2 pivot is real architecture, not documentation papering over a smell. Single pipeline owner with one stamp row per
metaentry; Rust source SHAs fold intoIMPORT_ECONOMICS_SOURCESso any change tomain.rs/names.rs/ the wrapper invalidates the stamp correctly. All 7 addressable items fixed; 2 suggestions deferred to #887/#888.Architectural deep-check on the pivot
import_economics.pyget too big? Grew from 1170 → 1306 lines. Growth isMIGRATION_SQLexpansion + a 30-lineregenerate_brands()subprocess wrapper + the DELETE-orphan migration. The Python file doesn't absorb brand-generation logic — it shells out. The Rust binary is untouched.tooling/generate-brandsworks standalone for debug/experimentation. Theregenerate_brands()docstring explicitly calls out the non-canonical-seed escape hatch ("callers that need non-canonical seeds must still invoke the wrapper directly — experimentation only"). The "must go through import_economics" rule is self-enforcing via the stamp: wrapper-only runs don't write a stamp, so the next stamp check catches the drift. That's the right level.DELETE FROM meta WHERE generator_name = 'generate_brands'runs on every import. Pull branch → regen → revert: orphan row stays gone because main'simport_economicswon't re-add it. Revert further to pre-PR state where the Makefile calledgenerate-brandsseparately and the binary wrote its own stamp — yes, the row comes back, but that's the correct pre-PR behavior. Clean.Round-1 status
generated_at, rowid drift, freelist reshufflingMIGRATION_SQL+COLUMN_MIGRATIONSas the sanctioned path and explicitly forbids hand-runsqlite-exec, GUI edits, one-off patch scripts, and committingsystems.dbaloneset -ein a single shell, `ec=0; ...origin/main(always exists) instead oforigin/$BRANCH. No false-positive risk: the stamp check validatesmeta.generator_shavs current source SHAs, so a DB-only commit with untouched generators passes cleanlygenerator_namereturns rc=1 with"Update GENERATOR_SOURCES in tooling/check-systems-db-stamp to register them before pushing"on stderrGENERATOR_SOURCEScomment says "Keep this list in sync with IMPORT_ECONOMICS_SOURCES"; the pr-push SKILL watch list cites T7 by name and lists all fourgenerate_brandssourcesNo new architectural concerns
Ship it.
Pre-merge action (not a reviewer issue)
PR test plan still has 5 unchecked boxes (pipeline smoke tests). The commit body reports manual verifications were done:
Worth ticking those boxes in the PR description before merge so the evidence is recorded where reviewers expect it, matching the Sprint 36 retro "evidence over aspiration" norm. Not a gate.
Summary
APPROVED across both reviewers. 14 round-1 items total (4 Hoshe + 9 Tyre + 1 cross-reference) → 0 blockers in round 2. The pivot on T2/H3 is the structural win — fixing the coupling by removing it rather than documenting it.
Pull request closed