From c25c5de08bd96c7d2355bf615340b7ab0b65ebaa Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 6 Nov 2026 11:30:18 +0000 Subject: [PATCH] fix: pre-landing review fixes (Step 11: unique UID proof, protected paths never declared test code) Review-Finding: v1/outside-voice#1 reader=outside-voice Review-Finding: v1/outside-voice#2 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/ship-policy.md | 5 ++++- docs/troubleshooting.md | 3 ++- lib/ship-patch.ts | 5 +++-- lib/ship-sidecar.ts | 13 ++++++++++++- test/ship-uid-sidecar.test.ts | 4 ++++ 5 files changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 10ef467c5..a829c89b1 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -70,7 +70,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `release_gate` | `{ cadence_hours, gate_workflow, branch, stable_ref, check_name, on_red, bisect_spend_cap_usd }` or `null` | `null` | One full paid gate on main every `cadence_hours` (8 to 168) instead of the per-push main backstop, feeding the stable release; see [release_gate](#release-gate). Every field is required: `on_red` is `hold-release`, `bisect_spend_cap_usd` 0 to 500 (0 skips the bisect). `null` keeps the per-push backstop. Needs `min_gstack_version`. | | `bookkeeping` | glob list | `[]` | Root-anchored globs of bookkeeping tables a verify or ratchet step rewrites (size baselines, serial lists, limits), each ending in `.tsv`, `.csv` or `.txt` and never under `.gstack/` or `.github/` (`["scripts/*-baseline.tsv"]`). A matching file changed at most 20 lines after review binds, first touched or not, takes finish's [one scoped reader](review-policy.md#bounded-post-review-edits) instead of Steps 5–11.5; the reader still reads every changed row. Read from the base revision only. Needs `min_gstack_version`. | | `mechanical_retake` | `[{ path, template, fields, regen }]` | `[]` | A version or rules constant a branch may have to retake after a base merge, next to `bookkeeping`'s bounded kind: `path` is one literal file, `template` the whole line with one `{n}` (`const RULE_NUMBERS := {n}`), `fields` the `{ glob, field }` fixture fields that carry the same value, and `regen` the [`.gstack/generated.json`](pregate.md#generated) entry ids whose command regenerates its goldens. A post-review edit finish proves to be only that retake routes as `mechanical=`, with no reader and no review round; see [mechanical retakes](#mechanical-retake). At most 8 constants and 16 fields or ids each; no glob, `..`, `.gstack/` or `.github/` path. Read from the base revision only. Needs `min_gstack_version`. | -| `test_code` | glob list | `[]` | Root-anchored globs of test code the built-in test names miss, never under `.gstack/` or `.github/` (`["tests/**/*.gd"]` for GDScript checks). A matching file changed after review routes as test code, one scoped reader of its test hunks, instead of behavior, beside the built-in globs; shared `helpers/` and `fixtures/` files stay behavior. See [test code and Godot UID sidecars](#test-code). Read from the base revision only. Needs `min_gstack_version`. | +| `test_code` | glob list | `[]` | Root-anchored globs of test code the built-in test names miss, never under `.gstack/` or `.github/`, and never matching a file there (`["tests/**/*.gd"]` for GDScript checks). A matching file changed after review routes as test code, one scoped reader of its test hunks, instead of behavior, beside the built-in globs; shared `helpers/` and `fixtures/` files stay behavior. See [test code and Godot UID sidecars](#test-code). Read from the base revision only. Needs `min_gstack_version`. | | `base_red` | `{ setup, select, timeout_sec }` or `null` | `null` | How `/ship` Step 5 proves a failure is the base branch's own before pausing on it; see [base_red](#base-red). Read from the base revision only. Needs `min_gstack_version`. | | `changelog_fragments` | `{ dir, required_checks? }` or `null` | `null` | PRs carry unversioned [CHANGELOG fragments](#changelog-fragments) in `dir` (one flat, repo-relative directory, no `..`) and the merge step ([`land`](#land)) assigns the version (under `merge: direct`, the [release PR](#release-stamp) does). `required_checks` (up to 50 distinct names) are the checks [`train land`](#stamped-trains-under-changelog_fragments-train-land) requires present and green on a stamp commit. `null` keeps `[Unreleased]` sections and per-PR stamps. Needs `min_gstack_version`. | @@ -1857,6 +1857,9 @@ reader, when all of these hold: whole content is `uid://` with an optional trailing `\n`: the id canonical (no leading `a` digit unless it is `a`), at most 13 digits and at most 2^63−1 (`d4n4ub6itg400`); +- no other file in the current tree names the same `uid://`: a copied + sidecar would give two resources one UID, and a scene that already refers + to the id leaves the `.uid` to a reader; - neither it nor its sibling matches the base policy's [`review_exclude_paths`](#review-exclude-paths) or `readers.forbid_paths`, since no reader saw an excluded script. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 4d4c1fbdf..3bc204620 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3208,7 +3208,8 @@ compared with the reviewed range`, `has no sibling that the reviewed range added or modified or the review-loop exception accepted`, `has no sibling at the current tree`, `existed at the reviewed tree or the current base`, `is not a regular non-executable file (mode )`, `is not -exactly one Godot uid:// line`, or `or its sibling matches the base +exactly one Godot uid:// line`, `shares its UID with `, `could not +be searched for its UID in the current tree`, or `or its sibling matches the base policy's review_exclude_paths or readers.forbid_paths`. **Fix.** Commit the sidecar exactly as Godot wrote it, beside a script this PR diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts index bfeaa7c04..15f0c7d1f 100644 --- a/lib/ship-patch.ts +++ b/lib/ship-patch.ts @@ -223,11 +223,12 @@ export function packetBrief(): string { * A test code file outside shared helpers and fixtures: `*.test.*`, `*.spec.*` and `*_test.go` anywhere, and the * name-only conventions (`test_*.py`, `*Test.java`, `*Tests.swift`, …) only under a test directory, since product * files can carry those names (`src/main/java/ABTest.java`). A path the base policy's `test_code` globs declare counts too - * (GDScript's `tests/*_check.gd`, GST-1604). A change to one gets the scoped review, not the full loop. + * (GDScript's `tests/*_check.gd`, GST-1604), never one under `.gstack/` or `.github/`. A change to one gets the scoped review, + * not the full loop. */ export function isTestCode(rel: string, declared: readonly string[] = []): boolean { if (/(^|\/)(helpers|fixtures|__fixtures__|__snapshots__)\//.test(rel)) return false; - if (matchesAny(declared, rel)) return true; + if (!/^\.(gstack|github)\//.test(rel) && matchesAny(declared, rel)) return true; if (!matchesAny(CARRY_TEST_CODE_GLOBS, rel)) return false; return /\.(test|spec)\.[^/]+$|_test\.go$/.test(rel) || /(^|\/)(test|tests|spec|__tests__|src\/test|[^/]+Tests)\//.test(rel); } diff --git a/lib/ship-sidecar.ts b/lib/ship-sidecar.ts index 1dffd1f2a..2e38cc3d0 100644 --- a/lib/ship-sidecar.ts +++ b/lib/ship-sidecar.ts @@ -9,7 +9,8 @@ * the script for every uid:// reference to it); * - its sibling (the path without `.uid`) is a regular file now, and one the reviewed range added or modified, or one * the review-loop exception accepted; - * - it is a regular, non-executable file whose whole content is exactly what Godot writes for one id; + * - it is a regular, non-executable file whose whole content is exactly what Godot writes for one id, and no other file + * in the current tree names that uid:// (a copied sidecar gives two resources one UID); * - neither it nor its sibling matches the base policy's review_exclude_paths or readers.forbid_paths (a sibling no reader * saw is not reviewed). * Any other `.uid` is refused with the reason and stays behavior; a `.import` file is never a candidate. @@ -48,6 +49,13 @@ function git(cwd: string, args: string[]): string | null { return r.status === 0 ? r.stdout : null; } +/** The other files in `tree` that name `uid` as a whole word; null when git cannot search it. */ +function holders(cwd: string, tree: string, uid: string, self: string): string[] | null { + const r = spawnSync('git', ['-c', 'core.quotepath=off', 'grep', '-z', '-l', '-w', '-F', '-e', uid, tree], { cwd, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); + if (r.status !== 0 && r.status !== 1) return null; + return r.stdout.split('\0').filter(Boolean).map(l => l.slice(tree.length + 1)).filter(p => p !== self); +} + /** The mode and blob of `rel` in `tree`; null when absent or unreadable. */ function entry(cwd: string, tree: string, rel: string): { mode: string; blob: string } | null { const m = /^(\d{6}) \w+ ([0-9a-f]{40,64})\t/.exec(git(cwd, ['--literal-pathspecs', 'ls-tree', '-z', tree, '--', rel]) ?? ''); @@ -79,6 +87,9 @@ export function uidSidecars(cwd: string, i: SidecarInput): { proven: Record line'); continue; } + const shared = holders(cwd, s.current, text.trim(), p); + if (shared === null) { refuse('could not be searched for its UID in the current tree'); continue; } + if (shared.length) { refuse(`shares its UID with ${shared.sort().join(', ')}`); continue; } if (excluded.has(p) || excluded.has(sibling)) { refuse('or its sibling matches the base policy\'s review_exclude_paths or readers.forbid_paths, so no reader saw the script'); continue; } proven[p] = sibling; } diff --git a/test/ship-uid-sidecar.test.ts b/test/ship-uid-sidecar.test.ts index 7febc4c3a..e1504b8da 100644 --- a/test/ship-uid-sidecar.test.ts +++ b/test/ship-uid-sidecar.test.ts @@ -105,6 +105,7 @@ describe('a Godot UID sidecar added after the fix-check (GST-1604: founderstar#6 [SIDECAR, 'is not a regular non-executable file (mode 120000)', f => fs.symlinkSync('cutscene_check.gd', path.join(f.work, SIDECAR))], [SIDECAR, 'is not a regular non-executable file (mode 100755)', f => { f.write({ [SIDECAR]: UID }); fs.chmodSync(path.join(f.work, SIDECAR), 0o755); }], ['scripts/ui/cutscene.gd.uid', 'existed at the reviewed tree or the current base', f => f.write({ 'scripts/ui/cutscene.gd.uid': 'uid://b4w2kq0ly8s5n\n' }), { 'scripts/ui/cutscene.gd.uid': UID }], + [SIDECAR, 'shares its UID with scripts/old.gd.uid', f => f.write({ [SIDECAR]: UID }), { 'scripts/old.gd': 'extends Node\n', 'scripts/old.gd.uid': UID }], ]; for (const [p, why, after, base, feat] of cases) { const r = founderstar611({ after, base, feat }).run(); @@ -141,6 +142,9 @@ describe('base-declared test_code (GST-1604: GDScript tests)', () => { expect(isTestCode('tests/helpers/fake_input.gd', ['tests/**/*.gd'])).toBe(false); expect(isTestCode('scripts/ui/cutscene.gd', ['tests/**/*.gd'])).toBe(false); expect(isTestCode('src/a.test.ts', ['tests/**/*.gd'])).toBe(true); + expect(isTestCode('.github/workflows/release.yml', ['**/*.yml'])).toBe(false); + expect(isTestCode('.gstack/ship-policy.yml', ['**/*.yml'])).toBe(false); + expect(isTestCode('tests/fixtures.yml', ['**/*.yml'])).toBe(true); }); test('the key validates as a glob list behind min_gstack_version, never under .gstack/ or .github/', () => {