chore(skills): sprint 36 retro process improvements
- pr-review: require manual smoke test for merge-path PRs (pre-game flow, scene transitions, save/load, connection handshake). Unchecked author-side smoke boxes downgrade verdict from APPROVED to REQUEST_CHANGES. Triggered by #872 — New Game hangs on 'connecting' landed through PR #134 with the exact smoke box unchecked. - pr-push: wipe Godot script class cache before the runtime smoke check so cold-start parse-order bugs surface pre-push instead of failing the next fresh-clone parse. Sprint 36 hit this post-merge — MetaScreen base class not found because the cache was warm on developer machines but cold in CI / post-merge. Cold-cache check stays in the skill (not the pre-push hook) to keep iteration fast.
This commit is contained in:
@@ -91,7 +91,22 @@ 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.
|
||||
|
||||
```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
|
||||
@@ -109,6 +124,13 @@ 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`
|
||||
|
||||
@@ -47,6 +47,28 @@ 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:
|
||||
- pre-game flow (main menu → character creation → connect)
|
||||
- scene transitions (`change_scene_to_file`, scene autoloads)
|
||||
- save / load / new-game paths
|
||||
- connection handshake (`sim_bridge`, protocol decode/encode)
|
||||
- any code path executed in the first 30 seconds of a new session
|
||||
|
||||
...the reviewer output MUST explicitly call out the state of author-side
|
||||
manual smoke boxes in the PR test plan. If any merge-path smoke box is
|
||||
unchecked, include a top-level note:
|
||||
|
||||
> **Merge-path smoke not performed.** PR test plan has unchecked manual
|
||||
> smoke box(es): [list]. A reviewer or the team must run the smoke before
|
||||
> merge approval. Sprint 36 bug #872 (New Game hangs on 'connecting')
|
||||
> landed exactly here — do not skip.
|
||||
|
||||
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
|
||||
|
||||
The project enforces a **zero warnings policy**. Before spawning reviewers,
|
||||
|
||||
Reference in New Issue
Block a user