Repository navigation
Publish only a whitelist of files in the gem - #48
Merged
Merged
Conversation
`spec.files` had no reject filter at all, so everything tracked was published. Two consequences: - `img/wigu.png` is 288 KB — 99% of the gem — for a README screenshot referenced only as a repo-relative path from README.md:74, never from the SCSS. GitHub renders it from the repo either way. - Any spec/ or test/ directory added later would have shipped automatically, with nothing in the gemspec to stop it. Drops `spec.test_files` while here: RubyGems deprecated it, and it only ever listed files that should not be packaged in the first place. Packaged: 277 KB -> 12 KB, with app/assets/stylesheets/wigu/ intact.
Fivell
force-pushed
the
chore/exclude-specs-from-gem
branch
from
September 30, 2026 10:23
049f89b to
dbf864f
Compare
Fivell
changed the title
The reject list only removes directories someone remembered to name. That is how spec/, .github/, screen/ and img/ got published in the first place — each needed a new pattern, and none was added until an audit went looking. A whitelist inverts the default: a new directory in the repo does not reach consumers until it is listed. Same shape the sibling gems activeadmin-oidc and credit_card_validations already use. Drops the remaining dev-only files the reject form kept: active_admin_theme.gemspec Gemfile package.json Rakefile Packaged: 9 -> 5 files. The runtime payload — everything under lib/, app/, vendor/, config/ and exe/ — is byte-identical to before, verified by diffing the built .gem both ways.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The documented package contents contradict the new whitelist and should be reverified.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Restricts gem packaging to runtime assets and essential documentation.
Changes:
- Whitelists
lib,app, README, and license files. - Removes deprecated
spec.test_files.
| File | Description |
|---|---|
active_admin_theme.gemspec |
Defines the reduced package whitelist. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`Dir[...]` globs the working tree, so any untracked or generated file under lib/, app/ or vendor/ would be published in a release — the build artifact depended on the releaser's local checkout. The reject form it replaced was tracked-only; this restores that property while keeping the whitelist. `git ls-files -- <paths>` also returns files only, where `Dir["**/*"]` returns directory entries too, so `files` no longer carries entries RubyGems just ignores. Built .gem is byte-for-byte the same file list as the Dir[] version.
Fivell
changed the title
`executables` greps `files` for `^bin/`, but `bin` was not in the whitelist, so that grep could never match. Adding a `bin/foo` later would have published a gem with no executables and no build error to say so — exactly the failure the whitelist exists to prevent, inverted. No repo file changes today: none of these gems tracks a bin/, and the built .gem is identical.
A whitelist fails quietly: `git ls-files -- config` against a tree with no config/ exits 0 and prints nothing, so the day someone adds `config/initializers/foo.rb` the gem installs, boots, and the initializer never runs. Nothing in `gem build` warns. That is not hypothetical for this family of gems — active_admin_datetimepicker's Ransack predicates live in exactly such an initializer, and its filters return no results without them. So list every root Rails::Engine loads from (lib app vendor config exe bin), present or not, instead of only the ones that happen to exist today. No package change: the built .gem is identical.
Bare backticks run `git ls-files` wherever `gem build` was invoked from. Build this gemspec by absolute path from any other directory and git returns nothing, `files` comes back empty, and `gem build` reports success — a valid, publishable, completely empty gem. Wrapping in `Dir.chdir(File.expand_path(__dir__))`, the way the sibling capybara_active_admin gemspec already does, turns that into a loud `Gem::InvalidSpecificationException: [...] are not files`.
Fivell
added a commit
to yeti-switch/active_admin_theme
that referenced
this pull request
Oct 2, 2026
master has moved three commits ahead: the gem file whitelist (activeadmin-plugins#48), the compile check and variable type guards (activeadmin-plugins#50), and the header menu work with its review fixes (activeadmin-plugins#51). activeadmin-plugins#51 matters here. Its first two commits are this branch's first two, cherry-picked, with thirteen defects fixed on top — and it was squash-merged, so git cannot tell they are already upstream. A rebase would have replayed them and quietly reverted every one of those fixes; merging keeps them. The stylesheet conflicted in eight places. Resolution: * Variable header: both sides kept. This branch's palette and the per-mode *Dark values stay; the eight menu variables the two sides share take master's definitions, because master's are the corrected ones — $skinMenuPillTextColor and $skinMenuItemHoverTextColor now default to $skinMenuTextColor instead of #ffffff independently, $skinMenuItemPaddingY is 8px, $skinHeaderPaddingY is split back into Top/Bottom so the header does not shift, and $skinMenuPanelMaxWidth comes across. The type guards from activeadmin-plugins#50 follow the variables. * Menu rules: master's throughout — the width ceiling, the squared pill and panel corners, the 5px bridge, the currentColor marker, :focus. * #utility_nav and ul.tabs > li font-size: this branch's, master has nothing there. * The dead #title_bar batch-actions block stays deleted. Verified after the merge: the stylesheet compiles, and every fix from activeadmin-plugins#51 is still present in it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
filesis now an explicit whitelist, instead of publishing everything tracked minus whatever someone remembered to exclude.A reject list only removes what someone named. That is how 1 README image ended up in the published gem — each needed its own pattern, and none was added until an audit went looking. A whitelist inverts the default: a new directory does not reach consumers until it is listed.
Every root
Rails::Engineloads from is listed, present in this repo or not. A whitelist fails quietly —git ls-files -- configagainst a tree with noconfig/exits 0 and prints nothing — so the day someone addsconfig/initializers/foo.rbthe gem would install, boot, and never run it. Naming the roots up front costs nothing (git ls-fileson a missing path is a no-op) and removes that trapdoor.Selected through
git ls-filesrather thanDir[...]so the artifact stays tracked-only — an untracked or generated file underlib/cannot leak into a release — and sofilescarries no directory entries.Verified
All 5 files that leave the package:
Nothing is added. The 3 files under
app/,lib/— the entire runtime payload — are unchanged: