Fix durable ShakaPerf release evidence reuse - #4833
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe release workflow adds durable tracker-bound ShakaPerf evidence reuse, fail-closed verification, stable-only observation waivers, target-specific tracker validation, npm readiness checks, and classified publication retries. Documentation, fixtures, and release-helper tests cover the new behavior. ChangesRelease controls
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseTask
participant GitHub
participant ShakaPerf
participant NpmRegistry
ReleaseTask->>GitHub: validate tracker, commit, and workflow evidence
ReleaseTask->>ShakaPerf: select or reuse verified run
ShakaPerf-->>ReleaseTask: return validated result or observation error
ReleaseTask->>NpmRegistry: publish with classified retry handling
NpmRegistry-->>ReleaseTask: return publication result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR expands the release workflow’s ShakaPerf evidence persistence, recovery, and waiver handling while adding npm readiness and publication hardening.
Confidence Score: 4/5The publication-boundary observation failure must be fixed before merging because a waived stable release can continue without revalidating the exact ShakaPerf run attempt. The new waiver flow deliberately tolerates observation outages, but its final boundary check also suppresses an unavailable live lookup and ignores the failed validation, allowing immutable publication after the run state has become unknown. Files Needing Attention: rakelib/release.rake Important Files Changed
Sequence DiagramsequenceDiagram
participant Release as Release task
participant Tracker as Release tracker
participant GitHub as GitHub Actions API
participant Boundary as Publication boundary
participant Registry as Package registry
Release->>Tracker: Load saved run and waiver
Release->>GitHub: Observe exact run attempt
GitHub--xRelease: Observation unavailable
Release->>Tracker: Persist/reuse observation waiver
Release->>Boundary: Revalidate before tag/package publication
Boundary->>GitHub: Fetch exact run attempt
GitHub--xBoundary: Observation still unavailable
Boundary-->>Release: Current code treats error as nil
Release->>Registry: Publication may continue
Reviews (1): Last reviewed commit: "Reject noncanonical ShakaPerf associatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the React on Rails unified rake release tooling to avoid redundant ShakaPerf work by reusing maintainer-verified, tracker-bound schema-v2 evidence when available, while hardening selection/recovery rules and adding a narrowly scoped stable-release-only observation waiver path.
Changes:
- Adds durable ShakaPerf evidence discovery/reuse via canonical release tracker comments, plus strict run selection and fail-closed handling for unknown/edited/spoofed states.
- Adds stable-final-only, tracker/SHA/attempt-bound ShakaPerf observation waiver support and publication-boundary revalidation.
- Adds pre-auth/pre-mutation npm readiness verification (pinned pnpm + frozen lock + build for publishable packages) and hardens npm publish retry classification and output redaction; updates docs and changelog accordingly.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| react_on_rails/spec/react_on_rails/fixtures/release/shakaperf_issue_4806_run_30417447319.json | Adds a fixture representing tracker-bound ShakaPerf evidence for replay/verification tests. |
| rakelib/release.rake | Implements ShakaPerf durable evidence selection/reuse/waiver logic, npm readiness preflight, and classified npm publish retries with redaction. |
| internal/contributor-info/releasing.md | Documents the new readiness preflight, durable ShakaPerf association reuse, and stable-only observation waiver workflow. |
| CHANGELOG.md | Adds a user-visible changelog entry describing the release-tooling hardening and evidence reuse behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rakelib/release.rake (1)
9496-9496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
release_tracker_inputfor the later tracker reads.Lines 9622 and 9778 read
ENV.fetch("RELEASE_TRACKER", nil)again. The task already captured that value here. Use the local variable at both call sites so the release task has one source for the tracker input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rakelib/release.rake` at line 9496, Reuse the existing release_tracker_input variable at the later tracker-read call sites instead of calling ENV.fetch("RELEASE_TRACKER", nil) again, ensuring both locations use the single captured tracker value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 54: Update the changelog entry for Issue 4812 to include the repository’s
standard canonical pull-request link and author attribution format, retaining
the issue link only as supplementary context if appropriate.
In `@rakelib/release.rake`:
- Around line 1870-1878: Update apply_final_shakaperf_observation_waiver! to
explicitly reject a nil error.run before calling
exact_final_shakaperf_waiver_run!, aborting with a clear fail-closed message
that explains a waiver cannot apply without an observed run. Preserve the
existing validation and waiver flow when error.run is present.
- Around line 8779-8805: The dependency readiness check in
validate_npm_release_readiness! must not compare pnpm-lock.yaml and
node_modules/.pnpm/lock.yaml byte-for-byte. Replace that comparison with a
pnpm-supported resolved-state validation or existing release-tooling check,
while still rejecting missing or genuinely stale installed dependencies and
preserving the current abort message behavior.
In `@react_on_rails/spec/react_on_rails/release_rake_helpers_spec.rb`:
- Around line 4748-4750: Clean up the globally installed
trusted_shakaperf_release_tracker_records! method after the example defining it
via Object.send(:define_method, ...). Add example-scoped ensure/after removal
with Object.send(:remove_method, :trusted_shakaperf_release_tracker_records!),
or replace the global patch with an RSpec allow(...).to receive(...) stub.
---
Nitpick comments:
In `@rakelib/release.rake`:
- Line 9496: Reuse the existing release_tracker_input variable at the later
tracker-read call sites instead of calling ENV.fetch("RELEASE_TRACKER", nil)
again, ensuring both locations use the single captured tracker value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c59712b-8fa2-49a1-8639-3475e6afce66
📒 Files selected for processing (5)
CHANGELOG.mdinternal/contributor-info/releasing.mdrakelib/release.rakereact_on_rails/spec/react_on_rails/fixtures/release/shakaperf_issue_4806_run_30417447319.jsonreact_on_rails/spec/react_on_rails/release_rake_helpers_spec.rb
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review summaryReviewed the diff ( What holds up well
Issues found (see inline comments for details)
Structural concern (no single bug, but worth raising)This PR adds ~2,455 lines to an already 10,000+ line single Minor
No security-critical exploit or data-corruption path was found; the issues above are about defense-in-depth, operator experience on retry, and changelog polish rather than a broken release. |
Review summaryWhat this PR does: Extends Security: All new Correctness: The core trust chain held up under scrutiny — I specifically checked whether an Test coverage: The spec file is extensive (22k+ lines) and covers most of the new paths well. One gap I didn't find covered: Code quality / maintainability: Overall: no blocking issues found. The three inline comments below are suggestions/hardening opportunities rather than confirmed exploitable bugs — the safety-critical verification chain (evidence digest + runtime-tree fingerprint binding to the real release SHA) appears sound. |
![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: b07b1700e1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cfbb85c11
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/contributor-info/releasing.md:233
- The docs say live ShakaPerf association is “blocked until the repository-owned release wrapper … exists,” but the code path for
RELEASE_SHAKAPERF_RUNpersists a verified association to the tracker during a live (non-dry-run) release viapersist_verified_shakaperf_release_tracker_evidence!/post_release_tracker_comment!(seerakelib/release.rakearoundselect_and_verify_shakaperf_release_gate_run!). This sentence appears to be outdated and could mislead maintainers about what a live release will actually do.
This preview validates the target and tracker inputs but does not fetch or persist the selected run. Live
association remains blocked until the repository-owned release wrapper described in the execution boundary exists.
QA Evidence v2 — final refreshed head
Independent, read-only adversarial QA verified the complete issue #4812 release-integrity surface. Same-run conflicts now fail closed at all three layers: digest/runtime-fingerprint identity, candidate SHA before preference, and repository/branch/target version before requested-identity filtering. The direct replay rejected all 12 field/order combinations. Checks:
Adversarial coverage included typed association provenance, fresh discovery remaining unassociated, association loss/mutation/spoof rejection, stale and naturally invalidated evidence, missing run/artifact and API failure behavior, immutable waiver authorization/run-attempt/scope/identity bounds, terminal-result and unrelated-gate bypass attempts, and TOCTOU revalidation at tag-push and package-publication boundaries. The issue #4806 fixture replay for run All 19 review threads were inspected and are resolved. Current-head Copilot review produced no new comment; current-head CodeRabbit and Claude checks completed successfully. Required CI readiness is No tag, package publication, OTP entry, live waiver, release dispatch, hosted-CI request, or release override occurred during QA. UI evidence is not applicable because this is CLI-only release automation. Process-gap disposition: |
QA Evidence v2 — final refreshed head
Independent, read-only adversarial QA verified the complete issue #4812 release-integrity surface. Same-run conflicts now fail closed at all three layers: digest/runtime-fingerprint identity, candidate SHA before preference, and repository/branch/target version before requested-identity filtering. The direct replay rejected all 12 field/order combinations. Checks:
Adversarial coverage included typed association provenance, fresh discovery remaining unassociated, association loss/mutation/spoof rejection, stale and naturally invalidated evidence, missing run/artifact and API failure behavior, immutable waiver authorization/run-attempt/scope/identity bounds, terminal-result and unrelated-gate bypass attempts, and TOCTOU revalidation at tag-push and package-publication boundaries. The issue #4806 fixture replay for run All 19 review threads were inspected and are resolved. Current-head Copilot review produced no new comment; current-head CodeRabbit and Claude checks completed successfully. Required CI readiness is No tag, package publication, OTP entry, live waiver, release dispatch, hosted-CI request, or release override occurred during QA. UI evidence is not applicable because this is CLI-only release automation. Process-gap disposition: |
…kaperf-evidence * origin/main: Docs: add missing content — release notes, upgrade guide, config, API references (#4843) (#4844) Fix incorrect docs: helper names, defaults, requirements, runtime refs (#4836) Scroll-priority streaming: candidate architecture evaluation (#4835) (#4841) Fix generated server config lint cleanup (#4840) # Conflicts: # CHANGELOG.md
QA Evidence v2 — recovered refreshed exact head
Fresh independent read-only QA at the refreshed head verified the full issue #4812 release-integrity surface. All non-CHANGELOG PR files are byte-identical to the prior functional head, while the merge resolution preserves the exact PR #4840 main entry and PR #4833 entry in newest-first order. Checks:
Adversarial coverage included spoofed and mismatched repository/workflow/event/branch identities; edited, duplicate-family, stale, naturally invalidated, missing-run, missing-artifact, API/permission, digest, runtime-fingerprint, candidate-SHA, repository, branch, target-version, global-conflict, order, authorization, run-attempt, scope, terminal-result, TOCTOU, irreversible-boundary, and unrelated-gate/multi-gate bypass cases. The guarded issue #4806 fixture replay for run The canonical security preflight was No tag, package publication, OTP entry, live waiver, release dispatch, hosted-CI request, release override, PR code edit, commit, push, or merge occurred during QA. UI, interaction, and visual-fix evidence are not applicable because this is CLI-only release automation. Process-gap disposition: |
|
head_sha: a0d51d8
|
…out-vm-pool * origin/main: Docs: move agent coordination to the HTTP backend (#4764) Detect unnoticed changes across generated webpack/Rspack configs (#4839) Fix durable ShakaPerf release evidence reuse (#4833) Docs: add missing content — release notes, upgrade guide, config, API references (#4843) (#4844) Fix incorrect docs: helper names, defaults, requirements, runtime refs (#4836) Scroll-priority streaming: candidate architecture evaluation (#4835) (#4841) Fix generated server config lint cleanup (#4840) Document serialized release backport policy (#4592) Package version-matched agent skills and docs (#4809) Fix ci-required base ref for PR merge-ref checkouts (#4819) Honor response charset and reject non-2xx HTTP-served SSR bundles (#4817) [Pro] Redact RSC render-error metadata on the fetched (client-navigation) payload path (#4821) Forward-port the 17.0.1 changelog section to main (#4814) # Conflicts: # CHANGELOG.md
…ential-broker * origin/main: (75 commits) Silence routine startup diagnostics for Rails commands (#4849) Docs: move agent coordination to the HTTP backend (#4764) Detect unnoticed changes across generated webpack/Rspack configs (#4839) Fix durable ShakaPerf release evidence reuse (#4833) Docs: add missing content — release notes, upgrade guide, config, API references (#4843) (#4844) Fix incorrect docs: helper names, defaults, requirements, runtime refs (#4836) Scroll-priority streaming: candidate architecture evaluation (#4835) (#4841) Fix generated server config lint cleanup (#4840) Document serialized release backport policy (#4592) Package version-matched agent skills and docs (#4809) Fix ci-required base ref for PR merge-ref checkouts (#4819) Honor response charset and reject non-2xx HTTP-served SSR bundles (#4817) [Pro] Redact RSC render-error metadata on the fetched (client-navigation) payload path (#4821) Forward-port the 17.0.1 changelog section to main (#4814) Handle selector metacharacters in renderComponent DOM IDs (#4808) [Pro] Prevent caching RSC renders with errors (#4804) Agents: trust Copilot review identities (#4807) Agents: bind fleet closeout to generated pack (#4805) Docs: ADR 0002 — Skills-in-package over MCP for agent-native DX (#4735) Scope GitHub release commands to the origin repository (#4803) ...
Why
Stable releases could repeat ShakaPerf dispatch and publication work even when the canonical release tracker already contained verified schema-v2 evidence for the exact release candidate. Recovery also needed to distinguish natural invalidation from unknown or integrity-sensitive failures, while keeping every unrelated release gate intact.
Fixes #4812.
What changed
Safety boundaries
Validation
a0d51d81a394d9780b4dc4bcd66273933ae0ac3fThe exact #4806 / run 30417447319 fixture replayed verified evidence reuse without dispatch or publication. No live release mutation was invoked.
Review and decision log
Multiple skeptical review waves exercised tracker spoofing/editing, stale and missing evidence, API ambiguity, selector precedence, live workflow identity, exact target/version binding, retry classification, clean-clone ordering, waiver authorization/scope/attempt binding, multi-gate bypass attempts, and post-pull freshness. Confirmed findings were fixed and independently replayed; the current head has zero unresolved review threads.
Process-gap disposition: script. The release helper now enforces and replays clean-clone readiness before any remote/auth/publish boundary. Non-goal: bypassing ShakaPerf assertions or any unrelated release gate.
Hosted validation class
This changes release and publication logic, so force-full hosted CI was run and passed for the current head.
Merge qualification
Release-mode gate:
mainis beta phase; the active tracker is strict RC mode, so standard merge qualification applies. Current-head CI, configured reviewers, independent QA, security preflight, stage dependency, unresolved-thread, and strict-ledger gates are satisfied.Confidence note:
a0d51d81a394d9780b4dc4bcd66273933ae0ac3f.Labels: ready-for-hosted-ci, force-full-hosted-ci — full hosted validation completed for the release/publication surface.
Benchmarks: not applicable.
Summary by CodeRabbit
Release Process
Documentation
Tests