ci: speed up RBS validation without Steep internals - #333
Conversation
dfa74c4 to
1fbbefe
Compare
1fbbefe to
ed82e0d
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors CI’s RBS validation to be a dedicated validate (rbs) job and introduces a Ruby wrapper script that accelerates Steep-based signature validation by consolidating eligible .rbs files into a single temporary signature file, while preserving exact per-file diagnostics via a fallback path.
Changes:
- Add
scripts/validate-rbsto run a fast consolidatedsteep check --no-type-checkwith an automatic per-file fallback for precise GitHub annotations. - Add Minitest coverage for success, fallback behavior, file-scoped directive bypass, and error/annotation reporting.
- Update Rake tasks and CI workflow to split
typecheck (rbi)fromvalidate (rbs)and wirevalidate-rbsintoci-required.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
test/scripts/validate_rbs_test.rb |
Adds automated tests covering consolidated validation, fallback, directive guards, and CI annotation output. |
scripts/validate-rbs |
New public-CLI orchestrator that consolidates signatures for fast Steep validation and falls back to per-file checking on failure or directives. |
Rakefile |
Replaces the prior Steep “typecheck” task with validate:rbs invoking the new script; updates aggregate typecheck task. |
.github/workflows/ci-checks.yml |
Splits RBI typecheck and RBS validation into separate jobs and makes RBS validation an explicit required dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ed82e0d to
c83fe2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/ci-checks.yml:101
- The previous workflow set
STEEP_JOBS: 8for the per-file Steep validation; the newvalidate-rbsjob no longer sets it. That means if consolidation is bypassed (file-scoped directives) or falls back for diagnostics, CI will run the slower reference path with Steep’s default job count instead of the tuned value, increasing red-build time.
permissions:
contents: read
runs-on: ${{ inputs.runner }}
steps:
test/scripts/validate_rbs_test.rb:156
- The tests aren’t hermetic:
run_validationpasses the providedenvtoOpen3.capture3, but any existing process environment variables (notablyCIandSTEEP_JOBS) will still be inherited by default. On GitHub Actions,CIis set, which would makescripts/validate-rbsadd--format=githuband change the expected fake-steep call arrays, causing these assertions to fail depending on the runner environment.
end
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c83fe2fe5f
ℹ️ 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 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/ci-checks.yml:101
- The workflow no longer sets
STEEP_JOBSfor thevalidate-rbsjob. Sincescripts/validate-rbsonly usesSTEEP_JOBSon the reference (per-file) fallback path, CI failures will now re-run the slower per-file check with Steep’s default worker count instead of the previously tuned value, increasing wall time on red builds.
validate-rbs:
timeout-minutes: 10
name: validate (rbs)
permissions:
contents: read
runs-on: ${{ inputs.runner }}
env:
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
I found one P1 correctness issue in the consolidation safety boundary: independently invalid RBS files can form a valid combined parser buffer, allowing the wrapper to report success without invoking its exact-diagnostics fallback. The inline comment includes an exact reproduction on locked RBS 3.9.5 / Steep 1.10.0 and a measured public-CLI mitigation. I also verified the file-scoped directive grammar, restored fallback worker count, required-check dependency, and actual CI speedup.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/validate-rbs:88
run_consolidated_checkusesOpen3.capture3but ignores the returned stdout/stderr. This still buffers Steep’s output into memory; on failures (where Steep can emit many diagnostics) that can be unnecessarily expensive before you immediately rerun the reference check. Consider spawning Steep with stdout/stderr redirected and waiting for the status only.
command = [env.fetch("STEEP_COMMAND", "steep"), "check", "--no-type-check", "--jobs=1"]
command << "--steepfile=#{temporary_steepfile}"
_stdout, _stderr, status = Open3.capture3(env, *command, chdir: root.to_s)
status
scripts/validate-rbs:70
run_parse_checkusesOpen3.capture3but discards stdout/stderr.capture3still buffers command output into Ruby strings, which is unnecessary work and can inflate memory/time ifrbs parseis chatty. PreferProcess.spawnwith stdout/stderr redirected andProcess.wait2to obtain the exit status only.
This issue also appears on line 85 of the same file.
def run_parse_check(root, env:)
_stdout, _stderr, status = Open3.capture3(env, "rbs", "parse", "sig", chdir: root.to_s)
status
end
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b339aae226a9100db4c25e60046cbeb12d8e2463, all three commits/four changed files, prior review #4848718240 and its resolution, and the locked RBS 3.9.5 / Steep 1.10.0 execution paths. The split class/end issue is fixed: independent parsing now fails before consolidation and ordinary Steep preserves original-file diagnostics. File-scoped directives, fallback workers/GitHub annotations, semantic errors, and the required-check dependency were also verified. However, the inline P1 demonstrates a distinct, independently reproduced consolidation-boundary false green: an embedded NUL terminates RBS lexing and can hide every later invalid signature. Submitting COMMENT because this still bypasses the required validation gate.
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ea5d75b0f7ce15b5cced4835da0e652569576476, all four commits/changed files, all prior conversations, and prior P1 review #4849204675. On locked RBS 3.9.5 / Steep 1.10.0, independently verified that NUL-containing signatures now route directly to ordinary Steep with the correct original-file diagnostic, the earlier split-file syntax regression stays fixed, and fallback workers/GitHub annotations, semantic validation, and ci-required wiring remain intact. However, the inline P1 independently reproduces another consolidation-boundary false green: valid whitespace-free/comment-adjacent use directives bypass the guard, leak file-scoped imports into later signatures, and let the required validation gate succeed when ordinary Steep fails. Submitting COMMENT because this still weakens required RBS validation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/scripts/validate_rbs_test.rb:5
fileutilsis required here but never used in this test file, which adds an unnecessary dependency and can confuse future readers.
require "fileutils"
require "minitest/autorun"
require "open3"
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 8b6a690c81b78f5a7e6248c061eb4e099276ef36, all five commits/four changed files, every prior review conversation, and the locked RBS 3.9.5 / Steep 1.10.0 parser and validator behavior. Prior P1 #4849278617 is fixed: exhaustive valid use keyword-boundary probes now take ordinary Steep, and the earlier NUL/split-file protections, original-file GitHub annotations, fallback workers, semantic validation, and ci / ci-required dependency remain intact. However, the inline P1 independently reproduces another required-validation false green: a valid multiline resolve-type-names: false magic directive evades the physical-line guard and suppresses a later original-file semantic error. Submitting COMMENT because the required RBS gate can still pass invalid signatures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/validate-rbs:93
run_consolidated_checkwrites the merged signatures under a temporarysig/, but runssteep checkwithchdir: root. Since the Steepfile uses relative paths (e.g.,signature("sig"),./manifest.yaml), this is likely to make Steep read the original repositorysig/instead of the consolidatedall.rbs, defeating the optimization.
concatenate(signatures, temporary_sig.join("all.rbs"))
command = [env.fetch("STEEP_COMMAND", "steep"), "check", "--no-type-check", "--jobs=1"]
command << "--steepfile=#{temporary_steepfile}"
_stdout, _stderr, status = Open3.capture3(env, *command, chdir: root.to_s)
scripts/validate-rbs:14
FILE_SCOPED_DIRECTIVEis anchored with^but isn’t using multiline mode, so ausedirective on a later line (e.g., after a header comment) won’t be detected. That can incorrectly allow signature consolidation in cases where file-scoped name resolution should force the reference per-file check.
# These directives change name resolution on a per-file basis, so signatures
# containing them cannot be safely combined and must take the reference path.
FILE_SCOPED_DIRECTIVE = /^\s*(?:use\b|#\s*resolve-type-names\s*:)/
test/scripts/validate_rbs_test.rb:103
- There’s no regression test covering a
usedirective that is not on the first line (e.g., after a comment header). That case currently exercises the fast path and can miss the intended reference-check bypass for file-scoped directives.
def test_checks_original_files_directly_when_a_signature_has_use_directives
_stdout, stderr, status, calls = run_validation(
{"uses.rbs" => "use Example::*\nmodule UsesExample\nend\n"},
fake_steep_results: [0]
)
assert_predicate(status, :success?, stderr)
assert_empty(stderr)
assert_equal([%w[check --no-type-check]], calls)
end
def test_checks_original_files_when_a_use_directive_has_no_whitespace
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 1a8c4498facc6c5ede04c662e44bfcf5fc41c5f6, all six commits and four changed files, every prior review conversation, and locked RBS 3.9.5 / Steep 1.10.0. The P1 from review #4849370345 is fixed: complete-binary-file matching covers the actual parser magic-comment grammar, including newline-spanning whitespace. An executable exhaustive audit checked 1,242,528 directive spellings (776,580 parser-accepted; zero missed) and 7,500 use inputs (1,014 parser-accepted; zero missed). Independently reproduced the earlier split-file, embedded-NUL, whitespace-free use, multiline-directive, and additional vertical-tab/form-feed cases: each now takes ordinary per-file Steep, rejects invalid signatures, and preserves the exact original-file GitHub annotation. Also verified parser boundaries, comments, line endings, encodings, BOM, filesystem enumeration, fallback workers/formatter, semantic validation, temporary Steepfile resolution, and explicit validate-rbs enforcement by ci / ci-required. No substantive findings; approving.
Summary
This changes the RBS half of CI from a generic “typecheck” job into an explicit
validate (rbs)job and replaces the slow per-file Steep invocation with a public-CLI validation pipeline.The validator now:
.rbsfiles;rbs parse sig, which parses every original file independently;steep checkCLI to perform semantic validation once over the merged declaration environment;steep checkagainst the original files whenever either fast-path stage fails, preserving exact source paths and GitHub annotations.The RBS job remains an explicit dependency of
ci-required, so it cannot be skipped while the aggregate required check passes.Root cause
The generated signatures are split across 1,209 files and heavily reopen the same namespaces. Steep validates the merged definition of a reopened namespace in the context of each input signature file. That makes the work grow with both the number of signature files and the size of the merged namespaces, even though there are no Ruby source files configured for Steep to typecheck.
This is the same split-signature performance shape discussed in steep#661. Consolidating signatures removes the repeated per-file namespace validation while keeping Steep itself as the semantic validator.
On the current repository:
steep check --no-type-check --jobs=8rbs parse sigpreflightThe reference and optimized commands both pass the full 1,209-file corpus. The optimized path remains roughly 8× faster locally. Before this change, the GitHub job spent roughly 3m31s in RBS validation, so this targets the actual long pole rather than Ruby setup or Sorbet.
Why this is safe
Public CLIs are the only tool boundaries
The implementation does not load Steep or RBS as Ruby libraries and does not reference
Steep::orRBS::implementation constants.It invokes two documented commands from the locked bundle:
rbs parse sigfor independent syntax parsing only;steep checkfor the existing semantic validation.The RBS parser preflight is not a replacement for Steep and does not make semantic acceptance decisions. Steep remains the authority for type names, inheritance, aliases, generic bounds, constants, members, and other semantic validity.
Original file boundaries remain authoritative for syntax
Concatenation cannot be allowed to decide whether the original files are syntactically valid. Without a preflight, two independently invalid files can repair each other. For example:
a.rbs:class Exampleb.rbs:endEach original file is invalid, but their concatenation is a valid class declaration. Split return types and orphan annotations can create the same class of false green.
The validator therefore runs
rbs parse sigbefore creating the combined buffer. The CLI parses every original file independently. A syntax failure skips consolidation and routes directly to ordinary per-file Steep, which reports the exact original file and line. A regression test uses the splitclass/endreproduction and requires the wrapper to fail with an original-fileRBS::SyntaxError.Independent parsing alone is not sufficient for every parser-boundary byte. Locked RBS 3.9.5 treats an embedded NUL (
\\x00) as EOF while still exiting successfully. If such a file were concatenated before another signature, the combined parser could silently ignore every later declaration. The eligibility scan therefore reads signatures as binary and routes the whole job through ordinary per-file Steep if any NUL is present. The regression test places a NUL in the first file and an unknown type in the second; validation must fail against the second original file.The semantic fast path changes partitioning, not declarations
After every source file has independently parsed, the script writes each file byte-for-byte, in deterministic glob order, into one temporary
sig/all.rbs, adding only newline separators. The repository’s existingSteepfileis copied into the temporary project, so the same target, libraries, diagnostic configuration, and dependency setup are used.RBS declarations live in a merged environment. For independently parseable declaration files, combining buffers changes how many parser buffers contain those declarations, but not the classes, modules, aliases, constants, methods, type parameters, bounds, or annotations that Steep validates.
File-scoped semantics are explicitly excluded
Locked RBS 3.9.5 has two constructs that intentionally alter name resolution on a per-file basis:
use ...# resolve-type-names: ...Concatenating files containing either construct could change which declarations the directive affects. The validator matches the actual
usekeyword boundary (use\\b), including whitespace-free forms such asuse::Example::Foo, wildcard imports, and comment adjacency. It applies the directive pattern to each complete binary file, rather than physical lines, because locked RBS 3.9.5's magic-comment grammar allows\\s*to span newlines (for example,#\\nresolve-type-names: false). It scans for both file-scoped forms, alongside the NUL/EOF boundary described above, and bypasses consolidation entirely if any hazard is present anywhere insig/. In that case it immediately runs ordinary per-file Steep.The current generated corpus contains neither construct. If generation introduces one later, correctness wins automatically and CI becomes slower rather than weaker.
Any fast-path rejection is checked against the original files
A nonzero result from either independent parsing or consolidated Steep is never reported directly as the final answer. The script reruns Steep over the untouched repository signatures and uses that reference invocation’s exit status.
That has three important properties:
The failure path intentionally retains the old cost. Slow red builds are preferable to fast but ambiguous diagnostics. The workflow preserves
STEEP_JOBS: 8specifically so this reference fallback produces diagnostics promptly.The validation level is not weakened
The command uses
--no-type-checkto state the job’s actual scope: RBS validation. The currentSteepfiledeclaressignature("sig")and no Rubycheckpaths, so the old job was not typechecking Ruby source code. It was already doing signature validation only.Steep’s normal signature validation remains enabled. Tests demonstrate that the wrapper rejects:
usedirectives whose imports would leak across file boundaries;resolve-type-namesmagic directives whose setting would leak into later files;Sorbet’s RBI check remains a separate
typecheck (rbi)job.Operational failures fail closed
Missing signatures, a missing
Steepfile, unavailablerbsorsteepexecutables, temporary-file errors, and unexpected exceptions all produce a nonzero exit. The optimization does not turn infrastructure failures into successful validation.The required-check contract is explicit
The reusable workflow has separate job IDs for
typecheckandvalidate-rbs. Therequiredjob:validate-rbsinneeds;needs.validate-rbs.result;success.Branch protection can therefore continue requiring the single aggregate
ci-requiredcontext without losing the RBS gate.Intentional tradeoffs
There is no dependency on private Steep/RBS APIs, no new language toolchain, and no reduction in semantic validation.
Verification
ruby test/scripts/validate_rbs_test.rb: 12 runs, 60 assertionsCI=1: 12 runs, 60 assertionsrbs parse sig: 1,209 original files parsed independentlygit diff --check: passesTests cover the cross-file syntax-repair regression, the NUL/EOF truncation regression, whitespace-free
usedirectives, the multiline magic-comment regression, consolidated success, reference fallback, both file-scoped directive families, reopened namespaces, ordinary syntax errors, semantic generic-bound errors, unknown names, fallback worker tuning, and GitHub annotation formatting.The full local integration harness requires Ruby 3.3 or newer; this machine has Ruby 3.2. PR CI runs all supported Ruby versions and is the authoritative full-suite validation.