Repository navigation
packs: enforce cross-pack @import ACL (rule 4) — #491 - #505
Conversation
`.exposes`/`.depends_on` were parsed but the ACL was taste-enforced. #441 already gates the DAG (unknown-dep + acyclic); this adds the one enforcement piece that is tractable in the assembler WITHOUT Zig semantic/call-graph analysis: a textual `labelle check` rule that flags a pack `@import`ing a source file inside ANOTHER pack's directory when that pack is not declared in its `depends_on` (and is not `contracts`). This closes the blatant, literal form of the direct-TYPE-import reach-around engine #656 identified as the real ACL escape (recovering a foreign type by importing its file, sidestepping module isolation AND the string-keyed registry rule-1 catch). Detection resolves each relative `.zig` `@import` lexically against the importing file's dir and tests containment against the canonical foreign pack dirs; an import of a DECLARED dependency's file stays allowed (packs share a copied tree with no module wall yet, RFC §6). Deliberately NOT attempted: full "a pack only CALLS exposed fns" enforcement, and semantic reach-around routes (re-export chains, a type reached through a value, imports via a named module) — those need call-graph analysis the assembler does not do (engine #656). `.exposes`-symbol resolution is also left out: it has zero consumers/examples today and #487 scans script FILENAMES only (no pub-fn symbol table), so a resolution check would encode an undefined contract and risk false positives during partial surface migration. - new Rule.cross_pack_import (+slug) and ForeignPack/Context plumbing in check.zig - check_cmd builds the per-pack foreign-pack view (allowed = declared dep or contracts) - 8 tests: 6 pure scanSource cases + 2 e2e runLint fixtures (real-path resolution) Claude-Session: https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new pack-lint rule ("rule 4": cross_pack_import) to check.zig that flags relative .zig ChangesCross-pack import lint rule
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
![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: 53973ff91b
ℹ️ 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 (ctx.current_pack_dir.len == 0) return; // rule disabled (unit-test path) | ||
| if (ctx.foreign_packs.len == 0) return; // nothing foreign to reach | ||
|
|
||
| const path = decodeStringLiteral(src[path_tok.start..path_tok.end]) orelse return; |
There was a problem hiding this comment.
When a pack uses a valid Zig string escape in the import path, for example @import("..\x2f..\x2fproduction/events/item_produced.zig"), decodeStringLiteral returns null and rule 4 skips the import, even though Zig still resolves it to the foreign file. The existing registry-name scanner could ignore escapes because those names are plain identifiers, but file paths need real string decoding before they are resolved or the new cross-pack import ACL is easy to bypass.
Useful? React with 👍 / 👎.
| const dir = std.fs.path.dirname(file) orelse "."; | ||
| const resolved = std.fs.path.resolve(arena, &.{ dir, path }) catch return; |
There was a problem hiding this comment.
For cached or otherwise non-sibling pack sources, this resolves the string against the original source file, but scanPack later copies every pack to target/packs/<pack_name> and Zig resolves these @imports from that copied layout. An undeclared import like @import("../../production/events/item_produced.zig") in a cached citizens pack will compile after generation as target/packs/production/..., yet here it resolves under the cached citizens source tree and never matches production's source dir, so rule 4 misses the cross-pack import for distributed packs.
Useful? React with 👍 / 👎.
| if (pathUnder(resolved, ctx.current_pack_dir)) return; | ||
|
|
||
| for (ctx.foreign_packs) |fp| { | ||
| if (!pathUnder(resolved, fp.dir)) continue; | ||
| if (fp.allowed) return; // declared dependency (or contracts) — permitted |
There was a problem hiding this comment.
When pack directories overlap, these early returns can classify an import into a nested foreign pack as safe because it is also under the current pack (or under an allowed ancestor pack) before the more specific foreign pack is checked. For example, if @packs/world and @packs/world/production are both declared packs, a world source can import production/... without depends_on and line 508 treats it as an own-pack path, bypassing rule 4.
Useful? React with 👍 / 👎.
| // walker produced, so the result shares a canonical base with the foreign | ||
| // pack dirs (both come from the same `resolvePlugin` realpath pass). | ||
| const dir = std.fs.path.dirname(file) orelse "."; | ||
| const resolved = std.fs.path.resolve(arena, &.{ dir, path }) catch return; |
There was a problem hiding this comment.
This compares the lexically resolved import path to canonical pack dirs without calling realpath on the imported file. On case-insensitive filesystems an import like @import("../../Production/events/item_produced.zig") still opens production/..., and symlinked directories have the same problem, but the string prefix test will not match the foreign pack dir (or may return as current-pack), so rule 4 misses a real cross-pack import.
Useful? React with 👍 / 👎.
1 participant
Closes part of #491. Turns one slice of the pack anti-corruption layer from taste/review into a compiler-adjacent
labelle checkgate — the slice that is actually enforceable in the assembler without Zig semantic/call-graph analysis.What #441 already enforces (confirmed, unchanged)
pack_validate.zig(#441, CLOSED) already failsgenerateon:depends_onnaming a pack/plugin not declared inproject.labelle(nor the implicitcontracts).depends_ongraph must stay acyclic (DFS with a cycle-path diagnostic).So candidate (b) from the #491 investigation is done — no work needed.
And
labelle check(check.zig) already lints three §6 ACL rules: cross-pack registry access (getType("other__X")/view(.{Foreign})), raw.global-facet writes, and event-direction inversions.What this PR adds — rule 4:
cross-pack-importNone of the existing rules caught the escape engine #656 flagged as the real ACL breach: recovering a foreign type by importing its file outright —
This sidesteps both module isolation and the string-keyed registry (rule 1). This PR flags it textually: each relative
.zig@importis resolved lexically against the importing file's directory and tested for containment under a foreign pack's canonical on-disk dir. An import into a pack not in the current pack'sdepends_on(and notcontracts) is a violation.An import of a declared dependency's file stays allowed — packs currently share a copied tree with no module wall (RFC §6 "isolation is future work"), so reaching a declared dep is the sanctioned path; only reaching a non-dependency pack is the reach-around.
What this PR deliberately does NOT do (and why)
@import: a re-export chain, a type reached through a value, or an import via a named module. Same call-graph limitation. Rule 4 closes the blatant literal case only, and says so in-code..exposes-symbol resolution (candidate (a))..exposes/PackExposesis parsed but has zero consumers and zero realpack.labelleexamples in the tree, and packs: scan a light pack's scripts/ dir (deferred from #440/#485) #487 scans pack script filenames only (nopub fnsymbol table). A resolution check would therefore encode a currently-undefined contract and risk false positives during partial surface migration (surface split between game-root and pack). Recommend packs: type/enforce the query+command surface from .exposes/.depends_on #491 first defines the exposes→symbol contract (or folds exposes-resolution into the engine-side compile enforcement, where the symbol table actually exists) before this is built.The enforceable boundary (for #491's record)
@importreach-around (this PR) + the existing registry/facet/event lints.Tests
+8 (6 pure
scanSourcecases incl. allowed-dep, own-file, module-import, disabled-context, and acitizens/citizens2prefix-boundary case; 2 e2erunLintfixtures exercising real-path resolution). Full cleanzig build test= 1149 pass,zig buildgreen.https://claude.ai/code/session_01P7B7UzgrWEbBYLT3YBrAog
Summary by CodeRabbit
New Features
.zigfile without an allowed dependency.Bug Fixes
Tests
.zig, and boundary-path cases.