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>
12 KiB
name, description, user-invocable, allowed-tools
| name | description | user-invocable | allowed-tools |
|---|---|---|---|
| pr-push | Push commits and create or update a pull request. Use when the user says "push pr", "push and create pr", "update pr", "create a pr", "open a pr", or invokes /pr-push. NOT triggered by plain "push" (that's just git push). Pushes the current branch, creates a PR if none exists, or confirms the existing PR was updated. NEVER merges the PR into main — this skill only pushes to the branch and manages the PR lifecycle. | true | Bash, Read, Grep, Glob, AskUserQuestion, Skill |
Push PR Skill
Push commits to remote and create or update a PR. Operates exclusively on the current branch — never touches main.
Safety Rules (NON-NEGOTIABLE)
- NEVER merge a PR into main. No
tea pr merge, nogit mergeinto main. - NEVER checkout or push to main.
- NEVER force-push unless the user explicitly requests it.
- NEVER use
--no-verifyor skip hooks. - Only push to the current working branch.
Workflow
0. Dry-run mode check
If the user invokes /pr-push --dry-run:
- Print: "Dry-run mode — inspecting state, nothing will be pushed or committed."
- Run steps 1 through 4a in inspect-only mode:
- Step 4: run
make check-systems-dbto check current stamp freshness (no merge) - Step 4a: report which watched files changed vs origin/main; show whether
make regen-dbwould be triggered; do NOT run the regen, stage, or commit
- Step 4: run
- Print a summary: watched files changed (list), regen needed (yes/no), DB stamp fresh (yes/no)
- Print "Dry run complete — use /pr-push to apply."
- Stop. Do not push or create a PR.
1. Validate branch
git branch --show-current
If on main, stop: "You're on main. Switch to a team branch first."
1a. Orphan process check (MANDATORY)
Stale Godot processes from prior test runs compete with fresh runs for CPU and can silently wedge test-runner invocations. Before any test-invoking step (1b, 1c), check for long-lived Godot processes from prior stuck test runs:
# List any godot/gdunit processes running longer than 5 minutes
ps -eo pid,etimes,cmd | awk '$2 > 300 && /godot.*gdunit4-run/ {print $1, $2"s", substr($0, index($0,$3))}'
If any are listed: they are almost certainly orphans from a prior test
run that hung. Ask the user before killing — they may be intentional.
Default: offer to kill <PIDs> and wait a few seconds for the processes
to exit before proceeding. Re-run the check until empty.
Do not proceed to 1b/1c with orphan Godot processes alive — they will steal CPU from the fresh runs and may cause the new invocation to hang indefinitely (Sprint 36 lost an hour of test verification to this exact failure mode).
1b. Zero warnings policy (MANDATORY)
Before pushing, verify the branch has zero lint warnings. Any warning must be either fixed or suppressed with a commented justification.
For client/visual branches:
gdlint client/scripts/ client/ui/ 2>&1
If warnings remain, fix them before pushing. For warnings that cannot be
fixed (e.g. intentional long lines in data literals), add a # gdlint: ignore comment with a reason.
For server branches:
cargo clippy -- -D warnings 2>&1
For CI/tooling branches:
ruff check tooling/ 2>&1
The goal is zero warnings in the pre-push output. Advisory warnings that the pre-push hook reports as "(advisory, not blocking)" should still be zero — they are advisory only because we haven't enforced them yet.
1c. Runtime smoke test (MANDATORY)
Before pushing, verify the game actually runs. This is non-negotiable — Sprint 28 proved that code review without runtime testing misses critical bugs (parse errors, depth sorting, scene tree failures).
For client/visual branches:
First, wipe the script class cache before parsing. Sprint 36 close
caught this: the team added a new class_name MetaScreen base class and
six scripts extending it. Warm cache on developer machines parsed fine,
but CI / fresh clones / post-merge parses hit Could not find base class "MetaScreen" because the autoload-vs-class_name registration order only
resolves correctly once the class cache is seeded. Wiping the cache here
(client-side, before push) simulates the cold-start path and catches the
bug locally — keeping the pre-push hook fast.
# Cold-cache parse check. Deleting the cached class registry forces
# Godot to rebuild it from source on the next parse, matching the
# cold-start ordering CI and fresh clones see.
rm -f client/.godot/global_script_class_cache.cfg
# Headless parse + scanner check. Godot's resource scanner emits
# category errors (e.g. "Export type can only be built-in, a resource,
# a node, or an enum" for @export on a RefCounted) that do NOT always
# prefix with SCRIPT ERROR — they appear as plain ERROR lines. Widen
# the grep to catch both, then filter known pre-existing noise from
# the autoload class_name parse-order trap (documented in CLAUDE.md).
godot --headless --path client --quit 2>&1 | \
grep -iE "^(SCRIPT )?ERROR|Parse Error|Export type" | \
grep -v "Failed loading resource: res://assets" | \
grep -v "Cannot infer the type" | \
grep -vE "(Messagepack|LocalBridge|ServerProcess|Constants)\" not declared"
# If the branch has UI changes, also run the game briefly:
timeout 10 godot --path client res://scenes/main_menu.tscn 2>&1 | \
grep -iE "^(SCRIPT )?ERROR|Parse Error|Export type"
If the cold parse reports a "Could not find base class X" error, the fix
is almost always an autoload-order issue (see CLAUDE.md → GDScript
conventions → Autoload parse-order rule). Rebuilding the cache with
godot --editor --headless --quit will mask it locally but the same error
will re-surface post-merge — fix the actual ordering problem, don't paper
over it with a cache rebuild.
Any lines that come through the filter represent new errors introduced
by this branch. Fix them before pushing — Sprint 36 shipped commit
84105916 with an @export var descriptor: CharacterVisualDescriptor
scanner error that the old narrower grep missed; Tyre caught it five
commits later during W6 review.
For server branches:
cd server && cargo test --lib 2>&1
If any errors are found, stop and fix them before pushing. Do not push broken code for reviewers to find — that wastes everyone's time.
If the branch includes visual changes (character creation, UI, rendering),
the team should have manually launched the game and verified the change
works on screen before invoking /pr-push. If they haven't, ask:
"Have you run make game and verified this works visually?"
2. Commit uncommitted changes
git status
git diff --stat
Run both commands from the repo root (git rev-parse --show-toplevel).
Running from a subdirectory can cause paths to not resolve, hiding real
changes — Sprint 30 proved this when git diff HEAD -- server/src/bin/atlas.rs
returned 0 lines from the wrong CWD, masking uncommitted agent work.
CRITICAL: Do not trust "already done" claims without checking git state.
If agents report that work was "already implemented in a prior commit," verify
by checking git status and git diff --stat first. Grepping source files
only proves the code exists on disk — it does NOT prove the code is committed.
Uncommitted working-tree changes look identical to committed code when you
read files. Only git status distinguishes "already shipped" from "just
written by a teammate."
If there are uncommitted changes (staged or unstaged), run the commit skill
first. Use the /git-commit skill to group changes into logical commits with
proper conventional commit messages. Wait for commit to complete before
proceeding.
If the working tree is clean (no uncommitted changes), skip to step 3.
3. Check for unpushed commits
git fetch --all
git log --oneline origin/<branch>..<branch>
If no unpushed commits, skip to step 5 (PR check).
4. Check for conflicts with main
git merge-tree --write-tree origin/main HEAD 2>&1
If conflicts reported, merge main into current branch:
git merge origin/main --no-edit
If merge conflicts, stop and report — let the user resolve. If clean, continue.
4a. Regen systems.db if generator sources or data changed (#858)
Check whether any file in the source-file watch list was modified on this branch
versus origin/main. This list covers generator code AND the data files that feed them.
The generator-source paths below must stay in sync with GENERATOR_SOURCES in
tooling/check-systems-db-stamp (PR #136 review T7) — if you add a new source file
to the stamp, add it here too, and vice versa. Drift between the two lists reintroduces
exactly the silent-stale-DB class of bug this skill exists to prevent.
git diff --name-only origin/main...HEAD -- \
tooling/economy-db/import_economics.py \
tooling/planet-gen/generate_atlas.py \
server/src/bin/generate_brands/main.rs \
server/src/bin/generate_brands/names.rs \
tooling/generate-brands \
server/data/systems-schema.sql \
wiki/star-systems/ \
wiki/economics/ \
content/economics/
If output is empty: skip this step entirely.
If any files appear in the output: the DB must be regenerated on top of the current main. Perform the following:
-
Integrate main. Step 4 merged main into the branch. If you find yourself on a branch that was NOT yet merged with main in step 4, do it now:
git fetch origin git merge origin/main --no-editIf there are merge conflicts in source files, stop and report which files conflict. Ask the user to resolve manually — do not attempt to auto-resolve generator source conflicts.
-
Regenerate the DB:
make regen-dbmake regen-dbruns all three generators and stamps the meta table. It tolerates coverage gate failures (exit 2 = data quality warning, not an error). If it exits with any other non-zero code, stop and report the stderr output — do not push. -
Stage the updated DB:
git add server/data/systems.db -
Commit only if the DB actually changed:
git diff --cached --stat -- server/data/systems.db- If the diff shows changes: commit with
/git-commit, message:chore(db): regen systems.db against rebased sources - If no diff (regen produced identical output — sources were self-consistent):
unstage the file (
git restore --staged server/data/systems.db) and skip the commit. The source changes alone are the PR content.
- If the diff shows changes: commit with
In dry-run mode (from step 0): report which watch-list files changed and whether regen would be triggered. Do NOT run the regen or modify any files.
This step prevents the pre-push hook from rejecting a push where the branch modifies a generator source or data file but did not regenerate the DB.
5. Push
git push origin <branch>
If push fails, stop and report. Never force-push without explicit request.
6. Check for existing PR
tea pr list --login schweitz --repo jpmschweitzer/settled-reach --state open --output simple
Match current branch name in PR list.
- PR exists: Report "Pushed N commits to
<branch>. PR #X updated." Done. - No PR: Continue to step 7.
7. Create a new PR
git log --oneline main..<branch>
git diff --stat main...<branch>
Draft title (<type>(<scope>): <summary>, max 70 chars) and description.
tea pr create \
--repo jpmschweitzer/settled-reach \
--login schweitz \
--title "<title>" \
--description "## Summary ..." \
--base main \
--head <branch>
Report PR URL when done.
8. Update ticket status to review
Scan all commit messages in the PR for ticket references (#NNN):
git log --oneline main..<branch>
Extract ticket IDs from #NNN patterns. For each ticket that is
currently in_progress, update it to review:
tooling/db/ticket status <id> review
Report which tickets were moved to review. Skip tickets that are
already done, review, cancelled, or backlog (only transition
in_progress → review).
9. Next steps
If a sprint team is active (you are the team lead), do NOT shut down agents after pushing. The team should remain alive for PR review and potential comment fixes.
Suggest: "PR created/updated. Run /pr-review to review before merge."
Arguments
If the user passes arguments (e.g., /pr-push "my title"), use them as the
PR title instead of generating one.