chore(ci): merge brand pipeline into import_economics + harden review findings
Addresses all blocking + minor items from PR #136 review. Architectural change (T2/H3 — the review's main complaint): generate_brands was previously a separate Rust binary that produced a TOML artifact, with its stamp written "on behalf" by import_economics.py at the end of its own run. Reviewers flagged the invisible coupling: two sources of truth in a system designed to have one, and no way to tell from the stamp that one "generator" was really a subroutine of the other. import_economics now invokes tooling/generate-brands as the first step of its main() flow, before opening its own DB connection. The TOML artefact is still produced and still committed (useful for diff-review of brand changes), but there's now one pipeline owner. The meta table carries two rows (import_economics, generate_atlas) not three; the Rust binary's source SHA folds into import_economics' stamp via IMPORT_ECONOMICS_SOURCES. A MIGRATION_SQL DELETE cleans up pre-merge DBs that still have the orphan generate_brands row. Other review items addressed in-line: H1 generate_atlas._write_stamp no longer commits — transaction ownership stays with the caller (matches import_economics pattern). Stamp + atlas data now commit atomically; a failed stamp rolls back the atlas data rather than leaving a stamp-missing-data intermediate. H2 _file_sha1 (in both import_economics, generate_atlas, check-systems-db-stamp) raises FileNotFoundError on missing sources instead of silently contributing an empty-bytes hash. A ghost-SHA convergence could otherwise produce vacuous "fresh" passes. H4 pre-push no-meta-table warning rephrased — was "run after next regeneration", now "run now if this DB was generated by you". T1 asset-pipeline.md determinism claim softened: the stamp is deterministic (same source → same recorded SHA), the DB binary is not (generated_at + SQLite rowids/freelist churn). T3 asset-pipeline.md gains a "migration escape hatch" section naming MIGRATION_SQL in import_economics.py as the only sanctioned path for direct writes, and forbidding hand-run sqlite-exec / one-off patch scripts / SQLite-GUI edits. T4 Makefile regen-db now runs as a single shell with `set -e`. A failure in one generator halts the pipeline immediately, preventing the "stale data, fresh stamp" state where a later step stamped a DB whose earlier step had failed. import_economics' exit code 2 (coverage gate warning) remains explicitly tolerated. T5 pre-push stamp check now runs on a branch's first push too — compares against origin/main instead of origin/$BRANCH, closing the gap where a new branch could ship a stale DB via the first push. T6 check-systems-db-stamp fails closed on unknown generator_names in meta — a future branch adding a new generator without registering it in GENERATOR_SOURCES will now be rejected, not silently skipped. T7 /pr-push watch list gains a mutual cross-reference comment with GENERATOR_SOURCES in check-systems-db-stamp, plus the missing names.rs source file, so the two lists cannot silently drift. Follow-up tickets created: #887 T8 decisions-orphan-tickets CLI — surfaces tickets whose decision_ref points at a non-existent D-record. #888 T9 meta.schema_version monotonic semver — for savegame migration lineage in Phase 5+ (SHA comparison can't be ordered). Verified: make regen-db end-to-end — OK make check-systems-db — OK, 2 generator(s) up to date STALE detection — OK, verified by touching generate_atlas.py /pr-push watch list — OK, flags this branch's changed sources decision show D-159 — OK, structured output with tickets + refs Refs: #855 #856 #857 #858 #859 PR #136 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.6
@@ -7,7 +7,7 @@ generator's source file(s) matches the current file content on disk.
|
||||
|
||||
Exit codes:
|
||||
0 — DB is stamped and all generator SHAs match current sources
|
||||
1 — DB is stale (one or more generators have changed since last regen)
|
||||
1 — DB is stale, has an unknown generator, or references a missing source file
|
||||
2 — DB does not have a meta table (treat as unstamped — run make regen-db)
|
||||
|
||||
Usage (called by .config/hooks/pre-push):
|
||||
@@ -31,12 +31,16 @@ DB_PATH = REPO_ROOT / "server" / "data" / "systems.db"
|
||||
# Maps generator_name (as stored in meta.generator_name) to the source
|
||||
# file(s) whose SHA is stamped. The SHA is computed as SHA-1 of the
|
||||
# concatenated bytes of all files in sorted order.
|
||||
#
|
||||
# import_economics' source set includes the Rust generate_brands binary it now
|
||||
# invokes as a subroutine (#136 review T2/H3). Keep this list in sync with
|
||||
# IMPORT_ECONOMICS_SOURCES in tooling/economy-db/import_economics.py.
|
||||
GENERATOR_SOURCES: dict[str, list[Path]] = {
|
||||
"import_economics": [
|
||||
REPO_ROOT / "tooling" / "economy-db" / "import_economics.py",
|
||||
],
|
||||
"generate_brands": [
|
||||
REPO_ROOT / "server" / "src" / "bin" / "generate_brands" / "main.rs",
|
||||
REPO_ROOT / "server" / "src" / "bin" / "generate_brands" / "names.rs",
|
||||
REPO_ROOT / "tooling" / "generate-brands",
|
||||
],
|
||||
"generate_atlas": [
|
||||
REPO_ROOT / "tooling" / "planet-gen" / "generate_atlas.py",
|
||||
@@ -45,11 +49,17 @@ GENERATOR_SOURCES: dict[str, list[Path]] = {
|
||||
|
||||
|
||||
def file_sha1(*paths: Path) -> str:
|
||||
"""SHA-1 of concatenated file contents (sorted paths, missing files skipped)."""
|
||||
"""SHA-1 of concatenated file contents (sorted paths).
|
||||
|
||||
Missing files raise FileNotFoundError rather than silently contributing
|
||||
an empty-string hash (H2): a ghost SHA could mask real breakage when
|
||||
stored and current SHAs converge on the empty-bytes digest.
|
||||
"""
|
||||
h = hashlib.sha1()
|
||||
for p in sorted(paths):
|
||||
if p.exists():
|
||||
h.update(p.read_bytes())
|
||||
if not p.exists():
|
||||
raise FileNotFoundError(f"generator source not found: {p}")
|
||||
h.update(p.read_bytes())
|
||||
return h.hexdigest()
|
||||
|
||||
|
||||
@@ -80,12 +90,25 @@ def check(verbose: bool = False) -> int:
|
||||
return 2
|
||||
|
||||
stale: list[str] = []
|
||||
unknown: list[str] = []
|
||||
for generator_name, stored_sha in rows:
|
||||
sources = GENERATOR_SOURCES.get(generator_name)
|
||||
if sources is None:
|
||||
# Unknown generator — skip (forward compat)
|
||||
# Unknown generator — fail closed (T6). A future branch adding a
|
||||
# new generator without registering it here must update this map
|
||||
# before the check will pass, preventing the "silent no-op" trap.
|
||||
unknown.append(generator_name)
|
||||
continue
|
||||
current_sha = file_sha1(*sources)
|
||||
try:
|
||||
current_sha = file_sha1(*sources)
|
||||
except FileNotFoundError as exc:
|
||||
# Source file moved/deleted — explicit failure instead of
|
||||
# silent empty-hash (H2).
|
||||
print(
|
||||
f"check-systems-db-stamp: BROKEN — {generator_name}: {exc}",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return 1
|
||||
if current_sha != stored_sha:
|
||||
stale.append(generator_name)
|
||||
if verbose:
|
||||
@@ -95,6 +118,19 @@ def check(verbose: bool = False) -> int:
|
||||
f"\n current: {current_sha}"
|
||||
)
|
||||
|
||||
if unknown:
|
||||
print(
|
||||
"check-systems-db-stamp: UNKNOWN generator(s) in meta table: "
|
||||
f"{unknown}",
|
||||
file=sys.stderr,
|
||||
)
|
||||
print(
|
||||
" Update GENERATOR_SOURCES in tooling/check-systems-db-stamp to "
|
||||
"register them before pushing.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return 1
|
||||
|
||||
if stale:
|
||||
if not verbose:
|
||||
print(
|
||||
|
||||
@@ -42,33 +42,54 @@ SCHEMA_SQL = REPO_ROOT / "server" / "data" / "systems-schema.sql"
|
||||
CORPORATIONS_DIR = REPO_ROOT / "wiki" / "corporations"
|
||||
BRANDS_TOML = REPO_ROOT / "wiki" / "economics" / "corporations" / "brands.toml"
|
||||
GENERATED_BRANDS_TOML = REPO_ROOT / "wiki" / "economics" / "corporations" / "generated_brands.toml"
|
||||
# Source file for the generate_brands Rust binary — stamped on behalf (#855)
|
||||
GENERATE_BRANDS_SRC = REPO_ROOT / "server" / "src" / "bin" / "generate_brands" / "main.rs"
|
||||
# Rust sources for the generate_brands subroutine. import_economics shells out to
|
||||
# tooling/generate-brands as part of its normal flow (see regenerate_brands()), so
|
||||
# both Rust files contribute to this script's effective source SHA: any change to
|
||||
# either must invalidate the meta stamp even though Python hasn't changed.
|
||||
GENERATE_BRANDS_RS = REPO_ROOT / "server" / "src" / "bin" / "generate_brands" / "main.rs"
|
||||
GENERATE_BRANDS_NAMES_RS = REPO_ROOT / "server" / "src" / "bin" / "generate_brands" / "names.rs"
|
||||
GENERATE_BRANDS_WRAPPER = REPO_ROOT / "tooling" / "generate-brands"
|
||||
|
||||
|
||||
def _file_sha1(*paths: Path) -> str:
|
||||
"""Return SHA-1 hex of the concatenated content of one or more files.
|
||||
|
||||
Files are sorted by path for determinism. Missing files are silently
|
||||
skipped so a fresh worktree that hasn't built the Rust binary yet
|
||||
doesn't fail to stamp.
|
||||
Files are sorted by path for determinism. Missing files raise FileNotFoundError
|
||||
rather than silently contributing an empty-string hash — a ghost SHA masks real
|
||||
breakage (review comment H2: da39a3ee… convergence could produce vacuous passes).
|
||||
"""
|
||||
h = hashlib.sha1()
|
||||
for p in sorted(paths):
|
||||
if p.exists():
|
||||
h.update(p.read_bytes())
|
||||
if not p.exists():
|
||||
raise FileNotFoundError(f"generator source not found: {p}")
|
||||
h.update(p.read_bytes())
|
||||
return h.hexdigest()
|
||||
|
||||
|
||||
# Canonical source set for import_economics' meta stamp. Covers its own .py file
|
||||
# plus the Rust binary it invokes (generate_brands main.rs + names.rs + wrapper
|
||||
# script) so any change to the brand generation pipeline flips the stamp. Keep
|
||||
# this list in sync with GENERATOR_SOURCES["import_economics"] in
|
||||
# tooling/check-systems-db-stamp.
|
||||
IMPORT_ECONOMICS_SOURCES: tuple[Path, ...] = (
|
||||
Path(__file__),
|
||||
GENERATE_BRANDS_RS,
|
||||
GENERATE_BRANDS_NAMES_RS,
|
||||
GENERATE_BRANDS_WRAPPER,
|
||||
)
|
||||
|
||||
|
||||
def _write_stamp(conn: sqlite3.Connection, generator_name: str, *source_files: Path) -> None:
|
||||
"""Upsert a row in the meta table recording this generator's current source SHA.
|
||||
|
||||
Called after every successful non-dry-run commit. Idempotent: running
|
||||
twice on the same sources writes the same sha with an updated timestamp.
|
||||
|
||||
Generators stamped here:
|
||||
import_economics — this file
|
||||
generate_brands — server/src/bin/generate_brands/main.rs (stamped on behalf)
|
||||
Only one stamp is written by this module: ``import_economics``, whose source
|
||||
set includes the Rust binary it invokes (see IMPORT_ECONOMICS_SOURCES).
|
||||
generate_atlas writes its own stamp. generate_brands does NOT write a stamp
|
||||
of its own — it's a subroutine of import_economics, not an independent DB
|
||||
writer (PR #136 review T2/H3).
|
||||
|
||||
The meta table is created by the MIGRATION_SQL block above; this
|
||||
function assumes it exists (caller must run migrations first).
|
||||
@@ -82,6 +103,42 @@ def _write_stamp(conn: sqlite3.Connection, generator_name: str, *source_files: P
|
||||
)
|
||||
|
||||
|
||||
def regenerate_brands() -> None:
|
||||
"""Run the Rust generate_brands binary to refresh generated_brands.toml.
|
||||
|
||||
Invoked as the first step of import_economics' main flow so the TOML on disk
|
||||
always matches the current Rust source before the Python import reads it.
|
||||
This replaces the former split (tooling/generate-brands run separately by
|
||||
make regen-db) with a single, coherent brand pipeline owned by one stamp.
|
||||
|
||||
The wrapper script builds the binary on demand and runs it with the default
|
||||
canonical seed=1; callers that need non-canonical seeds must still invoke
|
||||
the wrapper directly (experimentation only — committed output must be seed=1).
|
||||
"""
|
||||
import subprocess
|
||||
|
||||
if not GENERATE_BRANDS_WRAPPER.exists():
|
||||
raise FileNotFoundError(
|
||||
f"generate_brands wrapper not found at {GENERATE_BRANDS_WRAPPER}"
|
||||
)
|
||||
print(" [pre/10] Running generate_brands (Rust) to refresh generated_brands.toml...")
|
||||
result = subprocess.run(
|
||||
[str(GENERATE_BRANDS_WRAPPER)],
|
||||
cwd=str(REPO_ROOT),
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
print(result.stdout, file=sys.stderr)
|
||||
print(result.stderr, file=sys.stderr)
|
||||
raise _ImportAborted()
|
||||
# Print the Rust binary's own summary lines (brands generated, coverage).
|
||||
# Indent so they fold under the pre-step heading.
|
||||
for line in result.stdout.splitlines():
|
||||
if line.strip():
|
||||
print(f" {line}")
|
||||
|
||||
|
||||
class _ImportAborted(Exception):
|
||||
"""Raised internally by main() when a validation step wants a clean
|
||||
rollback + exit 1. Caught only by main(); error messages are printed
|
||||
@@ -217,6 +274,13 @@ CREATE TABLE IF NOT EXISTS meta (
|
||||
generator_sha TEXT NOT NULL,
|
||||
generated_at TEXT NOT NULL DEFAULT (datetime('now'))
|
||||
);
|
||||
|
||||
-- Drop the pre-merge 'generate_brands' stamp row if it exists (PR #136 review T2/H3).
|
||||
-- The Rust brand binary is now a subroutine of import_economics — its source
|
||||
-- SHA contributes to the 'import_economics' stamp — so it no longer merits its
|
||||
-- own meta row. This DELETE makes the check-systems-db-stamp "unknown generator"
|
||||
-- path (fail-closed per T6) compatible with older DBs that still have the row.
|
||||
DELETE FROM meta WHERE generator_name = 'generate_brands';
|
||||
"""
|
||||
|
||||
# Columns to add to existing tables (ALTER TABLE is idempotent via try/except)
|
||||
@@ -1057,6 +1121,18 @@ def main():
|
||||
wiki_corps = load_wiki_corps()
|
||||
print(f" {len(wiki_corps)} corporation files parsed")
|
||||
|
||||
# Regenerate generated_brands.toml via the Rust binary before the Python
|
||||
# import reads it. Single pipeline, single stamp — resolves review T2/H3
|
||||
# ("on-behalf stamping" coupling) by folding brand generation into
|
||||
# import_economics' flow rather than having the caller (Makefile / user)
|
||||
# remember to run it first. Skipped on --dry-run to avoid a disk
|
||||
# side-effect during validation.
|
||||
if not args.dry_run:
|
||||
try:
|
||||
regenerate_brands()
|
||||
except _ImportAborted:
|
||||
sys.exit(1)
|
||||
|
||||
conn = sqlite3.connect(str(db_path))
|
||||
conn.execute("PRAGMA foreign_keys=ON")
|
||||
|
||||
@@ -1190,10 +1266,9 @@ def main():
|
||||
# issues and must not prevent the stamp from landing.
|
||||
if not args.dry_run:
|
||||
try:
|
||||
_write_stamp(conn, "import_economics", Path(__file__))
|
||||
_write_stamp(conn, "generate_brands", GENERATE_BRANDS_SRC)
|
||||
_write_stamp(conn, "import_economics", *IMPORT_ECONOMICS_SOURCES)
|
||||
conn.commit()
|
||||
print(" Stamped: import_economics, generate_brands")
|
||||
print(" Stamped: import_economics (covers brand pipeline Rust sources)")
|
||||
except Exception as exc: # noqa: BLE001
|
||||
print(f" WARNING: failed to write generator stamp: {exc}", file=sys.stderr)
|
||||
|
||||
|
||||
@@ -95,13 +95,16 @@ _ATLAS_SCHEMA_END_MARKER = "-- END ATLAS INDEX"
|
||||
def _file_sha1(*paths: Path) -> str:
|
||||
"""Return SHA-1 hex of the concatenated content of one or more files.
|
||||
|
||||
Files are sorted by path for determinism. Missing files are silently
|
||||
skipped — a fresh worktree lacking optional build artefacts still stamps.
|
||||
Files are sorted by path for determinism. Missing files raise
|
||||
FileNotFoundError rather than silently skip — a ghost hash (empty-bytes
|
||||
digest) can mask real breakage when stored and current SHAs converge
|
||||
(#136 review H2).
|
||||
"""
|
||||
h = hashlib.sha1()
|
||||
for p in sorted(paths):
|
||||
if p.exists():
|
||||
h.update(p.read_bytes())
|
||||
if not p.exists():
|
||||
raise FileNotFoundError(f"generator source not found: {p}")
|
||||
h.update(p.read_bytes())
|
||||
return h.hexdigest()
|
||||
|
||||
|
||||
@@ -112,6 +115,10 @@ def _write_stamp(conn: sqlite3.Connection) -> None:
|
||||
with an updated timestamp. The meta table is created by the atlas schema
|
||||
migration executed in ensure_atlas_schema(); this function assumes it
|
||||
exists.
|
||||
|
||||
Transaction ownership stays with the caller (matches the import_economics
|
||||
pattern) — no inner commit here. Review H1 flagged the prior behaviour as
|
||||
a double-commit with the atlas data write that precedes it.
|
||||
"""
|
||||
schema_sha = _file_sha1(SYSTEMS_SCHEMA_PATH)
|
||||
generator_sha = _file_sha1(Path(__file__))
|
||||
@@ -121,7 +128,6 @@ def _write_stamp(conn: sqlite3.Connection) -> None:
|
||||
VALUES ('generate_atlas', ?, ?, datetime('now'))""",
|
||||
(schema_sha, generator_sha),
|
||||
)
|
||||
conn.commit()
|
||||
|
||||
|
||||
def _load_atlas_schema() -> str:
|
||||
@@ -1381,14 +1387,19 @@ def main():
|
||||
print(f" [{i+1}/{len(bodies)}] {body_id:20s} ERROR: {result.get('message', '')}")
|
||||
|
||||
if not args.dry_run:
|
||||
conn.commit()
|
||||
# Stamp generator metadata (#855, #856) after a successful run.
|
||||
# Failure is non-fatal (metadata only) but reported.
|
||||
# Stamp generator metadata (#855, #856) together with the atlas data
|
||||
# in a single commit — atlas data + stamp land atomically, and the
|
||||
# stamp function itself no longer commits (H1). Failure of the stamp
|
||||
# write rolls back the atlas data too rather than leaving a stamped-
|
||||
# but-missing-data intermediate state.
|
||||
try:
|
||||
_write_stamp(conn)
|
||||
conn.commit()
|
||||
print(" Stamped: generate_atlas")
|
||||
except Exception as exc: # noqa: BLE001
|
||||
conn.rollback()
|
||||
print(f" WARNING: failed to write generator stamp: {exc}", file=sys.stderr)
|
||||
print(" Atlas data NOT committed — regen required.", file=sys.stderr)
|
||||
conn.close()
|
||||
|
||||
elapsed_total = time.time() - t_total
|
||||
|
||||
Reference in New Issue
Block a user