From 32021bd550943ebdec1e9c8f2cfb67ec10be9f56 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Mon, 13 Jul 2026 12:16:34 +0200 Subject: [PATCH] =?UTF-8?q?chore(skills):=20workflow=20skills=20sweep=20?= =?UTF-8?q?=E2=80=94=20whats-next/workshop-start/pr-review/pr-process=20(T?= =?UTF-8?q?-1102)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit De-sprint pr-review, dynamic repo-root paths, gate-aligned checks; workshop-start Agent-tool rename + roster fixes (IMPROVEMENTS.md folded in and removed); whats-next pql-durability notes; pr-process orphan-check + full-suite alignment. New helper scripts tooling/godot-cold-parse + tooling/pr-watchlist-diff (allowlist entries deferred to first-use per permission policy). Part of T-1099. Co-Authored-By: Claude Fable 5 --- .claude/rules/asset-pipeline.md | 8 +- .claude/skills/pr-process/SKILL.md | 104 ++++++--------- .claude/skills/pr-review/SKILL.md | 123 ++++++++---------- .../pr-review/references/reviewer-profiles.md | 49 ++++--- .claude/skills/whats-next/SKILL.md | 22 ++++ .claude/skills/workshop-start/IMPROVEMENTS.md | 87 ------------- .claude/skills/workshop-start/SKILL.md | 27 ++-- tooling/godot-cold-parse | 52 ++++++++ tooling/pr-watchlist-diff | 28 ++++ 9 files changed, 239 insertions(+), 261 deletions(-) delete mode 100644 .claude/skills/workshop-start/IMPROVEMENTS.md create mode 100755 tooling/godot-cold-parse create mode 100755 tooling/pr-watchlist-diff diff --git a/.claude/rules/asset-pipeline.md b/.claude/rules/asset-pipeline.md index a1ef484d4..6745d2c4a 100644 --- a/.claude/rules/asset-pipeline.md +++ b/.claude/rules/asset-pipeline.md @@ -118,22 +118,22 @@ systems.db is stale — run `make regen-db` before pushing. ``` Fix: run `make regen-db`, stage `server/data/systems.db`, amend or add a commit. -Or use `/pr-push` — it detects stale generator sources and reruns `make regen-db` +Or use `/pr-process` — it detects stale generator sources and reruns `make regen-db` automatically before pushing. The check script is `tooling/check-systems-db-stamp`. Run it interactively with `make check-systems-db` or `python3 tooling/check-systems-db-stamp --verbose`. The `GENERATOR_SOURCES` dict in `tooling/generator_sources.py` is the single registry (T-1067) — the check script and the importer's stamp writer both import it, and -the `/pr-push` skill derives its source-file watch list from +the `/pr-process` skill derives its source-file watch list from `python3 tooling/generator_sources.py --list`. When you add a new generator or source file, register it there and nowhere else. --- -## /pr-push integration (T-858) +## /pr-process integration (T-858) -The `/pr-push` skill checks whether any generator source files are modified on the +The `/pr-process` skill checks whether any generator source files are modified on the branch. If they are, it automatically runs `make regen-db` and stages the updated `server/data/systems.db` before pushing — preventing pre-push hook rejections on branches that modify generators without regenerating. diff --git a/.claude/skills/pr-process/SKILL.md b/.claude/skills/pr-process/SKILL.md index 900534be9..a6e5fdc21 100644 --- a/.claude/skills/pr-process/SKILL.md +++ b/.claude/skills/pr-process/SKILL.md @@ -27,14 +27,14 @@ current branch — never touches main. ### 0. Dry-run mode check -If the user invokes `/pr-push --dry-run`: +If the user invokes `/pr-process --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-db` to check current stamp freshness (no merge) - Step 4a: report which watched files changed vs origin/main; show whether `make regen-db` would be triggered; do NOT run the regen, stage, or commit - 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." +- Print "Dry run complete — use /pr-process to apply." - Stop. Do not push or create a PR. --- @@ -55,8 +55,8 @@ step (1b, 1c), check for long-lived Godot processes from prior stuck test runs: ```bash -# 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))}' +# List any godot/gdUnit4 processes running longer than 5 minutes +ps -eo pid,etimes,cmd | awk '$2 > 300 && /godot.*(GdUnitCmdTool|[Gg]d[Uu]nit)/ {print $1, $2"s", substr($0, index($0,$3))}' ``` If any are listed: they are almost certainly orphans from a prior test @@ -85,9 +85,12 @@ ignore` comment with a reason. **For server branches:** ```bash -cargo clippy -- -D warnings 2>&1 +cargo clippy --manifest-path server/Cargo.toml --all-targets -- -D warnings 2>&1 ``` +This must match the pre-push gate's own invocation exactly (`--all-targets` is +easy to drop locally and then miss warnings the gate still catches). + **For CI/tooling branches:** ```bash ruff check tooling/ 2>&1 @@ -105,62 +108,41 @@ 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. +`tooling/godot-cold-parse` wipes the cached script-class registry (matching +the cold-start ordering CI / fresh clones see — Sprint 36 close caught a +`class_name` base-class registration bug that warm caches masked) and runs a +headless parse, filtering known pre-existing noise. Pass `--run-menu` if the +branch has UI changes to also launch the main menu briefly: ```bash -# 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" +tooling/godot-cold-parse --run-menu ``` -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. +If it 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) — fix the ordering, don't just rebuild the cache +to mask it locally (the same error resurfaces post-merge). -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. +Any lines it reports are new errors introduced by this branch. Fix them +before pushing. **For server branches:** ```bash -cd server && cargo test --lib 2>&1 +cargo test --manifest-path server/Cargo.toml 2>&1 ``` +Full suite, not `--lib` — `--lib` skips the believability/derivation golden +harnesses (separate test binaries) and would silently under-test cascade +changes. This must match the pre-push gate's own invocation (`team-patterns.md`) +exactly; a weaker local pass here just gives false confidence right before the +gate catches it anyway. + 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: +works on screen before invoking `/pr-process`. If they haven't, ask: "Have you run `make game` and verified this works visually?" ### 2. Commit uncommitted changes @@ -171,9 +153,8 @@ 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. +Running from a subdirectory can hide real changes (Sprint 30: a wrong-CWD +`git diff` returned 0 lines, masking uncommitted 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 @@ -197,7 +178,7 @@ git fetch --all git log --oneline origin/.. ``` -If no unpushed commits, skip to step 5 (PR check). +If no unpushed commits, skip to step 6 (PR check). ### 4. Check for conflicts with main @@ -222,24 +203,13 @@ versus `origin/main`. This list covers generator code AND the data files that fe The stamped generator sources come from the shared registry `tooling/generator_sources.py` (T-1067) — the same module the stamp writer and `tooling/check-systems-db-stamp` use, so the lists can no longer drift (PR #136 -review T7). The CLI call below expands to one repo-relative path per line; the -extra hardcoded entries are non-stamped watch items (retired/one-time planet-gen -importers, the schema DDL whose SHA is stamped separately, and the data -directories that feed the generators). +review T7). `tooling/pr-watchlist-diff` wraps the comparison — its header comment +documents the non-stamped watch items it also checks (the surviving one-time +planet-gen importers, the schema DDL whose SHA is stamped separately, and the +wiki data directories that feed the generators): ```bash -git diff --name-only origin/main...HEAD -- \ - $(python3 tooling/generator_sources.py --list) \ - tooling/planet-gen/generate_atlas.py \ - tooling/planet-gen/gemma_naming.py \ - tooling/planet-gen/naming_core.py \ - tooling/planet-gen/import_city_names.py \ - tooling/planet-gen/import_heightmaps.py \ - tooling/planet-gen/import_province_boundaries.py \ - server/data/systems-schema.sql \ - wiki/star-systems/ \ - wiki/economics/ \ - content/economics/ +tooling/pr-watchlist-diff origin/main HEAD ``` **If output is empty:** skip this step entirely. @@ -377,5 +347,5 @@ Suggest: "PR processed. Run `/pr-review` from main to review, or `/whats-next` f ## Arguments -If the user passes arguments (e.g., `/pr-push "my title"`), use them as the +If the user passes arguments (e.g., `/pr-process "my title"`), use them as the PR title instead of generating one. diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 82d9eb1eb..8f1d09010 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,12 +1,12 @@ --- name: pr-review description: > - Review a branch diff with team-appropriate agents before merge. Use when the - user says "review-pr", "review this PR", "review this branch", or invokes + Review a Gitea branch diff with team-appropriate agents before merge. Use when + the user says "review-pr", "review this PR", "review this branch", or invokes /pr-review. Spawns reviewers matched to the branch type (code, copy, visual, audio) in parallel. Reports approve/reject with inline comments. user-invocable: true -allowed-tools: Bash, Read, Grep, Glob, Task +allowed-tools: Bash, Read, Grep, Glob, Task, Write --- # PR Review Skill @@ -30,8 +30,8 @@ Stop and wait for the user to invoke `/pr-review` from main. ### 0b. Verify runtime smoke test was performed Before spawning reviewers, check that the pushing team performed basic -runtime verification. This was the #1 process failure of Sprint 28 — -3 review rounds without anyone launching the game missed critical bugs. +runtime verification — the #1 process failure of Sprint 28 (3 review +rounds shipped without anyone launching the game). Ask: "Did the team run `make game` or a headless smoke test before pushing this PR?" @@ -40,13 +40,6 @@ If the PR description or commit messages don't mention runtime testing, note this in the review output as a process gap. Reviewers should still proceed (the PR exists and needs reviewing) but the gap should be visible. -For **client/visual branches**, run a quick headless parse check from main: -```bash -godot --headless --path client --quit 2>&1 | grep -i "SCRIPT ERROR" -``` -If script errors appear in the branch diff files, flag them immediately -before spawning reviewers — no point reviewing code that doesn't parse. - #### 0b-i. Merge-path smoke test gate When the branch diff touches any of: @@ -69,18 +62,17 @@ Unchecked merge-path smoke boxes downgrade the verdict from APPROVED to REQUEST_CHANGES even if reviewers have no code comments. The smoke is a deliverable, not a suggestion. -### 0c. Zero warnings check +### 0c. Parse/lint — deferred to the push gate -The project enforces a **zero warnings policy**. Before spawning reviewers, -check if the branch introduces lint warnings: - -- **client/visual:** `gdlint client/scripts/ client/ui/` should report 0 issues -- **server:** `cargo clippy -- -D warnings` should be clean -- **ci/tooling:** `ruff check tooling/` should be clean - -If warnings exist, note the count in the review output. Reviewers should -flag any **new** warnings introduced by the branch as `warning` severity. -Pre-existing warnings are not PR blockers but should be tracked for cleanup. +`/pr-review` runs on an already-pushed branch (step 0's branch guard means the +reviewer's only local tree is **main**). Re-running `gdlint`/`cargo clippy`/`ruff` +here would check main's code, not the branch's — and `cargo clippy` fails outright +at the repo root (no root `Cargo.toml`; the server crate is `server/`). None of +that is necessary anyway: the pre-push hook (`.config/hooks/pre-push`, declared +authoritative by `team-patterns.md`) already ran `cargo fmt --check` / `cargo +clippy --all-targets -- -D warnings` / `cargo test` (server changes) or the full +gdUnit4 suite via `tests/run-godot` (client changes) before this push succeeded. +Trust the gate; don't duplicate it here with a weaker, wrong-tree variant. ### 1. Determine the branch to review @@ -113,7 +105,9 @@ git diff --stat main... Examine the changed file paths to determine the dominant change type: -- Mostly `server/` changes → **code** reviewers +- Mostly `server/` changes → **code** reviewers — *except* `server/content/` + (game content data: campaigns, gauntlet, global, `content.yaml`), which routes + to **copy** reviewers despite living under `server/` - Mostly `client/` changes (excluding `client/ui/` art assets) → **code** reviewers - Mostly `wiki/`, `docs/atlas/`, `content/` changes → **copy** reviewers - Mostly `client/ui/` or asset changes (`.tres`, `.tscn`, textures) → **visual** reviewers @@ -140,59 +134,54 @@ If the diff is empty, report "No changes to review" and stop. - `Cargo.lock` (auto-generated) - `client/addons/gdUnit4/` (vendor test framework) - `*.uid` (Godot-generated) -- `docs/backups/settledreach.db.backup` (binary) +- `docs/backups/*` (binary DB backups, e.g. `settledreach.db.backup.pre35`) Three-dot diff with pathspec exclusions is unreliable. Instead, either: 1. Use `git diff main...` (full diff) and filter in the prompt, or -2. Read source files directly from the team directory (see below). +2. Read source files directly from the worktree (see below). For large diffs (>1000 lines of source), provide **source files** rather than raw diff to reviewers — cleaner context, better reviews. -**Reviewer agents read source files from the team worktree.** +**Reviewer agents read source files from the branch worktree — verified, not +assumed — or via `git show`.** -Sprint branches follow `sprint-{N}/{team}`. Mid-sprint, a worktree -of each branch exists at `$(dirname )/.sprint/sprint-{N}/{team}/` -— a *sibling* of the repo root, not a child. This worktree IS the -branch: Read/Grep on paths rooted there resolve against the branch's -checkout, not main's. - -**Why this matters:** Sprint 37 PR #138 review produced 6 false- -positive findings because the reviewer defaulted to Read/Grep on the -main repo path (`/var/mnt/data/projects/settled-reach/main/`) instead -of the branch worktree. Every finding was a verbatim match against -main's state but irrelevant to the branch — the branch had already -cleaned the residue the reviewer flagged as "still present." Sending -those findings to the team would have caused busywork on already-clean -code, and more dangerously, the same drift hides *false negatives* -(branch-introduced bugs the reviewer never saw because it never read -the branch). - -Fix: before spawning reviewers, resolve the worktree path and pass it -into every reviewer prompt with prominent language. The reviewer -reads from the worktree, not from main. +Topic branches from `/whats-next` may have a worktree at +`/.worktrees/` (the kanban convention, D-221 — see +`tooling/worktree-setup`): ```bash -# Find worktree for this branch (if one exists) -WORKTREE=$(git worktree list --porcelain | grep -B2 "branch refs/heads/" | grep "worktree " | sed 's/worktree //') - -# Verify it exists and matches the branch tip -git -C "$WORKTREE" rev-parse HEAD # should equal `git rev-parse origin/` +REPO_ROOT="$(git rev-parse --show-toplevel)" +WORKTREE="$REPO_ROOT/.worktrees/" ``` -If the worktree exists and its HEAD matches `origin/`, use it -as the reviewer's source of truth. If it doesn't exist (e.g. the -worktree has been torn down or you're reviewing a branch without a -worktree), fall back to `git show origin/:` — explicitly -flag this fallback in the reviewer prompt so the reviewer knows -Read/Grep on any local path would be wrong. +If that path doesn't exist, fall back to the generic resolution (also used by +`/pr-review` §7a cleanup): + +```bash +WORKTREE=$(git worktree list --porcelain | grep -B2 "branch refs/heads/" | grep "worktree " | sed 's/worktree //') +``` + +Use the worktree as the reviewer's source of truth **only if both** hold — +otherwise fall back to `git show origin/:` and flag the fallback +in the reviewer prompt: + +1. **HEAD matches the branch tip:** `git -C "$WORKTREE" rev-parse HEAD` equals `git rev-parse origin/`. +2. **Not a sparse checkout:** `git -C "$WORKTREE" sparse-checkout list` is empty or errors ("not a sparse checkout"). + +Two incidents motivate both checks — cited once, here, so they don't drift +back into two contradictory rules: Sprint 37 (a reviewer defaulted to reading +the main repo root instead of the branch worktree — 6 false positives) and +Sprint 33 (a sparse worktree silently excluded the wiki directory — a false +"missing prose" finding). One rule now covers both: verify HEAD *and* +sparse-checkout status, or use `git show`. In the reviewer prompt, state the rule non-negotiably: -> **Read source from `` only.** Do NOT Read or Grep -> paths under the main repo root (`/var/mnt/data/projects/settled-reach/main/`). -> Those resolve to main, not the branch. The worktree at `` -> IS the branch — point all file tools there. +> **Read source from `` only** (or `git show +> origin/:` if no valid worktree exists). Do NOT Read or Grep +> paths under the main repo root (`$(git rev-parse --show-toplevel)`) — that +> resolves to main, not the branch. Also tell agents to read relevant `governance/**/*.md` files for context (these can be read from either path — they're usually identical — @@ -200,7 +189,7 @@ but for consistency, use the worktree path). ### 4. Spawn reviewers in parallel -Use the Task tool to spawn **all reviewers simultaneously** in a single message. +Use the Agent tool to spawn **all reviewers simultaneously** in a single message. Read `references/reviewer-profiles.md` for the full per-branch-type reviewer specifications (agent types, models, prompt focus areas). Match the branch type @@ -269,7 +258,7 @@ Verdict rules: something can be better, say so. ``` -## 6. Posting results to Gitea +### 6. Posting results to Gitea After presenting results to the user, post the review as a PR comment. @@ -283,8 +272,8 @@ Post using the `tea-comment` wrapper (handles temp files and cleanup). `tooling/tea-comment` in a separate short Bash call. 2. **Run `tooling/tea-comment` in the FOREGROUND, never with `run_in_background`.** The background execution path silently fails — - the comment never reaches Gitea and the team never sees the review. - Sprint 38 lost an entire review round this way. Always foreground. + the comment never reaches Gitea and the team never sees the review + (Sprint 38 lost an entire review round this way). ``` # Step 1: Use the Write tool to create the file @@ -297,7 +286,7 @@ tooling/tea-comment @/tmp/pr-review-.md Do NOT use `cat << 'EOF'` heredocs for review content — they create massive permission prompts that are slow to render and often get stuck. -## 7. Merging approved PRs +### 7. Merging approved PRs `tea pr merge` fails (405) when branches have conflicts with main. Merge locally instead: diff --git a/.claude/skills/pr-review/references/reviewer-profiles.md b/.claude/skills/pr-review/references/reviewer-profiles.md index 5bf12b827..16ac25bf2 100644 --- a/.claude/skills/pr-review/references/reviewer-profiles.md +++ b/.claude/skills/pr-review/references/reviewer-profiles.md @@ -2,23 +2,18 @@ Use `model: sonnet` for all reviewers — sufficient for review, saves cost. -**Reviewer agents read source files via `git show` or from sprint -worktrees.** Sprint branches use `sprint-{N}/{team}` naming. Include -the branch name and a list of changed files in every prompt. The -default approach is `git show origin/:`. If an active -worktree exists under `.sprint/`, agents can also use the Read tool -with the worktree path. **Always prefer `git show` over worktree -reads** — worktrees may use sparse checkouts that silently exclude -files, causing reviewers to miss content and produce false findings -(Sprint 33 lesson: Paula reported missing prose that was actually -present, because the worktree excluded the wiki directory). +**Reviewer agents read source files from the branch worktree or via `git +show`** — see SKILL.md §3 for the full resolution + validation rule (prefer +the worktree only when its HEAD matches the branch tip AND it isn't a sparse +checkout; otherwise `git show origin/:`). Include the branch +name and a list of changed files in every prompt. ## Code reviews (`server`, `client`, `ci`) **Hoshe (Code Quality)** - `subagent_type`: `hoshe`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Hoshe to read source files from the team directory, then review for: +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Hoshe to read source files from the worktree, then review for: - Correctness and bug risks - Error handling gaps - Test coverage (are new features tested?) @@ -28,8 +23,8 @@ present, because the worktree excluded the wiki directory). **Tyre (Architecture)** - `subagent_type`: `tyre`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Tyre to read the relevant `governance/**/*.md` files from the team directory +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Tyre to read the relevant `governance/**/*.md` files from the worktree first, then review for: - Architectural consistency with project decisions - API/interface design quality @@ -41,8 +36,8 @@ present, because the worktree excluded the wiki directory). **Hoshe (QA)** - `subagent_type`: `hoshe`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Hoshe to read the changed files from the team directory, then review for: +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Hoshe to read the changed files from the worktree, then review for: - Formatting consistency (markdown, file naming, frontmatter) - Broken references or links - Spelling and grammar @@ -51,9 +46,9 @@ present, because the worktree excluded the wiki directory). **Paula (Narrative Depth)** - `subagent_type`: `paula`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, commit log, and +- Prompt: Provide the worktree path, list of changed files, commit log, and list of relevant `governance/**/*.md` files to read. Tell Paula to read all - files from the team directory using the Read tool, then review for: + files from the worktree using the Read tool, then review for: - Narrative quality and character voice consistency - Whether dialogue and monologue feel authentic to the characters - Consequences and stakes — do choices carry weight? @@ -62,9 +57,9 @@ present, because the worktree excluded the wiki directory). **Miri (World Consistency)** - `subagent_type`: `miri`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, commit log, and +- Prompt: Provide the worktree path, list of changed files, commit log, and list of relevant `governance/**/*.md` files to read. Tell Miri to read all - files from the team directory using the Read tool, then review for: + files from the worktree using the Read tool, then review for: - Lore accuracy — do facts match established setting? - Internal consistency across files - IP originality — nothing should read as a copy from another franchise @@ -75,8 +70,8 @@ present, because the worktree excluded the wiki directory). **Hoshe (QA)** - `subagent_type`: `hoshe`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Hoshe to read the changed files from the team directory, then review for: +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Hoshe to read the changed files from the worktree, then review for: - File format and naming conventions - Asset organization and directory structure - Missing or broken references in scene/resource files @@ -84,7 +79,7 @@ present, because the worktree excluded the wiki directory). **Araminta (Art Direction)** - `subagent_type`: `araminta`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. +- Prompt: Provide the worktree path, list of changed files, and commit log. Tell Araminta to read the style guide and relevant design docs from the worktree first, then review for: - Visual consistency with the established style guide @@ -97,8 +92,8 @@ present, because the worktree excluded the wiki directory). **Hoshe (QA)** - `subagent_type`: `hoshe`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Hoshe to read the changed files from the team directory, then review for: +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Hoshe to read the changed files from the worktree, then review for: - File format and naming conventions - Audio asset organization and directory structure - Missing or broken references @@ -106,8 +101,8 @@ present, because the worktree excluded the wiki directory). **Ozzie (Player Experience)** - `subagent_type`: `ozzie`, `model`: `sonnet` -- Prompt: Provide the team directory path, list of changed files, and commit log. - Tell Ozzie to read all files from the team directory using the Read tool, then +- Prompt: Provide the worktree path, list of changed files, and commit log. + Tell Ozzie to read all files from the worktree using the Read tool, then review for: - Emotional impact — does the audio enhance the moment? - Atmosphere and tone — does it feel like the Settled Reach? diff --git a/.claude/skills/whats-next/SKILL.md b/.claude/skills/whats-next/SKILL.md index 47f2b8ecf..4795a7747 100644 --- a/.claude/skills/whats-next/SKILL.md +++ b/.claude/skills/whats-next/SKILL.md @@ -47,6 +47,11 @@ lowest-numbered non-`done` phase epic): pql ticket list --under T-745 --status in_progress --fields id,type,status,title ``` +> `--status` takes a SINGLE value (not a comma list — `backlog,in_progress` matches +> nothing and silently returns `[]`). Most tickets here sit in `backlog` or `done`; +> if you need multiple statuses at once (e.g. both `backlog` and `in_progress`), run +> the query once per status. + The phase epic is the `"type": "epic"` row. Use pql natively — no ad-hoc python/jq projections over its output: since pql 1.11.0, `ticket list` omits `description` by default and supports `--fields id,type,parent_id,priority,status,title` (the batch- @@ -179,6 +184,23 @@ For each ticket in the batch (batch with commas: `pql ticket status T-1,T-2 in_p pql does **not** enforce the WIP limit — check it yourself in step 1d and warn if the batch pushes in-progress past 5. +> **Then make sure it persists.** Ticket mutations (status here, and any +> `ticket append` in Step 2) land only in the gitignored `.pql/pql.db`. The +> pre-commit hook runs `pql plan export --stage` automatically — the changelog +> is exported and staged on every commit, so never hand-run the export or +> `git add .pql/changelog`. The rule is simpler: the turn must land at least +> one commit (through the git-commit skill). A ticket-only turn with no commit +> leaves the mutations in `pql.db` only, and the post-checkout/post-merge hooks +> rebuild that DB from the committed changelog on the next branch switch — +> silently dropping them. See the pql skill's "Versioning planning state" +> section for the full mechanics. +> +> - Don't hand-export or hand-stage `.pql/changelog/` — the pre-commit hook does +> both on every commit. The real footgun is a turn that mutates tickets but +> never commits: `pql.db` is gitignored and gets rebuilt from the committed +> changelog on branch switch, silently dropping un-committed mutations. Land +> at least one commit per ticket-mutating turn. + ### 3b. Create topic branch Name the branch after the epic or logical grouping. Examples: diff --git a/.claude/skills/workshop-start/IMPROVEMENTS.md b/.claude/skills/workshop-start/IMPROVEMENTS.md deleted file mode 100644 index 3ba9b1162..000000000 --- a/.claude/skills/workshop-start/IMPROVEMENTS.md +++ /dev/null @@ -1,87 +0,0 @@ -# workshop-start — Improvement Log - -Running notes on friction and fixes observed while running workshops. Fold the -confirmed ones into `SKILL.md` periodically; delete once applied. - -## From the `system-economic-specialization` relaunch (2026-05-31) - -- **Tool name drift.** SKILL.md §5 says "Use the **Task tool** to spawn each - agent." The actual spawn tool is **`Agent`** (with `team_name` + `name` + - `subagent_type`). "Task" reads as the TaskCreate/TaskUpdate family and is - misleading. Rename to "Agent tool" throughout §5. - -- **SI has no Round 1 work — say so.** The skill adds SI to every workshop but - SI's job is Round 2 ticket creation. Round 1 there is no SI task, so SI sits - with an empty TaskList and no instruction. Recommend the skill explicitly note: - "SI joins on standby; SI gets no task until Round 2 — spawn it in background - with a 'load context and wait for Round 2' prompt." Avoids an idle/confused - agent on spawn. - -- **Spawn order vs. task creation.** Following the skill literally (§4 create - tasks → §5 spawn) matters: if you spawn agents before the tasks exist, they - check TaskList, find nothing, and may idle. Worth a one-line warning in §5: - "Create and assign all Round 1 tasks BEFORE spawning, or the agents wake to an - empty list." (Recoverable by messaging them after, but cleaner to order it - right.) - -- **`TaskCreate` is one-task-per-call.** No batch/array form. A batch attempt is - rejected. The skill could state this so the lead emits N separate TaskCreate - calls (and assigns owners via TaskUpdate, since TaskCreate takes no `owner`). - -- **Read-only participants can't write their own files.** Several workshop agent - types (paula, gore, nigel, ozzie, and the read-only miri variant) have NO Write - tool — yet SKILL.md §4 makes "write your full output to disk" a hard - requirement for every participant. Paula hit this: she produced her full Round 1 - doc but had to send it to the lead to write `paula-round1.md`. The skill should - state up front: "Participants without a Write tool deliver their output via - SendMessage; the lead writes the file and marks the task complete on their - behalf." Or: assign a write-capable scribe. Either way, don't make disk-write a - per-agent requirement for read-only agent types — it guarantees a manual relay. - -- **Participants self-claim, which races owner assignment.** Paula picked up - task #3 on her own before the lead assigned owners. Harmless here, but if tasks - aren't clearly scoped per-agent an agent could grab the wrong one. The skill's - per-agent task titles ("Round 1 — Miri …") mitigate this; keep titles - agent-named. - -## Candidate feature — per-round "scout" critic (user idea, 2026-05-31) - -Add an optional independent **scout** agent that runs per round, AFTER the round's -output files land and BEFORE the user checkpoint, to surface missed options, -simpler designs, internal contradictions, and external design prior-art. - -- **Value is mostly the independent adversarial review**, not the web. Participants - build on each other and converge; a fresh agent with no stake catches blind - spots convergence hides. Web search is a bonus, useful for the *mechanism* (real - economic geography, how other games encode X, schema-granularity tradeoffs) — - NOT the lore. -- **Hard guardrails:** (1) lore is authored-canonical and fictional — the scout must - NOT "correct" the fiction with real-world/web facts; scope web to design/mechanism - prior-art only. (2) Only surface findings that would CHANGE a decision — a critic - that always finds 10 things gets ignored. -- **Shape:** cheap model (Haiku), `run_in_background: true`, reads the round files + - relevant D-records, writes `scout-round{N}.md`. Lead folds decision-changing items - into the checkpoint presented to the user. Escalate to Sonnet only to dig into a - real find. -- Pairs well with the quality-over-scope-creep principle: the scout is a cheap way - to pressure-test that "quality" claims actually hold and nothing simpler was missed. - -## Wrap-up gotchas (2026-05-31) - -- **`pql decisions claim` is side-effect-free** (it just prints the next available ID). - This fixes the old `tooling/db/decision claim` footgun, which burned an ID and appended - a placeholder on every call — retrying it created duplicate records. With pql, claim is - safe to re-run; the record only exists once you write the `### D-NNN: …` heading into the - governance file. Still claim each ID once and write it immediately so parallel branches - don't pick the same number. -- **Read-only documenter can still file** — the lead hands it the claimed ID(s); the - documenter writes the records into `governance/{decisions,questions,rejected}/.md`. -- **Background (`run_in_background`) teammates can be slow/unreliable to consume - `shutdown_request`.** At wrap-up, qatux + si (both spawned in background per the - skill's §5 guidance) stayed "active" through multiple shutdown sends, blocking - TeamDelete. Foreground participants shut down immediately. Options to fold into - the skill: (a) don't background the always-present agents, or (b) document that - wrap-up may need several shutdown retries / a longer settle, and that the lead - should not spin indefinitely — the work is already on disk, TeamDelete is just - resource cleanup. Consider whether the skill should note the team can be left to - expire rather than blocking the session on cleanup. diff --git a/.claude/skills/workshop-start/SKILL.md b/.claude/skills/workshop-start/SKILL.md index d2be90f84..cc54f5164 100644 --- a/.claude/skills/workshop-start/SKILL.md +++ b/.claude/skills/workshop-start/SKILL.md @@ -4,7 +4,7 @@ description: > Start a multi-agent design workshop from a workshop brief. Use when the user says "start workshop", "run workshop", "let's start the workshop", or invokes /workshop-start. Parses the workshop brief to extract participants, questions, and round format. - Creates a team, tasks, and spawns agents as teammates via the Task tool. + Creates a team, tasks, and spawns agents as teammates via the Agent tool. --- # Start Workshop @@ -24,7 +24,8 @@ Read the workshop brief. Extract: - Workshop name (from directory name) - Participant list (from `**Participants:**` line) - Per-participant questions (scan for `**{AgentName}**:` patterns in questions sections) -- Round count and round descriptions (from `**Workshop Format**` section) +- Round count and round descriptions (from the `## Workshop Format` heading — a real + heading, not a bold marker; every brief under `docs/workshops/` uses this form) ### 2. Create Team @@ -38,8 +39,8 @@ Two agents join every workshop regardless of the participant list: | Agent | Role | Task | Participates in discussion? | |-------|------|------|-----------------------------| -| **Qatux** | Documenter | Captures all decisions, questions, dissent, consensus. Writes `workshop-notes.md` per round, produces final `workshop-outcomes.md`. | No — observes and records only | -| **SI** | Sprint prep | Suggest adding when outputs include tickets. Creates tickets from decisions, links to sprint backlog. | No — execution prep only | +| **Qatux** | Documenter | Captures all decisions, questions, dissent, consensus. Writes `round-{N}-notes.md` per round, produces final `workshop-outcomes.md`. | No — observes and records only | +| **SI** | Refinement reviewer | Suggest adding when outputs include tickets. Reviews `workshop-outcomes.md` for ticket-worthy items and context completeness — does **not** create tickets (si.md's "What you do NOT do" list is explicit: no ticket creation or DB writes). The lead creates tickets via the `/ticket` skill; SI reviews them for gaps. | No — execution prep only | Qatux and SI are **never dismissed early.** If the user reduces the team mid-workshop, keep qatux and si (if added). Documenting everything prevents loss of valuable information. @@ -50,6 +51,7 @@ One task per participant containing: - The specific questions assigned to that participant (extracted from all layers) - The output format from the brief's round description - **IMPORTANT — file output requirement:** Instruct each agent to write their full output to `docs/workshops/{name}/{agent}-round{N}.md` (e.g., `docs/workshops/test-architecture/gestalt-round1.md`). Agents must write to disk, not just send messages. This ensures Qatux and other agents can read all outputs directly without relying on message forwarding. + **Several participant types have no Write tool** (ozzie, gore, nigel, paula, miri — verify current grants in `.claude/agents/*.md` frontmatter). Participants without a Write tool deliver their output via SendMessage instead; the lead writes `{agent}-round{N}.md` on their behalf and marks the task complete. One task for Qatux: "Document Round N — read all agent output files at `docs/workshops/{name}/*-round{N}.md` and capture decisions, questions, and dissent." @@ -57,16 +59,21 @@ Assign all tasks using TaskUpdate with `owner` = agent name. ### 5. Spawn Agents -Use the Task tool to spawn each agent as a teammate. Each call should: +**Create and assign all Round 1 tasks (§4) before spawning.** Spawning first means +agents wake to an empty TaskList and may idle. SI joins on standby with no Round 1 +task — SI's job starts at Round 2 ticket review — so give it a "load context and +wait" prompt rather than an empty task. + +Use the Agent tool to spawn each agent as a teammate. Each call should: - Set `team_name` to the workshop team name - Set `name` to the agent name (e.g., "gestalt") - Set `subagent_type` to the matching agent type (same as name — see reference table) - **Set `model: "sonnet"`** — team-mode participants pin to Sonnet 4.6 for literal-rule-following discipline. Opus 4.7 was observed ignoring scope rules and failing to report back in team mode (Sprint 37). The *team lead* (this session) stays on whatever model the user has selected. - Provide a prompt telling the agent to check TaskList for their assigned task -Spawn all agents in parallel (one Task call per agent in a single message). Agents will appear as teammates in the Claude Code UI and pick up their tasks from the shared task list. +Spawn all agents in parallel (one Agent call per agent in a single message). Agents will appear as teammates in the Claude Code UI and pick up their tasks from the shared task list. -For large workshops (>6 agents), spawn participants in batches to avoid overwhelming the system. Always-present agents (Qatux, SI) can run in background via `run_in_background: true`. +For large workshops (>6 agents), spawn participants in batches to avoid overwhelming the system. Always-present agents (Qatux, SI) can run in background via `run_in_background: true` — but background teammates can be slow or unreliable to consume `shutdown_request`, so wrap-up (§8) may need several shutdown retries. Don't spin indefinitely waiting on it: the work is already on disk, and the team can be left to expire rather than blocking the session on `TeamDelete`. ### 6. Monitor @@ -96,9 +103,9 @@ When all Round N tasks are complete: Before the user dismisses the team, the following are **hard requirements**: 1. **User reviews final outcomes** — Present `workshop-outcomes.md` content to the user via AskUserQuestion. Get explicit approval before proceeding to filing. -2. **D-records filed** — All new D-records, amendments, and supersessions are written to `governance/` domain files. This is non-negotiable — workshops that produce decisions MUST file them before shutdown. +2. **D-records filed** — **Claim each ID first** with `pql decisions claim D "title"` (side-effect-free — safe to re-run, just prints the next free id) and write the record immediately after claiming, so parallel branches don't collide on the same number (CLAUDE.md requirement). All new D-records, amendments, and supersessions are written to `governance/` domain files. This is non-negotiable — workshops that produce decisions MUST file them before shutdown. 3. **Discussion captured** — Qatux produces final `workshop-outcomes.md` from accumulated notes. Qatux creates or updates diagrams (via `/d2-diagram`) for any new D-records produced by the workshop. -4. **Tickets created** — If SI is present, SI creates tickets from decided items and the user reviews the ticket list. +4. **Tickets created** — The lead creates tickets from decided items via the `/ticket` skill, grounded in `workshop-outcomes.md`. If SI is present, SI reviews the drafted tickets for context completeness before the user reviews the ticket list. 5. **User gives explicit go-ahead to dismiss** — Only after steps 1-4 are complete AND the user confirms, send shutdown_request to all agents (qatux and si last). 6. TeamDelete to clean up. @@ -156,3 +163,5 @@ Agent name maps directly to subagent_type: | inigo | Sound design | Audio, soundscape, spatial audio | | troblum | Tech consultant | Second opinion on architecture/tech choices | | hoshe | QA/testing | Test plans, verification, quality | +| burnelli-sheldon | Economist | Market models, price formation, economic credibility | +| justine | Polish/deployment | Build, performance, release quality | diff --git a/tooling/godot-cold-parse b/tooling/godot-cold-parse new file mode 100755 index 000000000..b52c181d3 --- /dev/null +++ b/tooling/godot-cold-parse @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# tooling/godot-cold-parse [--run-menu] — cold-cache headless parse check. +# +# Used by /pr-process step 1c before push. Deletes the cached script-class +# registry so the parse simulates the cold-start ordering CI / fresh clones +# see: Sprint 36 close caught a new `class_name MetaScreen` base class and +# six extending scripts that parsed fine on warm developer caches but hit +# `Could not find base class "MetaScreen"` post-merge, because the +# autoload-vs-class_name registration order only resolves correctly once the +# class cache is seeded (see CLAUDE.md -> GDScript conventions -> Autoload +# parse-order rule). +# +# Godot's resource scanner emits category errors (e.g. "Export type can only +# be built-in, a resource, a node, or an enum") that do NOT always prefix +# with SCRIPT ERROR — they appear as plain ERROR lines. The filter below +# catches both, then drops known pre-existing noise from the autoload +# class_name parse-order trap. Sprint 36 shipped a scanner error the old +# narrower grep missed; this is why the filter stays wide. +# +# --run-menu: also launch main_menu.tscn briefly (for branches with UI changes). +# +# Exit 0 + "clean" if no matches. Exit 1 + the matched lines if any are found. +set -euo pipefail + +REPO_ROOT="$(git rev-parse --show-toplevel)" +RUN_MENU=false +[ "${1:-}" = "--run-menu" ] && RUN_MENU=true + +rm -f "$REPO_ROOT/client/.godot/global_script_class_cache.cfg" + +FILTER='^(SCRIPT )?ERROR|Parse Error|Export type' + +MATCHES=$(godot --headless --path "$REPO_ROOT/client" --quit 2>&1 \ + | grep -iE "$FILTER" \ + | grep -v "Failed loading resource: res://assets" \ + | grep -v "Cannot infer the type" \ + | grep -vE '(Messagepack|LocalBridge|ServerProcess|Constants)" not declared' || true) + +if [ "$RUN_MENU" = true ]; then + MENU_MATCHES=$(timeout 10 godot --path "$REPO_ROOT/client" res://scenes/main_menu.tscn 2>&1 \ + | grep -iE "$FILTER" || true) + if [ -n "$MENU_MATCHES" ]; then + MATCHES="$MATCHES +$MENU_MATCHES" + fi +fi + +if [ -n "$MATCHES" ]; then + echo "$MATCHES" + exit 1 +fi +echo "godot-cold-parse: clean" diff --git a/tooling/pr-watchlist-diff b/tooling/pr-watchlist-diff new file mode 100755 index 000000000..bcedecbe5 --- /dev/null +++ b/tooling/pr-watchlist-diff @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# tooling/pr-watchlist-diff — list watch-list files changed +# between and . +# +# Used by /pr-process step 4a (T-858) to decide whether `make regen-db` must +# run before push. The stamped generator sources come from the shared +# registry tooling/generator_sources.py (T-1067) — imported live here so this +# list can't drift from the stamp writer / tooling/check-systems-db-stamp. +# +# The extra hardcoded paths below are non-stamped watch items: the surviving +# one-time planet-gen importers (import_heightmaps.py, import_province_ +# boundaries.py — not part of `make regen-db`, but their data feeds the +# committed DB), the schema DDL (stamped separately via schema_sha), and the +# wiki data directories that feed the generators. +set -euo pipefail + +BASE="${1:?usage: tooling/pr-watchlist-diff }" +HEAD="${2:?usage: tooling/pr-watchlist-diff }" + +mapfile -t GENERATOR_SOURCES < <(python3 tooling/generator_sources.py --list) + +git diff --name-only "$BASE...$HEAD" -- \ + "${GENERATOR_SOURCES[@]}" \ + tooling/planet-gen/import_heightmaps.py \ + tooling/planet-gen/import_province_boundaries.py \ + server/data/systems-schema.sql \ + wiki/star-systems/ \ + wiki/economics/