Repository navigation
fix: run the version floors in generate/check/upgrade; normalize the bgfx remote; floor the 2.x core line (#739, #742) - #746
Conversation
…e bgfx remote, floor the 2.x core line (#739, #742) Two holes left by #736's `init`-only validators, closed against the same tables — no second copy of the floors. #742.1 — `isOfficialBgfx` compared the RAW repo string, so the `https://…/labelle-bgfx.git` spelling of the official repo (which the fetch path clones happily) was classified as a CUSTOM provider, skipped `contractViolation`, and failed at the generated `MATERIAL_CONTRACT_VERSION` guard instead of at generate with the pin named. `config.normalizeRemote`/`sameRemote` strip the scheme and a `.git` suffix and case-fold the host; `local:` paths compare verbatim. #742.2 — the trio rules all keyed off a NEW gfx or engine, so `init --engine-version=2.12.2 --gfx-version=1.30.1` under the default core 2.0.0 scaffolded a set the table's own docs call incompatible. Two reverse rules (core >= 2.0.0 floors engine >= 3.0.0 and gfx >= 2.0.0), ordered after the gfx/engine-subject rules so an old core under a new gfx keeps naming gfx as the subject. #739 — the floors moved from `init_cmd.zig` to `version_floors.zig` (which also drops init_cmd under the 1000-line line) and now run at every door that writes or resolves the pairing: * `generate`/`check` — `configBackendCoreFloorViolation` resolves the EFFECTIVE backend package, so an explicit `.backend_package` (which `init` has no flag for, so it was unreachable before) is judged too, in every spelling of the official remote. A non-official provider carries its own semver and is left to the generated contract guard. * `upgrade` — validated on the PROSPECTIVE pins, before the rewrite lands, so a refusal leaves project.labelle untouched. It refuses only when the upgrade moves a floor-bearing pin: `upgrade cli` on an already-below-floor project warns and proceeds rather than blocking an unrelated bump, and `upgrade all` still REPAIRS such a project. `enforce` splits into a pure `verdict` (so tests assert which table refused, without emitting error logs) and the logging wrapper the commands call. The #736 severity split is unchanged: compile break refuses, curated floor warns. Verified STANDALONE (whole checkout mirrored outside the toolkit, scratch LABELLE_HOME, registry packages), each with a negative control on the pre-fix binary: * bgfx 0.20.0 via `https://…/labelle-bgfx.git` + materials/: before generate exit 0; after, refused naming the 0.20.0 pin. * `init --engine-version=2.12.2 --gfx-version=1.30.1`: before exit 0 and a scaffolded project; after exit 2, nothing written, all three pins + the floor named. * hand-edited core 1.32.0 / engine 2.12.2 / gfx 1.30.1 under builtin bgfx 0.21.0: before generate exit 0 and `zig build` died with "root source file struct 'root' has no member named 'shader_material'" inside labelle-bgfx; after, refused at generate. * `upgrade core 1.32.0`: before rewrote the pin; after exit 2, unchanged. * Regression: default `init --backend=bgfx` and the condenser / material-demo / pack-city examples generate AND fully `zig build` (exit 0); tile-explorer, sprite-runner, camera-builtin generate. Tests: 2926/2938 passed (12 skipped), exit 0 — +9 over the 2917/2929 baseline; all six new tests force-failed and confirmed collected. Closes #739 Closes #742 Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XBGRLojiysEr5zy1U5mbP6
|
Warning Review limit reached
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Next included review available in 5 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 838744472d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bp = cfg.effectiveBackendPackage() orelse return null; | ||
| if (!isOfficialProvider(cfg.backend, bp)) return null; | ||
| if (!config.isSemverVersion(bp.version)) return null; | ||
| return providerCoreFloorViolation(cfg.backend, bp.version, cfg.core_version); |
There was a problem hiding this comment.
When a project selects the official bgfx repository solely through .backend_package, ProjectConfig specifies that the default .backend enum is ignored, but this code still compares the package against builtinProvider(cfg.backend) and then uses that enum to choose the floor table. With the normal default .backend = .raylib, an explicit bgfx 0.21.0 package paired with core 1.32.0 is therefore classified as non-official, so generate, check, and upgrade all miss the compile-breaking bgfx/core floor. Determine the provider type from the resolved package repository rather than the ignored enum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 582d0c1. isOfficialProvider(cfg.backend, bp) is replaced by officialBackendOf(bp), which walks the Backend values, compares bp.repo against each builtin provider's repo through sameRemote, and returns the tag the PACKAGE identifies. configBackendCoreFloorViolation selects the floor table with that, never with cfg.backend.
Standalone negative control (checkout mirrored outside the toolkit tree with a scratch LABELLE_HOME, so sibling checkouts cannot mask the pins) — your exact config, .backend left at its default:
.backend_package = .{ .name = "bgfx", .repo = "github.com/labelle-toolkit/labelle-bgfx", .version = "0.21.0" },
.core_version = "1.32.0",
| binary | labelle-assembler check |
|---|---|
| pre-fix | exit 0 — labelle check: no packs found — nothing to lint |
| post-fix | exit 2 — bgfx 0.21.0 requires labelle-core >= 2.0.0; got 1.32.0 … Pass --core-version=2.0.0 |
The new unit test asserts the mechanism, not just the verdict: officialBackendOf(bp) must return .bgfx, and it pins that the bgfx repo really does differ from the raylib remote so the test cannot pass for the wrong reason. A genuine third-party provider (github.com/acme/bgfx) is still left alone.
| pub fn trioFloorViolation(core_version: []const u8, engine_version: []const u8, gfx_version: []const u8) error{UnparsableVersionPin}!?TrioViolation { | ||
| if (!config.isSemverVersion(core_version) or !config.isSemverVersion(engine_version) or !config.isSemverVersion(gfx_version)) return null; |
There was a problem hiding this comment.
If any one of the three pins is non-semver, this early return suppresses every trio rule, including rules whose subject and requirement are both known release pins. For example, core_version = "local:../core", engine 3.0.0, and gfx 1.30.1 bypasses validation even though the table independently declares that engine 3.x requires gfx >= 2.0.0; likewise, a local gfx pin hides the known engine-3/core-1 incompatibility. Skip only rules whose own subject or required pin cannot be parsed so generate, check, and upgrade still reject the provably incompatible release pair.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 582d0c1. The blanket early return is gone; each rule is skipped only when ITS OWN subject or requirement pin is unparseable (a rule reads exactly two of the three pins — the third is only quoted in the diagnostic). Decidability is per rule, not per trio.
Standalone negative control, your first example verbatim:
.core_version = "local:../labelle-core", .engine_version = "3.0.0", .gfx_version = "1.30.1"
| binary | labelle-assembler check |
|---|---|
| pre-fix | exit 0 |
| post-fix | exit 2 — labelle-engine 3.0.0 requires labelle-gfx >= 2.0.0; got core local:../labelle-core / engine 3.0.0 / gfx 1.30.1 … Pass --gfx-version=2.0.0 |
Your second example is covered too: a local GFX pin with core 2.0.0 / engine 2.12.2 now reports the core-2.x/engine-3.x break. The new test asserts WHICH rule fired (subject and requirement package, plus severity), not merely that something was returned, and pins the negative side — an undecidable trio (local: on all three) and a local: pin over a coherent pair both still return null, so the change cannot invent violations.
One existing assertion encoded the old blanket behaviour: trioFloorViolation("local:../labelle-core", "2.12.2", "2.0.0") expected null, but gfx 2.0.0 under engine 2.12.2 is a compile break the table states without reference to core. It now asserts that violation, with the coherent 3.0.0/2.0.0 pair kept beside it as the negative control.
Full suite 2928/2940, exit 0 (+2 tests); force-failing each takes it red.
| try version_floors.enforce(cfg, "labelle-assembler generate"); | ||
|
|
||
| const backend_manifest_name = manifest_detect.detectV2ManifestName(allocator, cfg, game_dir); | ||
| try manifest_splice.requireManifestIfExternal(allocator, cfg, game_dir, backend_manifest_name); |
There was a problem hiding this comment.
For an external provider that both lacks its required manifest and has incompatible release pins, this newly inserted floor check exits before requireManifestIfExternal, despite the surrounding invariant that a manifest-less external must report that clearer configuration error first. Such a project is told to change framework versions even though the selected provider is unusable regardless, and only discovers the missing-manifest problem afterward. Move the floor enforcement after requireManifestIfExternal while keeping both checks ahead of target-file emission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 582d0c1 — version_floors.enforce now runs AFTER detectV2ManifestName + requireManifestIfExternal, still ahead of validateProviderContracts and of any target-file emission. The invariant is spelled out at the call site: a configuration error that invalidates the provider outright precedes an error about how the provider is pinned.
Standalone negative control — a manifest-less local: external backend plus a trio compile break (core 2.0.0 / engine 2.12.2 / gfx 1.30.1), labelle-assembler generate:
| binary | first error |
|---|---|
| pre-fix | labelle-core 2.0.0 requires labelle-engine >= 3.0.0 … → generate failed: VersionFloorViolation |
| post-fix | external backend 'acme' … ships no backend.manifest.zon → generate failed: ExternalBackendNeedsManifest |
And the reorder did not drop the floor: dropping a backend.manifest.zon into that provider and re-running post-fix brings back VersionFloorViolation with the same diagnostic, exit 1.
…on, restore the manifest-first ordering, evaluate decidable trio rules (#746 review) Four review findings, all verified standalone against a pre-fix binary (mirrored checkout outside the toolkit tree with a scratch LABELLE_HOME, so sibling checkouts cannot mask the pins). 1. [P1] Explicit providers were identified via the IGNORED `.backend` enum. When a project selects the official bgfx repo solely through `.backend_package`, `ProjectConfig` documents that `.backend` is ignored and it sits at its meaningless `.raylib` default — yet `isOfficialProvider(cfg.backend, bp)` compared the bgfx package against the RAYLIB remote, answered "third party", and dropped the floor. `officialBackendOf(bp)` now derives the backend from the resolved package alone. Control: bgfx 0.21.0 + core 1.32.0 via `.backend_package` — `check` pre-fix exit 0 ("nothing to lint"), post-fix exit 2 naming `bgfx 0.21.0 requires labelle-core >= 2.0.0`. 2. [P2] `normalizeRemote` did not strip `?query`/`#fragment` (or a `git+` prefix outside the one `git+https://` spelling), while the fetch path's `repoHostPath` did — so `git+https://github.com/labelle-toolkit/labelle-bgfx?ref=main` FETCHED the official backend and was then judged custom. The divergence was the bug, so `repoHostPath` now delegates to `config.normalizeRemote`: one normalizer, and the identity a gate compares is by construction the identity the fetcher resolves. Control: that exact repo string, `.backend = .bgfx` so finding 1 cannot confound — pre-fix exit 0, post-fix exit 2. 3. [P2] The floor check ran BEFORE `requireManifestIfExternal`, so an external provider that both lacked its manifest and carried bad pins was told to change framework versions while the real blocker — a provider unusable at any version — went unmentioned. Moved after it, still ahead of any target-file emission. Control: manifest-less `local:` provider + a trio compile break — pre-fix reports VersionFloorViolation, post-fix reports ExternalBackendNeedsManifest; adding the manifest brings the floor error back, so the reorder did not drop the check. 4. [P2] One non-semver pin suppressed EVERY trio rule. Each rule reads exactly two pins, so decidability is per rule: a rule is skipped only when its own subject or requirement is unparseable. Control: `core_version = "local:../labelle-core"`, engine 3.0.0, gfx 1.30.1 — pre-fix exit 0, post-fix exit 2 naming `labelle-engine 3.0.0 requires labelle-gfx >= 2.0.0`. Two in-tree assertions that encoded the old blanket behaviour were updated and now assert WHICH rule fires, not merely that the result is null. `zig build test --summary all`: 2928 pass / 2940 total, exit 0 (was 2926/2938 — +2, the new tests). Force-failing each takes the suite to 2 fail, exit 1. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XBGRLojiysEr5zy1U5mbP6
|
All four review findings addressed in 582d0c1. Every fix was verified standalone — the checkout mirrored outside
Notes on the shape of each fix:
Tests: No overlap with |
Closes #739. Closes #742.
Both issues harden the floor validators #736/#733 added to
init, and both touch the same tables — one PR.The tables themselves moved from
init_cmd.zigto a newsrc/version_floors.zig(which also dropsinit_cmd.zigfrom 1089 to 575 lines).init_cmdre-exports the public names, so there is still exactly ONEbgfx_core_floorsand ONEtrio_floors; every door now runs them.Gap 1 (#742.1) —
isOfficialBgfxcompared the raw repo stringparseProjectConfigkeepsPluginDep.repoverbatim and the fetch path clonesgithub.com/…/labelle-bgfx,https://github.com/…/labelle-bgfxand….gitalike — but the materials gate compared against the bare literal, so an alternate spelling of the OFFICIAL repo was treated as a custom provider and skippedcontractViolation.BEFORE (standalone, pre-fix binary) —
examples/condenser(ownsmaterials/) with.backend_package = .{ … .repo = "https://github.com/labelle-toolkit/labelle-bgfx.git", .version = "0.20.0" }:FIX —
config.normalizeRemote/config.sameRemote: strip the scheme, strip a trailing/and.git, case-fold the host, compare the path exactly.local:paths are returned untouched and compare verbatim.material_pipeline.isOfficialBgfx(and the new generate-time gate) use it.AFTER:
A genuinely custom provider (
github.com/acme/…,local:…) is still left to the generated contract guard — the normalization does not widen the gate.Gap 2 (#742.2) — no reverse floors for the 2.x core line
Every trio rule keyed off a NEW gfx or engine, so the new CORE against an old engine/gfx passed. Added, ordered after the gfx/engine-subject rules so an old core under a new gfx keeps naming gfx as the subject:
core >= 2.0.0⇒engine >= 3.0.0(compile break)core >= 2.0.0⇒gfx >= 2.0.0(compile break)BEFORE (standalone, pre-fix):
AFTER:
Gap 3 (#739) — the validators beyond
initgenerate/checkversion_floors.enforce(cfg, …)runs ingenerate()before any target file is written, and incmdCheckbefore the lint.configBackendCoreFloorViolationresolveseffectiveBackendPackage()— an explicit.backend_package, else the builtin provider the.backendtag is shorthand for — so the explicit-package case, whichinithas no flag for and could never reach, is covered here.BEFORE (standalone, pre-fix) — a project hand-edited to core 1.32.0 / engine 2.12.2 / gfx 1.30.1 under the builtin bgfx 0.21.0:
AFTER:
upgrade— a different shapegenerate/checkjudge a config that already exists;upgrademust judge the config it is about to write. The command was restructured to decide every prospective pin first (next_core/next_engine/next_gfx/next_cli), validate, and only then rewrite — so a refusal leavesproject.labellebyte-identical.One further difference: the gate refuses only when the upgrade actually moves a floor-bearing pin.
upgrade clion a project that is already below a floor cannot make it worse, and blocking it would stop an unrelated bump — so that case warns and proceeds.upgrade all/ bareupgradestill snap the trio to the curated set, which repairs such a project.BEFORE / AFTER (standalone):
Verification method
The assembler prefers SIBLING checkouts over pins (
findRepoRootwalks up from the executable), so an in-tree run proves nothing. Every result above comes from the whole checkout mirrored outside the toolkit tree (…/scratchpad/pre|post/labelle-assembler, binary included), driven with a scratchLABELLE_HOMEso packages are fetched from the registry at the pinned tags. Each fix has a negative control: the pre-fix binary exhibits the bug, the post-fix binary does not. Every generate log was grepped forgenerate failed, and exit codes were captured separately from any pipeline.Regression evidence (standalone, post-fix,
zig build— not generate-only)zig buildinit --backend=bgfx(defaults)examples/condenser(bgfx 0.21.0 + materials)examples/material-demo(bgfx 0.20.0 / core 1.32.0)examples/pack-city(bgfx 0.20.0 / core 1.32.0)examples/tile-explorer(raylib)examples/sprite-runner(sokol)examples/camera-builtin(null)(#732: CI stops at generate for bgfx games, which is how this class of break hides — hence the full builds.)
Tests
zig build test --summary all→ 2926/2938 passed (12 skipped), exit 0 (baseline onmain: 2917/2929, exit 0) — +9.zig build test-materials→ exit 0. Note thetest-materialsname filter does not collectmaterial_pipelinetests (test-materials step never runs material_pipeline tests (name filter excludes them) #741), so the new ones were checked through the full suite.New tests: remote normalization (every accepted spelling official, forks/other hosts/case-differing paths still custom,
local:untouched);isOfficialBgfx+contractViolationon thehttps://….gitspelling; the 2.x reverse floors (including that an old core under a new gfx still names gfx); the resolved-config gate for the builtin provider and an explicit.backend_package, asserting WHICH table refused and that a curated floor warns rather than refusing.https://claude.ai/code/session_01XBGRLojiysEr5zy1U5mbP6
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.