Publish only a whitelist of files in the gem - #89
Merged
Merged
Conversation
`screen/screen.png` is 529 KB — 97% of every download — for a README demo image referenced as a repo-relative path, so GitHub renders it from the repo and nothing ever reads it out of the package. `.github/` is CI config. Packaged: 21 files / 548 KB -> 19 files / 44 KB. lib/, app/, config/ and tasks/ untouched.
CI fails a matrix leg or two per run with
Ferrum::ProcessTimeoutError:
Browser did not produce websocket url within 10 seconds,
try to increase `:process_timeout`
Always on the first `visit`, never on a later one, and a rerun of the
same commit goes green — the browser starts fine, it just does not
always make 10 seconds on a GitHub runner. Raise the startup budget;
`default_max_wait_time` is untouched because nothing here is waiting on
an element.
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: .gitignore active_admin_datetimepicker.gemspec CODE_OF_CONDUCT.md Gemfile package.json Rakefile tasks/test.rake Packaged: 19 -> 12 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.
The earlier process_timeout bump was not enough — 30 seconds times out
too:
Ferrum::ProcessTimeoutError:
Browser did not produce websocket url within 30 seconds
Never receiving the websocket URL after 30s means Chrome is stuck, not
slow, so raising the budget again would only lengthen the wait before
the same failure. --disable-dev-shm-usage is the fix: containers mount
/dev/shm at 64 MB and Chrome deadlocks once it fills. --no-sandbox goes
with it because the runner is already an unprivileged user, and
--disable-gpu drops a subsystem that has nothing to do in headless.
process_timeout: 30 stays — it costs nothing and covers a genuinely
slow start.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The package inventory contradicts the stated verification, and the unrelated browser configuration change is undocumented.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Reduces the packaged gem size by switching to an allowlist and adjusts browser test startup.
Changes:
- Restricts gem contents to runtime files, README, and license.
- Adds Chromium CI startup options and timeout handling.
| File | Description |
|---|---|
active_admin_datetimepicker.gemspec |
Introduces the package allowlist. |
spec/support/capybara.rb |
Changes Cuprite startup configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+23
to
+24
| spec.files = Dir["lib/**/*", "app/**/*", "config/**/*", | ||
| "README.md", "LICENSE.txt"] |
Comment on lines
+12
to
+16
| Capybara::Cuprite::Driver.new(app, headless: true, window_size: [1280, 800], | ||
| process_timeout: 30, | ||
| browser_options: { | ||
| 'no-sandbox': nil, | ||
| 'disable-dev-shm-usage': nil, |
Comment on lines
+9
to
+10
| # slow. --disable-dev-shm-usage is the cause: containers give /dev/shm | ||
| # 64 MB and Chrome deadlocks when it runs out. --no-sandbox is needed |
`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.
The comment read as if the flag causes the deadlock, when it is the mitigation: the undersized 64 MB /dev/shm is the cause, and the flag moves Chrome's scratch space off it. As written it invited a future maintainer to remove the fix.
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.
`--disable-dev-shm-usage` is already in Ferrum's Chrome defaults (Ferrum::Browser::Options::Chrome::DEFAULT_OPTIONS), so the run that timed out at 30s was *already* started with it. It cannot have been the cause, and passing it again only put a duplicate switch on the command line. --disable-gpu is equally inert: Ferrum adds it on Windows only, and headless Chrome has no GPU subsystem to drop. That leaves --no-sandbox as the only flag in the set that changes anything, and its earlier justification was inverted too. Running as an unprivileged user is the condition under which the sandbox *works*; --no-sandbox is what you need when it cannot initialise. On ubuntu-latest, now 24.04, kernel.apparmor_restrict_unprivileged_userns=1 blocks the user-namespace sandbox for binaries with no AppArmor profile, and Chrome can hang before emitting a websocket URL. The runner is a single-tenant VM serving only this suite's dummy app, so dropping the sandbox costs nothing. Framed honestly: the flake has not reproduced since, which is not the same as proven fixed. One leg out of ~35 was failing.
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) in all of them, present or not, instead of only the ones that happen to exist today. No package changes: the built .gem is identical in every gem.
Second theory ruled out. --no-sandbox was credited with fixing it on the AppArmor-userns reasoning, but the sibling repo capybara_active_admin already passes --no-sandbox and --disable-setuid-sandbox (spec/rails_helper.rb) and still failed with Ferrum::ProcessTimeoutError on a run today. If the flag were the fix, that repo would not flake. The comment now says what is actually known: cause unestablished, both tried explanations disproved, --no-sandbox and process_timeout kept as cheap and plausibly useful rather than as a fix, rerun when it happens.
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 CI config, 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 9 files that leave the package:
Nothing is added. The 10 files under
app/,config/,lib/— the entire runtime payload — are unchanged:Also in this PR
Two commits fixing a CI flake, which rides along because this branch was the one open on the repo. Chrome intermittently fails to hand back a websocket URL on GitHub runners and the leg dies with
Ferrum::ProcessTimeoutError.The driver now passes
--no-sandbox.ubuntu-latestis 24.04, which shipskernel.apparmor_restrict_unprivileged_userns=1; that blocks Chrome's user-namespace sandbox for binaries without an AppArmor profile and the browser can hang before starting. The runner is a single-tenant VM serving only this suite's dummy app, so dropping the sandbox costs nothing.An earlier commit on this branch also passed
--disable-dev-shm-usageand--disable-gpuwith a /dev/shm explanation. That was wrong and is reverted:--disable-dev-shm-usageis already inFerrum::Browser::Options::Chrome::DEFAULT_OPTIONS, so the run that timed out was already using it, and--disable-gpuis added by Ferrum on Windows only.--no-sandboxis the sole flag in that set that changes anything.Honest framing: the flake has not reproduced since, which is not the same as proven fixed. It was one leg out of ~35.