Fix CI: check out core submodule, drop retired macos-13 runner#18
Merged
Conversation
Integration Test – Harper UDS proxy failed at "Build harper" because harper-pro embeds core as a git submodule but the checkout step didn't request submodules, leaving core/ empty (thousands of TS2307 errors). harper-pro main also carries pre-existing type errors, so tsc exits non-zero even with core present (it still emits). Mirror harper-pro's own integration-tests.yaml: submodules: recursive, tolerate the build error, gate on the built artifact. Test – macos-13 sat queued for the 24h ceiling then auto-cancelled (runner_name empty, no steps ran) — GitHub has retired the Intel/macos-13 hosted runners. Drop it from the matrix; macos-14 (arm64) keeps macOS test coverage and the x64-darwin artifact is still built by build-macos. Add timeout-minutes to the test/integration jobs as a guard against genuine execution hangs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
The status.json test hardcoded the expected version as '0.4.0', which the 0.5.0 release bump broke (server writes pkg.version from package.json). Read the expected version from the same source of truth so a version bump no longer breaks the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a cargo-test job so the Rust unit suite is actually run in CI (npm test only covers the Node specs). This surfaced a real bug: is_grease only matched 0x0A0A, not the full GREASE set (0x0A0A, 0x1A1A, ... 0xFAFA). GREASE cipher/extension values are randomized per-connection, so failing to filter them destabilizes JA3 fingerprints. Fix: match both bytes equal with low nibble 0xA. All 5 cargo tests now pass (test_is_grease was red). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cb1kenobi
reviewed
Jul 8, 2026
| pull_request: | ||
|
|
||
| jobs: | ||
| cargo-test: |
Member
There was a problem hiding this comment.
Low: PR description contradicts this change
The PR body's "Note" states npm test runs only the Node specs and "never cargo test", with sni::tests::test_is_grease "Tracking that separately." But this same PR adds this cargo-test job (which runs cargo test) and includes the is_grease fix in src/sni.rs that makes test_is_grease pass. As written, the description misleads a reader about what CI now covers and where the GREASE fix lives.
Suggested fix: update the description to note that Rust tests are now gated in CI here and the GREASE fix is included in this PR, rather than tracked separately.
—
Generated by Barber AI
cb1kenobi
approved these changes
Jul 8, 2026
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
2 participants
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.
Problem
Every CI run on
mainhas been red. Two distinct failures:1. Integration Test – Harper UDS proxy (failed at "Build harper")
harper-pro embeds
coreas a git submodule (git@github.com:HarperFast/harper.git), but the Checkout harper-pro step didn't request submodules, socore/was empty → thousands ofTS2307: Cannot find module '../core/...'errors. Compounding it, harper-pro'smaincarries pre-existing type errors, sotscexits non-zero even with core present (it still emits output).Fix mirrors harper-pro's own
integration-tests.yaml:submodules: recursiveon the checkoutnpm run build || true, then gate ontest -f dist/bin/harper.js2. Test – macos-13 (showed "24h0m0s")
The job never got a runner — verified via the API:
runner_name: "",steps: [],conclusion: cancelled. It sat queued for the 24h ceiling then auto-cancelled. GitHub is retiring the Intel/macos-13 hosted runners, sotimeout-minutescan't help (it bounds execution, not queue wait).Fix: drop macos-13 from the test matrix. macos-14 (arm64) keeps macOS test coverage, and the x64-darwin artifact is still built by
build-macos.Also added
timeout-minutes(15 test / 30 integration) as a guard against genuine execution hangs.Note
sni::tests::test_is_grease(red locally) is not a CI failure —npm testruns only the Node specs, nevercargo test. Tracking that separately.🤖 Generated with Claude Code