Repository navigation
test: guard src/ against platform, store and package names (RFC #406 enforcement) - #417
Conversation
…enforcement) Add src/agnostic_guard_test.zig, modelled on labelle-bgfx's heap guard: it walks every .zig file under src/, splits each into [A-Za-z0-9]+ runs and flags platform/store/package/backend names (android, ios, wasm, emsdk, emscripten, steam, itch, xcode, gradle, apk, aab, ndk, raylib, sokol, sdl, sdl2, bgfx, wgpu) case-insensitively. Host OS names (macos, windows, linux, darwin, win32) are permanently allowed. Files that still carry legacy platform code sit on a migration allowlist (43 entries: 42 files plus the cli/android/ prefix, recomputed on this branch) that can only shrink: the test also fails when an allowlisted file is clean, so the entry must be removed in the same change. build.zig wires it into `zig build test` via addAgnosticGuard (host target, cwd = repo root) and adds a standalone `zig build test-guard`. The RFC's Enforcement section points at the file and the shrink rule. Claude-Session: https://claude.ai/code/session_01R2K5sJDL5Msn1uRTkybiDP
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 32d8c27cdc
ℹ️ 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".
| var scan: Scan = .{ .gpa = gpa }; | ||
| defer scan.deinit(); | ||
| while (try walker.next(io)) |entry| { | ||
| if (entry.kind != .file or !std.mem.endsWith(u8, entry.path, ".zig")) continue; |
There was a problem hiding this comment.
The .zig filter leaves compiled C sources and headers completely outside the guard, despite the RFC requiring forbidden names not to appear under src/. This already produces a false clean result: src/cli/stb_image_impl.c:6 contains bgfx, while src/cli/stb_image.h:270 contains iOS and Android, but neither file is explicitly allowlisted or reported. Scan applicable C/header files as well and place existing exceptions on the migration allowlist.
Useful? React with 👍 / 👎.
| "cli.zig", | ||
| "cli/add.zig", | ||
| "cli/android.zig", | ||
| "cli/android/", |
There was a problem hiding this comment.
Using a directory-prefix allowlist means every future .zig file under src/cli/android/ is automatically exempt, even though it was never part of the legacy baseline; the self-test explicitly confirms this with not_yet_written.zig. As long as any one file in the directory contains a forbidden token, the single dirty bit also prevents the prefix from becoming stale, so new platform-specific code can accumulate while the documented “file allowlist can only shrink” check remains green. List the eight existing files individually instead.
Useful? React with 👍 / 👎.
| while (self.pos < self.text.len and isWordByte(self.text[self.pos])) self.pos += 1; | ||
| if (classify(self.text[start..self.pos])) |word| return word; |
There was a problem hiding this comment.
Treating the entire alphanumeric run as one token misses the idiomatic Zig names most likely to carry these identities, such as AndroidProvider, WasmConfig, or IosConfig. This already lets src/cli/config.zig:78 mention the non-allowlisted IosConfig while the guard reports no offender, and an allowlisted file can similarly become “clean” while retaining platform-specific types. Recognize case transitions/acronym boundaries before comparing against the forbidden names.
Useful? React with 👍 / 👎.
| for (scan.offenders.items) |o| std.debug.print("{s}\n", .{o}); | ||
| for (stale.items) |s| std.debug.print("src/{s} is on the migration allowlist but is clean; remove it\n", .{s}); | ||
| // The walk really covered the tree (a wrong cwd would scan nothing and pass). | ||
| try std.testing.expect(scan.scanned > 60); |
There was a problem hiding this comment.
The guard fails every otherwise-clean checkout once src/ contains 60 or fewer Zig files, even though that is a valid outcome of the RFC's planned extraction of platform code into provider packages. The current tree has only 79 such files, so moving or consolidating enough legacy modules will make zig build test fail solely because of this magic number. Verify a stable repository sentinel instead; opening src already prevents the ordinary wrong-working-directory case from passing vacuously.
Useful? React with 👍 / 👎.
Four Codex findings, all valid against the tree merged with development: 1. Scan C sources and headers too. The walk now reads `.zig`, `.c` and `.h` under src/, so the vendored stb files are covered; the JSON wire fixtures under cli/provider_contract/ stay out of scope as data, not source (documented in the header). New allowlist entries: cli/stb_image.h, cli/stb_image_impl.c. 2. Enumerate the eight cli/android/*.zig files individually. Directory prefix support is removed from the allowlist matcher, so a new file under that directory is no longer exempt; the self-test now asserts `not_yet_written.zig` is NOT covered. 3. Tokenize CamelCase. Alphanumeric runs split at lowercase->uppercase and digit->uppercase transitions and at acronym boundaries, digits staying attached (IosConfig -> ios; SDL2Provision -> sdl2, provision; getSDLPath -> get, sdl, path), while std.Io, Iostream, biosphere and wasmtime do not flag. Self-tests cover exactly these. Newly dirty: cli/config.zig (IosConfig). 4. Replace the `scanned > 60` threshold with a stable sentinel: the walk must see src/cli.zig (the exe root in build.zig) or the test fails with a clear message instead of passing vacuously. Allowlist recomputed: 53 file entries, shrink-only rule and message formats unchanged. Claude-Session: https://claude.ai/code/session_01R2K5sJDL5Msn1uRTkybiDP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e9d09080
ℹ️ 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".
| fn file(self: *Scan, path: []const u8, bytes: []const u8) !void { | ||
| if (std.mem.eql(u8, path, cli_root)) self.saw_cli_root = true; | ||
| const allowed = allowedIndex(path); | ||
| var line_no: usize = 0; |
There was a problem hiding this comment.
Tokenization is applied only to bytes, while path is used solely for allowlist lookup. Consequently, a clean file such as src/cli/android/run.zig is considered stale and, after its allowlist entry is removed, passes despite retaining a forbidden platform name in the core tree; a newly added src/cli/steam/upload.zig can pass for the same reason. Scan the relative path as well as the contents so moving or renaming platform-specific files remains part of the migration.
Useful? React with 👍 / 👎.
| if (!std.ascii.isUpper(cur)) return false; | ||
| if (std.ascii.isLower(prev) or std.ascii.isDigit(prev)) return true; | ||
| return i + 1 < text.len and std.ascii.isLower(text[i + 1]); |
There was a problem hiding this comment.
When a source contains the conventional spelling iOS (or an identifier such as iOSConfig), the lowercase-to-uppercase rule splits it into i and OS, so neither piece matches the forbidden ios entry. This lets new, non-allowlisted code name the iOS platform while the guard stays green; classify exact runs or otherwise handle lowercase-leading acronyms before applying CamelCase boundaries.
Useful? React with 👍 / 👎.
| "android", "ios", "wasm", "emsdk", "emscripten", "steam", | ||
| "itch", "xcode", "gradle", "apk", "aab", "ndk", | ||
| "raylib", "sokol", "sdl", "sdl2", "bgfx", "wgpu", |
There was a problem hiding this comment.
The RFC explicitly moves the web implementation into the labelle-web provider and says only host OS names should remain after migration, but neither web nor labelle-web can match this table. A new non-allowlisted core file can therefore contain web, WebProvider, or labelle-web and still pass the guard, even though the equivalent Android/provider identities are rejected.
Useful? React with 👍 / 👎.
Three Codex findings on b4e9d09: - Scan the relative path too. Directory and file-name segments go through the same tokenizer and report as `src/<path>: '<word>' in path (...)`, so a clean-bodied `src/cli/steam/upload.zig` is a finding and the `cli/android/*.zig` and `cli/ios.zig` entries stay dirty until the file is moved or renamed, not merely emptied of platform words. - Lowercase-leading acronyms. Each `[A-Za-z0-9]+` run is classified whole before it is split into CamelCase pieces, and a run that opens with one lowercase letter followed by two or more uppercase ones keeps its acronym as one piece: `iOS` -> `ios`, `iOSConfig` -> `iOS|Config`, `iosConfig` -> `ios|Config`, while `iostream`, `std.Io` and `IoReader` still pass. - `web` joins the forbidden set (RFC #406 moves it to the `labelle-web` provider); `wasm`/`emsdk` stay. The allowlist was recomputed against the tree merged with development (#415): still 53 entries, every file the new rules reach was already listed. Self-tests cover the path hit, the acronym cases, `web`, and the shrink-only rule under the path scan. Claude-Session: https://claude.ai/code/session_01R2K5sJDL5Msn1uRTkybiDP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e8d7c6119
ℹ️ 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".
| if (run.len > max_word_len) return null; | ||
| var buf: [max_word_len]u8 = undefined; | ||
| const lower = std.ascii.lowerString(buf[0..run.len], run); | ||
| for (allowed_words) |a| if (std.mem.eql(u8, a, lower)) return null; | ||
| for (forbidden) |f| if (std.mem.eql(u8, f, lower)) return f; |
There was a problem hiding this comment.
Because classification requires an exact table match and digits remain attached to the preceding token, common platform identities such as wasm32, android14, and sdl3 all return clean. A new non-allowlisted source file can therefore hard-code wasm32—the architecture used by the web provider—without tripping this guard; preserve exact allowed forms such as win32, but classify a forbidden root before an all-numeric suffix.
Useful? React with 👍 / 👎.
| const forbidden = [_][]const u8{ | ||
| "android", "ios", "web", "wasm", "emsdk", "emscripten", "steam", | ||
| "itch", "xcode", "gradle", "apk", "aab", "ndk", "raylib", | ||
| "sokol", "sdl", "sdl2", "bgfx", "wgpu", |
There was a problem hiding this comment.
The forbidden table includes ios but omits the related target platforms tvos, watchos, visionos, and maccatalyst, even though these exact spellings already occur in src/astc/cmd.zig. A new non-allowlisted core file can therefore branch on any of those provider-specific targets while the guard remains green; unlike macos, these are not supported host OS names covered by the permanent exception.
Useful? React with 👍 / 👎.
Round-3 review on #417 (two Codex findings): - A token spelled `<letters><digits>` (`wasm32`, `android14`, `sdl3`, `bgfx2`, the CamelCase piece `Wasm32` of `Wasm32Target`) returned clean because classification was an exact table match. `classify` now tries the exact spelling first and then the letter root before an all-digit suffix. The permanent host allowlist keeps its exact forms: `win32` is allowed as spelled and `win` is not forbidden, so `win32`, `x86_64`, `utf8`, `base64` and `sha256` stay clean; a self-test asserts both sides. - `tvos`, `watchos`, `visionos` and `maccatalyst` join `ios` on the forbidden table; `macos` stays a permanently allowed host. Allowlist recomputed: unchanged (53 entries); every file the new rules reach (`astc/cmd.zig`, `cli/doctor.zig`, `cli/sdl_provision.zig`, ...) was already listed, and no entry went stale. Claude-Session: https://claude.ai/code/session_01R2K5sJDL5Msn1uRTkybiDP
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01a39b3a39
ℹ️ 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".
| test "the tokenizer flags whole alphanumeric runs, case-insensitively" { | ||
| try expectWords("std.Io ios_cmd biosphere Android cli/android/run.zig SDL2_image", &.{ "ios", "android", "android", "sdl2" }); | ||
| // Substrings inside a longer lowercase run never match: `wasm` is not a word here. | ||
| try expectWords("wasmtime bgfxdebug libsdl", &.{}); |
There was a problem hiding this comment.
When a new core file uses conventional provider-specific names such as Steamworks or the Linux package libsdl2-dev, the tokenizer sees the runs Steamworks and libsdl2; CamelCase splitting does not extract steam, and numeric-root classification reduces the latter only to libsdl. Both therefore pass even though the RFC assigns Steamworks and SDL provisioning to providers. Add these known compound/package forms or recognize their conventional affixes rather than treating every libsdl run as clean.
Useful? React with 👍 / 👎.
| var scan: Scan = .{ .gpa = gpa }; | ||
| defer scan.deinit(); | ||
| while (try walker.next(io)) |entry| { | ||
| if (entry.kind != .file or !isSource(entry.path)) continue; |
There was a problem hiding this comment.
On platforms where the directory walker reports symlinks as .sym_link, a committed link such as src/cli/steam.zig is skipped by this condition even though Zig can import it and read its target. Consequently both the forbidden path and any platform-specific contents behind a source symlink bypass the guard; follow source links safely or fail when a source-shaped symlink is encountered.
Useful? React with 👍 / 👎.
| "android", "ios", "web", "wasm", "emsdk", "emscripten", "steam", | ||
| "itch", "xcode", "gradle", "apk", "aab", "ndk", "raylib", | ||
| "sokol", "sdl", "sdl2", "bgfx", "wgpu", "tvos", "watchos", | ||
| "visionos", "maccatalyst", |
There was a problem hiding this comment.
The RFC explicitly assigns the butler executable to the itch provider and UIKit glue to the iOS provider, but neither butler nor uikit appears in this table. A new non-allowlisted core file can therefore invoke butler or reference UIKit without using the words itch or ios, and the guard reports it as clean; add the provider-specific tools and SDK identities already enumerated by the RFC.
Useful? React with 👍 / 👎.
|
Merged on green after three review-fix rounds. The three remaining P2 findings from the last pass are tracked in #419. |
What
PR 3 of the RFC #406 follow-up: the agnosticism guard the RFC's "Enforcement" section promises, modelled on labelle-bgfx's
src/heap_guard_test.zig.Mechanism
src/agnostic_guard_test.zigwalks every.zigfile undersrc/at test time (cwd pinned to the repo root bybuild.zig), splits each line into[A-Za-z0-9]+runs, lowercases them and compares against the forbidden table:android ios wasm emsdk emscripten steam itch xcode gradle apk aab ndk raylib sokol sdl sdl2 bgfx wgpu. Everything else is a word boundary, soios_cmdandcli/android/flag whilestd.Ioandbiospheredo not. Comments count (the mandate is textual). Host OS names (macos windows linux darwin win32) are permanently allowed; a comptime check keeps the two tables disjoint.A finding prints as:
The test also fails when fewer than 60 files were scanned (a wrong cwd must not pass vacuously).
Allowlist: 43 entries, shrink-only
Files that still carry legacy platform code sit on
allowed_files: 42 files plus thecli/android/directory prefix (covering 8 files), recomputed by running the scanner on this branch rather than copied from the plan. Compared with the plan's list, three more files are dirty here:cli/linux_desktop.zig,cli/progress.zig,cli/status.zig(all namebgfx/raylib/sokol).The list can only shrink: an allowlisted file that no longer contains any forbidden word fails the test with
src/<f> is on the migration allowlist but is clean; remove it. New files (PR 1'sprovider_hooks.zig, PR 2'sprovider_targets.zig) are not on it and must stay clean.Self-tests (mechanism, not just value)
std.Io ios_cmd biosphere Android cli/android/run.zig SDL2_imageyields exactlyios android android sdl2;wasmtime bgfxdebug libsdlyields nothingcli\android\run.zig) and directory prefixes, and rejectscli/androidx/…and the provider filesScanproduces the documented finding lines for a non-allowlisted file and marks a clean allowlisted entry staleBuild wiring
addAgnosticGuard(b, test_step, optimize)mirrors bgfx'saddHeapGuard(host target,setCwd(b.path("."))): part ofzig build teston all three CI hosts, plus a standalonezig build test-guard.Verification
zig build test-guard: exit 0 (5/5)zig build test: exit 0 (13/13 steps, 964 passed, 5 skipped)// … Android …to the cleansrc/cli/provider_manifest.zigmakeszig build test-guardexit 1 withsrc/cli/provider_manifest.zig:156: 'android' (…); revertedDocs:
docs/rfc-package-commands.md"Enforcement" now points at the file and states the shrink rule. The RFC's acceptance check "guard passes with only host OS names" stays open until extraction.https://claude.ai/code/session_01R2K5sJDL5Msn1uRTkybiDP
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.