[Mathih13]

Problem

Core's character-fence tripwire scans every installed Package's source for a raw game_character lookup and checks it against a per-file budget. That budget list hard-coded packages/playerbots/src/mod.rs, so a change inside playerbots could fail a Core test, and Core's own source named a Package.

A separate scan test used the same path as a worked example of the optional-directory rule, for the same reason.

Solution

WHITELIST now holds Core's own files only. A new PACKAGE_FILE_BUDGET list keys a budget on a file's path relative to its own Package root (src/mod.rs), not on which Package ships it. The same budget applies to any installed Package, and Core carries no Package name to grant it.

The example-path test now uses a made-up package name, sample_package.

Verified locally with the playerbots Package installed: the full tripwire and scan-test suite passes, and inserting one extra raw game_character lookup into packages/playerbots/src/mod.rs fails no_unwhitelisted_raw_character_lookups as expected (reverted before this PR). With no Package installed, the same tests pass.

---

Claude Sonnet, via Claude Code, made this change.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6651ad1f-29d3-4b55-85be-53dcb2e2add4

📥 Commits

Reviewing files that changed from the base of the PR and between 50e9709 and ae025f9.

📒 Files selected for processing (3)

• module/build.rs
• module/src/test_scan.rs
• module/src/tripwires.rs

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Mathih13]

Code review: two axes (Standards, Spec)

Fixed point: main (merge-base 50e9709). One commit, 4b4c20d. There is no linked issue, so the Spec axis uses the PR description and the commit message as the spec.

Standards

P1: the ratchet now gets looser, not tighter (behaviour change). The character_fence_tripwire doc says the budget "should only ever shrink". PACKAGE_FILE_BUDGET gives src/mod.rs in every installed Package a budget of 4. Before this PR only playerbots had that budget. Now packages/example/src/mod.rs (the public reference Package, 0 raw lookups today), packages/fire_nova/src/mod.rs, and any new Package can each add 4 raw game_character lookups and the tripwire stays green. The new doc also says no staleness ratchet covers the list, so nothing ever forces the 4 down. That conflicts with CODING_STANDARDS "Keep exemptions narrow" and with the tripwire's own stated promise. One alternative that keeps Core free of Package names and keeps the budget per Package: let each Package declare its own budget in its own directory (for example a small budget file or const under packages/<name>/). The scan reads it, and the stale-entry ratchet checks it against that Package's real count.

P2: tracker references in code comments. tripwires.rs:1217 says "the audited exception ticket 2 inherited" and :1227 says "Ticket 3 is already expected to revisit this number". CODING_STANDARDS, Comments and documentation: "Do not put issue numbers in code comments. Record durable reasoning in the comment or an ADR so it remains understandable without tracker history." State the reason directly, or remove it.

P3 (judgement): possible Duplicated Code. budget_for has the same .iter().find(..).map(..).unwrap_or(0) chain twice, once for each list. A small budget_in(list, key) would remove the copy. If you take the P1 change, this probably goes away.

P3 (judgement): no test for the new path parsing. CODING_STANDARDS, Tests, asks an Architecture Test to "prove it detects the forbidden mutation". The PR description says this was checked by hand. A unit test for package_relative_path (a Package path gives Some("src/mod.rs"), a Core path gives None, packages/README.md gives None) would keep that proof. test_scan.rs already has tests like this.

No other findings. Removing is_installed from character_whitelist_has_no_stale_entries is correct: all 17 WHITELIST entries are module/src/... files.

Spec

Partial: the sibling tripwire in the same file still names a Package. The spec says "Core's own source named a Package" and the fix is to stop that. But partition_discipline_tripwire's WHITELIST still has ("packages/deadmines/src/choreography.rs", 1, ...) at tripwires.rs:906, and its whitelist_has_no_stale_entries still has the is_installed skip for it. A change in the deadmines Package can still fail a Core test, which is the exact problem the PR describes. Fix it the same way as the character fence, or say in the PR why it stays out of scope.

Scope note (not blocking). The spec says "Core named a Package". Core comments and identifiers still name playerbots in many places (group.rs:275 cites packages/playerbots/src/goals.rs; bridge.rs exports playerbots_fixture_command_*). The title limits this PR to tripwires, so this is fine. But the description's wording claims more than the PR does.

Implemented as described: the playerbots entry is removed from WHITELIST, the error message now names both lists, and the two example-path tests now use sample_package. No scope creep beyond P1's widening, which is listed under Standards.

Summary: Standards has 4 findings; the worst is P1, which applies the budget of 4 to every Package with no staleness ratchet. Spec has 1 partial and 1 scope note; the worst is that packages/deadmines/... is still hard-coded in the sibling tripwire.

---

Generated by Claude Code