feat: rolling_deploy_adapter for seeding previous bundle hashes

[justin808]

Summary

Adds a pluggable config.rolling_deploy_adapter protocol that eliminates 410→retry cold-start round-trips for previously-deployed bundle hashes during rolling deploys — complementing the current-hash seeding already handled by PRs #3124 + #3167.

Stacked on #3167 (PR B). Target base is `jg/3122-unify-renderer-cache-staging`; will rebase onto `master` once PRs A + B merge. Refs #3122. Design doc previously shared: see earlier session context.

Why

PR A seeded the current hash. PR B unified copy/symlink staging. But during a rolling deploy:

• Old Rails instances (hash `abc`) still drain traffic.
• New Rails instances (hash `def`) serve new traffic.
• New renderer instances receive requests for both hashes.

Pre-seeding only the current hash leaves `abc` cold on new renderers → every draining-version request hits the 410 retry path. This PR closes that gap.

Protocol

module MyRollingDeployAdapter
  def self.previous_bundle_hashes   # Array<String>
  def self.fetch(bundle_hash)        # Hash(:bundle, :assets) | nil
  def self.upload(bundle_hash, bundle:, assets:)
end

ReactOnRailsPro.configure do |config|
  config.rolling_deploy_adapter = MyRollingDeployAdapter
end

Parallel in shape to `remote_bundle_cache_adapter` — same duck-typed module pattern, same validation at configure time.

The loadable-stats wrinkle

Each bundle hash must carry its own companion `loadable-stats.json` and RSC manifests (built in lockstep with that bundle). Otherwise the renderer emits HTML referencing chunk URLs for the wrong build → client-side hydration breakage. The `fetch` signature returns bundle + assets together to force callers to think about this; see `docs/pro/rolling-deploy-adapters.md` for the full discussion.

Integration points

1. `PreSeedRendererCache.call` (from PR B) now invokes `RollingDeployCacheStager` after staging the current hash(es). Deduplicates against current hashes.
2. `AssetsPrecompile.call` auto-invokes `adapter.upload(current_hash, ...)` after precompile in production-like environments, closing the publish side so the next deploy can fetch.
3. `PREVIOUS_BUNDLE_HASHES` env var (comma-separated) overrides `previous_bundle_hashes` discovery for CI / testing.

Error handling — degrades gracefully

Runtime 410-retry remains the fallback for any seeding failure, so rolling-deploy seeding can never cause a correctness regression:

Scenario	Behavior
Adapter not configured	No-op.
`previous_bundle_hashes` raises	Warn, skip previous-hash seeding.
`fetch` returns nil or raises	Warn, skip that hash.
`upload` raises in precompile	Warn, don't fail precompile.
Returned hash matches current	Deduped.

Doctor (`react_on_rails:doctor`) probes

• ✅ Protocol conformance (all three required methods).
• ✅ `previous_bundle_hashes` probe with 3s timeout — reports latency and count.
• ⚠️ Empty list ("upload side has never run on a prior deploy").
• ℹ️ Resolved renderer cache dir + bundle-hash subdirs present.
• ℹ️ `PREVIOUS_BUNDLE_HASHES` env set without an adapter.

Doctor never calls `fetch` or `upload` — those have side effects.

Docs

• New: `docs/pro/rolling-deploy-adapters.md` — protocol spec, three reference implementations (S3, Control Plane, Filesystem), loadable-stats wrinkle discussion, relationship to `remote_bundle_cache_adapter`.
• `docs/pro/node-renderer.md` rolling-deploys section updated to point at the new page.
• Sidebar entry added.

Test plan

• 7 new specs for `RollingDeployCacheStager` covering every invocation path: adapter unset, hash list discovery, env override, copy mode, symlink mode, fetch nil, fetch raises, previous_bundle_hashes raises, missing :server_bundle in payload, dedup vs current.
• 5 new doctor specs for `check_rolling_deploy_adapter`.
• Existing Pro specs updated where `instance_double(Configuration, ...)` needed the new accessor.
• 36 Pro specs pass locally (27 from PR B + 9 new).
• 168 doctor specs pass locally (1 pre-existing unrelated failure in `auto_fix_versions`).
• `bundle exec rubocop` clean on all changed files.
• CI (will verify after push).

Relationship to other adapters

	`remote_bundle_cache_adapter`	`rolling_deploy_adapter`
Scope	Webpack build outputs (pre-compile caching)	Deployed bundle hashes (rolling deploy)
When	Build phase	Post-precompile + pre-seed phase
Avoids	Rebuilding webpack when source hasn't changed	410 retries for draining-version requests
Keyed by	Source digest	Bundle hash

Complementary — configure both if useful.

🤖 Generated with Claude Code

---

Note

Medium Risk
Adds new Pro deploy-time caching/publishing behavior (assets:precompile uploads; pre_seed_renderer_cache fetches/stages) and a sizeable new stager with filesystem moves/timeouts, which could affect deploy reliability and cache correctness if misconfigured.

Overview
Adds a new Pro config.rolling_deploy_adapter protocol (previous_bundle_hashes, fetch, upload) to pre-seed previously deployed bundle hashes (and companion loadable-stats.json/RSC manifests) into the Node Renderer cache to avoid 410→retry during rolling deploys.

Integrates this into the build/deploy flow: PreSeedRendererCache now invokes RollingDeployCacheStager (with PREVIOUS_BUNDLE_HASHES override, hash sanitization, timeouts, and safe staging via temp dirs/rollback), and AssetsPrecompile best-effort publishes current bundle hashes + assets via adapter.upload in non-dev/test environments.

Extends react_on_rails:doctor to validate adapter protocol/latency and report cache-dir status, adds adapter configuration validation (including upload signature checks), refactors cache staging helpers (central stage_file, symlink logic) and ensures loadable-stats.json is included when present; updates docs/sidebar/changelog and adds comprehensive specs.

^{Reviewed by Cursor Bugbot for commit 7537fc2. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e566f9dd-e6dc-4a38-85ff-3ca5c541cbc6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/3122-rolling-deploy-adapter

---

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 and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9cd1657f83

ℹ️ 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".

[greptile-apps]

Greptile Summary

This PR introduces RollingDeployCacheStager, a pluggable rolling_deploy_adapter protocol, and auto-upload in AssetsPrecompile to eliminate 410→retry cold-start round-trips for previous bundle hashes during rolling deploys. The overall design — duck-typed adapter module, graceful degradation, doctor probes, and deduplication against current hashes — is solid.

• P1: In RollingDeployCacheStager.call the early-return guard (return if adapter.nil? && ENV[\"PREVIOUS_BUNDLE_HASHES\"].to_s.empty?) lets the code proceed when PREVIOUS_BUNDLE_HASHES is set but adapter is nil. fetch_payload(nil, hash) then raises NoMethodError: undefined method 'fetch' for nil, rescued with a confusing message. The companion doctor check compounds this by reporting "env override will be used" — misleading operators into thinking the env var is standalone. The fix is to change the guard to return if adapter.nil?.
• P2: stage_file in :symlink mode creates absolute symlinks (File.symlink(File.expand_path(src), dest)), while PreSeedRendererCache#make_relative_symlink creates relative ones — the inconsistency could break if the cache dir is ever bind-mounted at a different path.

Confidence Score: 4/5

Safe to merge after addressing the nil-adapter + PREVIOUS_BUNDLE_HASHES early-return bug; remaining findings are style/test-coverage.

One P1 defect: the early-return guard allows a nil-adapter path that silently fails with NoMethodError and produces a misleading doctor message. This affects any operator who sets PREVIOUS_BUNDLE_HASHES without an adapter, and it is untested. The P2 absolute-symlink inconsistency is low-risk for typical same-filesystem deploys. All other pieces — configuration validation, graceful degradation, doctor probes, deduplication logic — are well-implemented and tested.

react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb (early-return guard + symlink mode), react_on_rails/lib/react_on_rails/doctor.rb (misleading 'env override will be used' message)

Important Files Changed

Filename	Overview
react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_cache_stager.rb	New module for seeding previous bundle hashes; has two issues: early-return guard allows nil-adapter + env-set path that silently fails with NoMethodError, and symlink mode creates absolute rather than relative symlinks unlike PreSeedRendererCache.
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb	Adds rolling_deploy_adapter accessor and DEFAULT/validate_rolling_deploy_adapter; validation correctly requires Module with all three methods.
react_on_rails_pro/lib/react_on_rails_pro/assets_precompile.rb	Adds publish_current_bundle_if_configured to upload the just-built bundle after precompile; error handling is correct but missing node_renderer? guard before calling NodeRenderingPool.server_bundle_hash.
react_on_rails/lib/react_on_rails/doctor.rb	Adds check_rolling_deploy_adapter probe with protocol conformance check, previous_bundle_hashes latency probe, and cache-dir resolution; doctor message for nil-adapter + env-set case is misleading ("env override will be used") when fetch will actually fail.
react_on_rails_pro/spec/dummy/spec/rolling_deploy_cache_stager_spec.rb	7 specs covering adapter-unset, fetch nil/raises, previous_bundle_hashes raises, missing server_bundle, dedup, copy and symlink modes; missing coverage for PREVIOUS_BUNDLE_HASHES with nil adapter.
react_on_rails_pro/spec/dummy/spec/pre_seed_renderer_cache_spec.rb	Correctly updated to include rolling_deploy_adapter: nil in instance_double stubs; all existing paths covered.
react_on_rails/spec/lib/react_on_rails/doctor_spec.rb	5 new doctor specs for rolling_deploy_adapter check covering nil adapter, env override, protocol success/missing, and empty hash list; comprehensive for the happy paths.
react_on_rails_pro/lib/react_on_rails_pro.rb	Adds require for rolling_deploy_cache_stager; load order is correct (before pre_seed_renderer_cache which depends on it).
react_on_rails_pro/spec/dummy/spec/prepare_node_renderer_bundles_spec.rb	Updated to include rolling_deploy_adapter: nil in instance_double; no functional changes to the spec logic.

Sequence Diagram

sequenceDiagram
    participant AP as AssetsPrecompile.call
    participant PS as PreSeedRendererCache.call
    participant RD as RollingDeployCacheStager.call
    participant DA as rolling_deploy_adapter

    AP->>PS: call(mode: :symlink)
    PS->>PS: stage current bundle hashes
    PS->>RD: call(cache_dir:, current_hashes:, mode:)
    alt adapter nil AND env empty
        RD-->>PS: return (no-op)
    else adapter present OR PREVIOUS_BUNDLE_HASHES set
        RD->>DA: previous_bundle_hashes() [or ENV override]
        DA-->>RD: ["abc", "def", ...]
        RD->>RD: deduplicate against current_hashes
        loop each previous hash
            RD->>DA: fetch(hash)
            DA-->>RD: {server_bundle:, rsc_bundle:, assets:} or nil
            RD->>RD: stage_file(src, dest, mode)
        end
    end
    AP->>AP: publish_current_bundle_if_configured
    AP->>DA: upload(current_hash, server_bundle:, rsc_bundle:, assets:)

Loading

_{Reviews (1): Last reviewed commit: "feat: add rolling_deploy_adapter for see..." | Re-trigger Greptile}

[claude]

Code Review

This is a well-structured PR with good overall design — the adapter protocol is clear, error handling degrades gracefully at every layer, test coverage is solid, and the docs are thorough. A few issues worth addressing before merge:

Bugs

1. nil adapter + PREVIOUS_BUNDLE_HASHES env var causes a confusing NoMethodError
When PREVIOUS_BUNDLE_HASHES is set but rolling_deploy_adapter is nil, the guard at RollingDeployCacheStager#call passes (nil adapter, non-empty env). resolve_previous_hashes correctly returns the env hashes via the explicit.any? branch, but then fetch_payload(adapter, hash) calls nil.fetch(hash) → NoMethodError. The rescue in seed_previous_hash catches it, so it degrades silently, but the warning message will read "NoMethodError" rather than anything actionable. Worse, the doctor check says "env override will be used" — implying standalone operation is supported.

Fix: add an explicit guard after hash resolution when adapter is nil, or document that PREVIOUS_BUNDLE_HASHES always requires a configured adapter for the fetch step. See inline comment at rolling_deploy_cache_stager.rb:65.

Security

2. Shell injection in ControlPlane reference implementation
previous_bundle_hashes uses backtick interpolation with WORKLOAD and GVC, and fetch passes an image string (derived from GVC + hash) to system(string). If any of these contain shell metacharacters, this is command injection. Use the array form of system and Open3.capture2e with argument arrays instead. See inline comment at rolling-deploy-adapters.md:240.

Correctness / Reliability

3. ENV.fetch at class-load time in reference implementations
BUCKET = ENV.fetch("ROLLING_DEPLOY_BUCKET") (S3) and GVC/WORKLOAD (ControlPlane) are evaluated when the file is required. Any environment where these vars aren't set raises KeyError on load. Use lazy def self.bucket = ENV.fetch(...) accessors instead. See inline comment at rolling-deploy-adapters.md:163.

4. No timeout on adapter.fetch during seeding
adapter.fetch(hash) has no timeout guard — only the doctor probe has a 3s timeout. A hung external store will block pre_seed_renderer_cache (and assets:precompile) indefinitely. Consider wrapping with Timeout.timeout. See inline comment at rolling_deploy_cache_stager.rb:98.

5. Race condition in S3 update_manifest!
update_manifest! is a read-modify-write with no concurrency guard. Concurrent deploys silently lose entries (last writer wins). At minimum document this; optionally use a conditional PUT with if_match: etag. See inline comment at rolling-deploy-adapters.md:218.

Style / Idioms

6. methods.include? vs respond_to?
Both validate_rolling_deploy_adapter and report_adapter_protocol use adapter.methods.include?(m). respond_to? is idiomatic for duck-typing checks and handles method_missing-based delegation. Note the existing validate_remote_bundle_cache_adapter uses the same pattern — if there's a reason, it's worth a comment. See inline comment at configuration.rb:266.

7. module_function + private_class_method vs class << self
RollingDeployCacheStager uses module_function then private_class_method. This also installs private instance method copies on any includer — almost certainly unintended. class << self with private is cleaner and matches PreSeedRendererCache. See inline comment at rolling_deploy_cache_stager.rb:63.

Minor

• publish_current_bundle_if_configured uses puts for success and warn for errors — inconsistent; consider warn or Rails.logger throughout so success messages aren't swallowed in background processes.
• The check_rolling_deploy_adapter spec doesn't cover the 3s timeout path; a test with a stubbed slow previous_bundle_hashes would close that gap.

🤖 Generated with Claude Code

[justin808]

Addressed review feedback in 2e9b3f8

MUST-FIX (2):

1. Nil adapter + `PREVIOUS_BUNDLE_HASHES` crash (greptile P1, claude) — when the env override was set without a configured adapter, the guard let execution proceed and `adapter.fetch(hash)` crashed on `nil`. Now warns and returns early. Doctor promotes the corresponding message from `:info` to `:warning` and explains that both env and adapter are required.

2. RSC previous-bundle staging path (codex P2) — the renderer routes RSC requests by `rsc_bundle_hash`, which is a separate `//.js` entry from the server bundle. The original protocol merged both bundles under one hash's directory using basenames, so the RSC bundle never landed where the renderer looks. Protocol refactor:

   • `fetch(hash)` → `{ bundle: path, assets: [paths] }` (opaque per-hash).
   • `upload(hash, bundle:, assets:)`.
   • `previous_bundle_hashes` returns a flat list including both server and RSC hashes.

   `AssetsPrecompile.publish_current_bundle_if_configured` now calls `upload` twice when RSC is enabled (once per hash). Stager stages each hash at `//.js`.

SHOULD-FIX (4):

3. `Timeout.timeout` on `adapter.fetch` (claude) — 30s default. Prevents a hung S3 / network call from blocking pre-seeding or `assets:precompile`.
4. Relative symlinks (greptile P2) — `stage_file` in `:symlink` mode now uses the same `Pathname#realpath` + `relative_path_from` pattern as `PreSeedRendererCache.make_relative_symlink`.
5. Remove stray `module_function` (claude) — was leaking private instance methods to any `include`r. All methods now `def self.`; private helpers marked `private_class_method`.
6. `respond_to?` instead of `methods.include?` (claude) — applied to both `Configuration.validate_rolling_deploy_adapter` and `Doctor.report_adapter_protocol`.

DOC POLISH (3):

7. Control Plane example — migrated from backtick/`system` shell interpolation to `Open3.capture2e` with array-form args. `previous_bundle_hashes` now returns both `REACT_ON_RAILS_BUNDLE_HASH` and `REACT_ON_RAILS_RSC_BUNDLE_HASH` so RSC deploys seed correctly.
8. Lazy env-var accessors — S3 and Control Plane examples now read env vars inside methods rather than at class-load time, so `KeyError` can't fire on `require` in environments that don't configure the adapter.
9. S3 race condition note — explicit callout that `update_manifest!` is read-modify-write; documents serialized deploys or `If-Match`/ETag as strict-safety alternatives.

Tests: 13 rolling-deploy specs (was 7) — adds nil-adapter+env warning, fetch timeout, relative-symlink assertion. Doctor spec updated for the warn-vs-info change. 38 Pro specs pass, 167/168 doctor specs pass (1 pre-existing unrelated failure in `auto_fix_versions`, unchanged).

I (Claude Code) addressed the feedback on behalf of @justin808.

[claude]

Code Review — PR #3173: rolling_deploy_adapter protocol

Overall this is a well-designed addition. The duck-typed protocol mirrors remote_bundle_cache_adapter, the error-handling table is thorough, the doctor probes are appropriately scoped (no fetch/upload side-effects), and the 7 stager specs cover all the edge cases cleanly. A few issues worth addressing before merge:

---

Bugs / correctness

1. publish_current_bundle_if_configured is missing a node_renderer? guard (inline comment on assets_precompile.rb:83)

NodeRenderingPool.server_bundle_hash is referenced unconditionally. If a user configures rolling_deploy_adapter while still using ExecJS this will raise at precompile time. The sibling call to PreSeedRendererCache.call already has a node_renderer? guard — publish_current_bundle_if_configured should too.

2. Path traversal via PREVIOUS_BUNDLE_HASHES (inline comment on rolling_deploy_cache_stager.rb:62)

Env-var values flow directly into File.join(cache_dir, hash) without any sanitisation. A value like ../../../tmp/evil escapes the cache directory. Adapter-returned hashes come from your own code and are presumably safe; the env-var path is not. A simple allowlist regex (/\A[A-Za-z0-9_\-]+\z/) covers all realistic hash formats.

---

Missing test coverage

publish_current_bundle_if_configured (new in assets_precompile.rb) has zero specs. This is the hot path during every production assets:precompile — it calls the adapter, calls NodeRenderingPool class methods, and has branching for RSC. At minimum, add specs for: adapter nil → no-op; development env → no-op; upload succeeds; upload raises → warns but doesn't propagate; RSC enabled → uploads twice.

---

Reference implementation issues (inline comments on rolling-deploy-adapters.md)

3. ControlPlane adapter: Dir[File.join(tmp, "*.json")] is too broad (line 236)
An image pull may deposit lock files or other JSON alongside the manifests. The S3 adapter explicitly names loadable-stats.json, react-client-manifest.json, react-server-client-manifest.json — the Control Plane adapter should do the same.

4. S3 adapter: @s3 ||= is not thread-safe (line 162)
Under concurrent callers two threads can each instantiate an Aws::S3::Client. Wrapping with a Mutex (or simply not memoizing, given the call frequency) fixes this.

5. update_manifest! stores RETENTION + 2 but previous_bundle_hashes returns RETENTION (line 187)
This is likely intentional (soft buffer) but undocumented, making the code confusing. A one-line comment explaining the intent would help.

---

Minor observations (not blocking)

• FilesystemRollingDeployAdapter#fetch returns dir.join("bundle.js").to_s without checking File.exist?. The stager's fetch_payload handles this, but it silently violates the contract the protocol doc sets out (returning a non-nil payload that fails the existence check). Consider checking existence before returning or returning nil.
• The Dir[dir.join("*.json")] in FilesystemRollingDeployAdapter#fetch passes a Pathname to Dir[]. Ruby calls to_s on it so it works, but dir.join("*.json").to_s is more explicit about intent.
• validate_rolling_deploy_adapter in configuration.rb is not directly unit-tested. The doctor specs exercise a similar check but not the configuration-time validation path.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e9b3f8e58

ℹ️ 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".

[justin808]

Addressed post-revision review feedback in fb3de1b

Seven new actionable items since `2e9b3f8e5` — all fixed. None skipped.

Security / correctness:

1. Path traversal via `PREVIOUS_BUNDLE_HASHES` (claude) — hash values are used as directory names. Now reject anything not matching `/\A[A-Za-z0-9_\-.]+\z/` with a warning. Spec asserts `../../../etc` is rejected while safe hashes continue through.
2. `node_renderer?` guard in `publish_current_bundle_if_configured` (claude) — skip publication under ExecJS renderer. Without this, `NodeRenderingPool.server_bundle_hash` would crash precompile.
3. Top-level rescue in publish method (cursor Medium) — `collect_assets` / `server_bundle_hash` / `rsc_bundle_js_file_path` can raise from the setup code. Wrapped the whole publication path with `rescue StandardError` so failures degrade next-deploy seeding but don't fail this deploy's `assets:precompile`.
4. Partial-staging cleanup on asset failure (codex P1) — previously a failed asset copy after a successful bundle copy would leave the cache with a bundle without its companion manifests. Renderer would serve it, producing hydration failures instead of the clean 410-retry fallback. Now `rm_rf` the hash directory on any failure inside `seed_previous_hash`. Spec asserts the bundle dir is gone after a mid-stage failure.

Docs:

3. Control Plane fetch allowlist (claude) — replaced `Dir[tmp/*.json]` with explicit `loadable-stats.json` + RSC manifest allowlist. Stray image-layer JSON won't be picked up.
4. S3 client mutex (claude) — `@s3 ||=` is not thread-safe under concurrent callers. Example now uses `Mutex.synchronize` memoization with a doc comment.
5. S3 RETENTION alignment (claude) — both read and write now trim to `RETENTION`; dropped the `+2` asymmetry. Added comment explaining the intent for users who want a soft buffer.

Tests: 40 specs pass (was 38; +2 for path-traversal reject + partial-staging cleanup). Rubocop clean. Pre-commit hooks pass.

I (Claude Code) addressed the feedback on behalf of @justin808.

[claude]

Code Review

Overall: Well-designed, carefully implemented feature with excellent test coverage. The graceful degradation strategy (every failure path degrades to 410-retry, never to a correctness regression) is exactly right for this kind of infrastructure component. The path-sanitization, atomic staging with temp-dir/backup/rollback, and timeout wrappers are all solid. A few things worth discussing:

---

Potential bug — auto_fix_versions info message always shown

doctor.rb removed the if result.changes.any? guard around the `FIX=true only updates package.json` message (line 753 in the current file). The message now prints even when no version changes were made, which is noise and slightly misleading. This looks like an unrelated cleanup that slipped in — worth reverting unless it was intentional.

---

Missing private_class_method on new AssetsPrecompile helpers

publish_bundles, filter_existing_assets, warn_skipped_invalid_assets, warn_if_unavailable_required_rsc_assets, required_rsc_asset_basenames, publish_bundle, and upload_bundle are all public class methods on AssetsPrecompile::Singleton. RollingDeployCacheStager consistently marks its implementation helpers private_class_method. The AssetsPrecompile pre-PR code doesn't use private_class_method either, but since these are new helpers with no external callers (tests reach them via described_class.filter_existing_assets), making them private would lock down the surface area.

---

Sequential seeding could be slow at deploy time

hashes.each { |hash| seed_previous_hash(adapter, hash, cache_dir, mode) }

Each seed_previous_hash call wraps a fetch inside a 30s timeout. With 2+ previous hashes (a common case when RSC is enabled — server + RSC bundle per deploy), worst-case pre-seed latency is 10s (discovery) + 30s × N (fetches). For N=4 (two deploys worth of server+RSC hashes), that's 2+ minutes added to pre_seed_renderer_cache. This is documented-but-silent behavior. A note in the docs/task output about expected latency scaling, or even a per-hash progress log line before the fetch starts, would help operators triage slow pre-seeds.

---

valid_asset_payload? rejects a hash on any invalid asset path — including non-required ones

If the adapter returns a non-required asset (e.g. an optional chunk file) whose path no longer exists on disk, the whole hash is skipped — not just the missing asset. The current behavior is defensively correct (better to 410-retry than serve a bundle with missing assets), but the warning messages (warn_missing_asset_payload / warn_non_file_asset_payload) only mention "non-required" when the missing file is not RSC-required. An operator seeing "Skipping this hash" after a non-required-asset warning might not expect the full skip. A one-liner in the doc's error-handling table or the warning text itself would make the strict-skip policy explicit.

---

PR description timeout mismatch

The PR description says "✅ `previous_bundle_hashes` probe with 3s timeout" but the code has DISCOVERY_TIMEOUT_SECONDS = 10 and the docs page correctly says "10 seconds". Just a stale summary — the code and docs are consistent with each other.

---

Minor: webpack development.js cleanup is unrelated

Removing sockPort: devServer.port from ReactRefreshWebpackPlugin config is correct (the option was removed in react-refresh-webpack-plugin v0.6), but it's orthogonal to the rolling-deploy adapter work. Fine to land together; just noting it for commit-message hygiene.

[claude]

This unconditionally adds the "FIX=true only updates package.json" info line regardless of whether any versions were actually changed. The original code only showed it when result.changes.any?. As-is it prints even on a no-op run, which is noisy and slightly misleading. Intentional change or accidental removal of the guard?

Suggested change

	checker.add_info(" ℹ️ FIX=true only updates package.json; update Gemfile constraints manually if needed.")
	checker.add_info(" ℹ️ FIX=true only updates package.json; update Gemfile constraints manually if needed.") if result.changes.any?

[claude]

Hashes are seeded sequentially, each with up to FETCH_TIMEOUT_SECONDS (30s) per fetch. With RSC enabled, a typical setup produces two hashes per deploy (server + RSC bundle). Seeding the previous N deploys means up to 10s (discovery) + 30s × 2N added to pre_seed_renderer_cache in the worst case.

Worth logging a per-hash "fetching..." line before the adapter.fetch call so operators can see which hash is slow rather than watching a silent hang:

Suggested change

	hashes.each { |hash| seed_previous_hash(adapter, hash, cache_dir, mode) }
	hashes.each do |hash|
	puts "[ReactOnRailsPro] Fetching previous bundle hash #{hash} via rolling_deploy_adapter..."
	seed_previous_hash(adapter, hash, cache_dir, mode)
	end

[claude]

The strict-skip policy (reject the whole hash when any asset path is invalid, even a non-required one) is defensively correct, but the warning text for non-required assets doesn't make it obvious that the entire hash is being dropped. An operator seeing "returned non-required asset path(s) that do not exist … Skipping this hash" may be surprised that a single optional chunk file causes a full skip.

Consider adding a brief clause like "Skipping this hash to avoid staging an incomplete bundle directory." to make the causation explicit.

[claude]

publish_bundles and all the helpers below it (filter_existing_assets, warn_skipped_invalid_assets, warn_if_unavailable_required_rsc_assets, required_rsc_asset_basenames, publish_bundle, upload_bundle) are public class methods on a Singleton, with no intended external callers. RollingDeployCacheStager marks its analogous helpers private_class_method throughout. Worth applying the same pattern here to lock down the public surface of AssetsPrecompile.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7537fc2ff3

ℹ️ 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".

[chatgpt-codex-connector]

Match temp-dir regex to actual PID width

Update TEMPORARY_DIRECTORY_PATTERN so it matches temp dirs generated by this module for all process IDs. temporary_bundle_directory and backup names embed Process.pid without padding, but the regex requires \d{4,}; in common container/runtime setups where PID is 1..999, entries like abc123.staging-1-deadbeefcafe are never recognized as temporary, so stale staging/backup directories are never swept and can accumulate indefinitely.

Useful? React with 👍 / 👎.