Conversation
![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: 4e7455c63f
ℹ️ 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
Modernizes Upright’s compatibility, CI, dependency automation, and release engineering.
Changes:
- Restores Rails 8.0 compatibility and synchronizes Playwright versions.
- Adds hardened CI, CodeQL, Dependabot automation, and ownership rules.
- Introduces reproducible, verified release and recovery pipelines.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
upright.gemspec |
Supports strict gem builds. |
test/dummy/docker-compose.yml |
Updates Playwright image. |
test/dummy/db/schema.rb |
Pins Rails 8.0 schema DSL. |
test/dummy/db/queue_schema.rb |
Pins Rails 8.0 schema DSL. |
test/dummy/db/persistent_schema.rb |
Pins Rails 8.0 schema DSL. |
script/release/verify_package.rb |
Adds package-verification CLI. |
script/release/registry.rb |
Implements registry reconciliation. |
script/release/registry_download.rb |
Adds verified registry download. |
script/release/registry_confirm.rb |
Adds publication confirmation. |
script/release/registry_check.rb |
Adds pre-publish registry check. |
script/release/package_verification.rb |
Validates built gem integrity. |
RELEASING.md |
Documents release and recovery procedures. |
README.md |
Updates Playwright deployment example. |
Rakefile |
Adds bump and tag tasks. |
gemfiles/rails-main.gemfile |
Adds Rails-main probe. |
gemfiles/rails-8.0.gemfile |
Adds Rails 8.0 floor probe. |
bin/release |
Removes laptop-based release script. |
.gitignore |
Ignores matrix lockfiles. |
.github/workflows/release.yml |
Adds tag-driven release pipeline. |
.github/workflows/release-recovery.yml |
Adds gated release recovery. |
.github/workflows/dependabot-auto-merge.yml |
Adds constrained dependency auto-merge. |
.github/workflows/codeql.yml |
Adds CodeQL scanning. |
.github/workflows/ci.yml |
Expands and hardens CI. |
.github/release.yml |
Categorizes generated release notes. |
.github/dependabot.yml |
Groups and labels dependency updates. |
.github/CODEOWNERS |
Protects sensitive paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
script/release/package_verification.rb:63
- This new package gate is only described as manually exercised; no checked-in test covers it. Add tests using a fixture gem and the injected runner for dependency/content mismatches, install/probe failures, and CLI exit statuses, especially because a regression here can either publish contaminated bytes or block every release.
def verify!
script/release/registry.rb:40
- No checked-in test exercises this release-critical registry state machine, despite the injectable HTTP, sleeper, and clock. Add deterministic unit tests for push/skip/conflict decisions, retry and deadline bounds, malformed metadata, redirect restrictions, and downloaded-digest verification so a refactor cannot silently change publishing behavior.
def self.live(name)
The dummy app's dumped schemas said ActiveRecord::Schema[8.1], which Rails 8.0 can't load — contradicting the gemspec's 'rails >= 8.0'. The engine migrations host apps run are already Migration[8.0], and the suite passes under Rails 8.0.5, so flip the dummy schema pins to [8.0] rather than raising the floor. Also sync the docker-compose/README Playwright server image to the 1.59 pin in Upright::PLAYWRIGHT_VERSION.
Mirror surfguard's Dependabot config, keeping upright's existing cooldowns: grouped weekly PRs per ecosystem, deps/deps(actions) commit prefixes, and labels that feed .github/release.yml's notes categories. Grouping composes with auto-merge — fetch-metadata reports a grouped PR's highest update-type, so a group containing a major stays human-gated.
- Matrix: ruby 3.4 x rails 8.0 (floor), 4.0 x 8.1 (primary, committed lockfile), plus continue-on-error probes for ruby head and rails main via unlocked gemfiles/. All legs run the full suite. The head probe needs its own unlocked gemfile too: the committed multi-platform lock can't resolve against head's unreleased ABI. - Fan-in 'CI' job as the single branch-protection check; experimental legs can't block it. - Name every job, add workflow_dispatch and per-ref concurrency, port surfguard's checksum-verified actionlint + zizmor at pedantic over the whole tree, and add dependency review on PRs. - Refresh action pins to the current surfguard set.
- CODEOWNERS gates release- and security-sensitive paths behind review, deliberately without a catch-all or Gemfile.lock rule so Dependabot lockfile PRs don't deadlock on code-owner review. - release.yml categorizes generated release notes by label (using the repo's existing github_actions label) and excludes Dependabot noise. No CodeQL workflow: the repo already runs CodeQL default setup (extended suite over ruby, javascript-typescript, and actions), and advanced-config SARIF uploads are rejected while default setup is enabled.
Hardened pull_request_target port from surfguard: no checkout, no PR-controlled code execution; actor/author/head-ref gates plus full API re-validation (head SHA, same-repo head, Dependabot-signed commits, changed-file allowlist) before an approval pinned to the validated SHA and gh pr merge --auto --match-head-commit. A human push revokes any pending auto-merge. The allowlist admits only Gemfile and Gemfile.lock — deliberately not upright.gemspec, so gemspec-touching bumps like playwright-ruby-client (which require the multi-file Playwright version sync) stay human- reviewed. Bundler majors and all github-actions PRs stay human-gated too.
Port surfguard's release engineering, adapted for a Rails engine with ~30 runtime dependencies: - Tag-triggered release.yml: test -> unprivileged build (ancestry + tag/version guards, SOURCE_DATE_EPOCH-reproducible gem build --strict, package verification) -> reviewer-gated OIDC publish with a fail-closed registry reconciliation state machine -> credential-free confirm -> attestation of registry-confirmed bytes -> GitHub Release. Artifact identity flows forward by digest. workflow_dispatch is a no-publish rehearsal. - release-recovery.yml finishes a published-but-unreleased tag by rebuilding from the tagged source with the tag's own toolchain pins and proving rebuilt == canonical registry bytes before attesting. - script/release: verify_package checks archive integrity, spec identity, an exact dependency match against the gemspec (in place of surfguard's zero-dependency assertion), package contents against the gemspec's git-tracked file list (catches build-environment contamination), and an isolated install; registry.rb is surfguard's total-state-machine client, renamed. - Rakefile bump/tag tasks with guards; bundler/gem_tasks dropped so no laptop-push release task survives. bin/release retired in favor of RELEASING.md. - Drop the gemspec's homepage_uri metadata: it duplicated spec.homepage, which gem build --strict rejects.
The rails-main CI probe surfaced two behavioral changes in Rails main that weakened the CSRF work from #126 there: - Token.token_and_options now matches any Authorization scheme, not just Token/Bearer, so a Basic header started counting as 'bearer token provided' — skipping the cross-site session gate and passing verified_request?. Check the scheme in proxy_token_provided? instead of inheriting Rails' parsing breadth. - The 8.2 defaults verify CSRF via Sec-Fetch-Site alone and allow a header-less non-SSL POST, so the forged login-callback POST in LoginCsrfTest sailed through. Send cross-site provenance headers, as a real cross-origin browser POST would — refused under both the token strategy (8.1) and the header strategy (main).
Port surfguard's registry state-machine tests (every transition of check/confirm/verified_download plus the CLI entry points, with a scripted HTTP layer and fake clock) and adapt its package-verification tests to upright's checks: dependency match against the reference gemspec, packaged contents against the git-tracked file list (including the contamination case), isolated install, and the version probe. PackageVerification now takes its reference gemspec and tracked-file list as injected dependencies, wired to upright.gemspec and git ls-files in .live — same seam the runner already used.
Member
Author
|
This is a hardening + quality-of-life sync of release engineering machinery from surfguard and basecamp-sdk gems. Closing rather than doing wholesale adoption. Available for cherry-picks. |
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.
Brings upright's release engineering to parity with (and past) surfguard.
What's here (one commit each, independently revertable)
ActiveRecord::Schema[8.1], which Rails 8.0 can't load, contradicting the gemspec floor. The engine migrations host apps run were alreadyMigration[8.0], and the full suite passes on 8.0.5, so the schema pins flip to[8.0]rather than raising the floor. Also syncs the drifted Playwright server image (1.55 → 1.59) in docker-compose/README.deps/deps(actions)prefixes, labels feeding release notes.gemfiles/(head needs one too: the committed multi-platform lock can't resolve against head's unreleased ABI); all legs run the full suite. Fan-inCIjob as the single branch-protection check. Every job named, per-ref concurrency, checksum-verified actionlint + zizmor at pedantic over the whole tree (0 findings), dependency review on PRs.pull_request_targetport: no checkout, API re-validation (head SHA, same-repo head, signed Dependabot commits), changed-file allowlist ofGemfile/Gemfile.lockonly (deliberately excludingupright.gemspecso Playwright-sync bumps stay human-reviewed), approval pinned to the validated SHA,--match-head-commitmerge, revoke-on-human-push.SOURCE_DATE_EPOCH-reproduciblegem build --strict, package verification) → reviewer-gated OIDC publish with fail-closed registry reconciliation → credential-free confirm → attestation of registry-confirmed bytes → GitHub Release. Plusrelease-recovery.yml,rake bump/rake tag, RELEASING.md, andbin/releaseretired.verify_packageswaps surfguard's zero-dependency assertion for an exact dependency match against the gemspec and checks package contents against the gemspec's git-tracked file list (catches build-environment contamination — adversarially tested).Probe findings from the first CI run
The rails-main probe already earned its keep: two genuine Rails main (8.2.0.alpha) behavioral changes, non-blocking —
LoginCsrfTest: cross-origin POST without authenticity token now 302s instead of 422ProxyCsrfTest: non-bearer Authorization header now 401s (HTTP Token: Access denied) instead of 403Verified locally
bin/ci: rubocop, brakeman, actionlint, zizmor (pedantic,--collect all, 0 findings) all green; suite green except two pre-existing traceroute errors from the missing localmtrbinary (green in CI).SOURCE_DATE_EPOCHbuilds → identical sha256.verify_package.rbpasses clean and fails on a planted untracked file.registry_check/registry_downloadlive-tested against published upright 0.3.0 (skipdecision; digest-verified download).rake bump[0.4.0]rewrites version.rb + Gemfile.lock;rake tagrefuses dirty tree / non-main branch.Before the first pipeline release (one-time, see RELEASING.md)
Environments
release-rubygems/release-recoverywith required reviewer, RubyGems trusted publisher, tag rulesets,CIas the required check, repo auto-merge setting,breaking/cilabels.