Repository navigation
ci(integration): split the suite into parallel parts, select by tag - #756
Conversation
Ten tests in tests/integration bring up servers or containers of their own (a ClickHouse, a NATS server, a Redis, processes on random ports) and spend most of their time waiting on leases, handovers and restarts. Run serially they made up most of the package's 337s on a 4-core machine. Marked t.Parallel, they overlap each other and the multi-process roles test, which already ran in parallel. Each was checked for shared state first: per-test tables and databases on the shared ClickHouse, no t.Setenv, no captured default logger, no fixed ports. TestShardClaims_ScaleOneThreeTwo and the tests that share the wired app stay sequential. Refs #701 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pre-push review gate could attest to code no reviewer read. review-marker.sh keyed the marker to whatever HEAD the session's launch directory had when the reviewer stopped, so a commit made while a reviewer ran inherited its ship_it, and a review of a sibling worktree marked the launch directory's branch instead. Reviewers now end with `REVIEWED: <sha>` naming the commit they pinned before reading anything. The hook writes the marker into each worktree on that commit, and only when that worktree's HEAD reflog shows it on the commit from the start of the review (the subagent transcript's first timestamp) until now. Same-commit entries such as `git reset` or `git stash` don't count as moves. A report delivered through the subagent hand-back leaves only closing text in last_assistant_message, so the hook reads the hand-back's message from the transcript. Every decision goes to stderr and to tmp/review-marker.log, which the push gate prints when it blocks. agent-bash-gate.sh checked HEAD of the session's launch directory for every push. It now follows the push to where it runs (the tool call's cwd, a preceding cd/pushd, git -C), resolves each refspec source to the commit it publishes, and looks for that commit's markers in every worktree of the repository. It blocks when it can't tell: a computed directory or refspec, GIT_DIR, --all/--mirror, a glob refspec, a wrapped push, or a payload without cwd. A push into another repository with no reviewer manifest is outside the policy. Pushes through `git -C` and from a detached HEAD were not gated at all before. skip-pre-push-review.sh writes to the worktree root when run from a subdirectory. .claude/hooks/review-gate.test.sh drives both hooks with synthetic events in a scratch repository with sibling worktrees; it is a make verify leaf (test-review-gate). Because the pre-commit hook runs it with GIT_DIR exported, it clears git's repository variables, names every repository with -C, and refuses to write unless git sees the scratch directory as outside any repository and creates each scratch repository where asked. /prepush, the reviewer agents, AGENTS.md and the Claude Code docs page describe the REVIEWED line and the rule not to move HEAD while reviewers run. Closes #454 Closes #565 Closes #659 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Integration job had grown to about 8 minutes and was CI's critical path. make test-integration now runs two parts, test-integration-app (tests/integration, with -parallel 8 since its parallel tests mostly wait) and test-integration-backends (internal/cache, natsspike and internal/mq's tagged tests), at once. CI runs one matrix job per part: the changes job reads INTEGRATION_PARTS from the Makefile (scripts/ci/integration-parts.sh) and hands the coverage job the fragments and producer jobs to wait for, so a part added to the Makefile gets its own job, and CI and a local run execute the same commands. Each part keeps its own build-cache suffix, so no part saves a cache holding only its own test binaries. internal/mq's integration tests were chosen by a -run name pattern, so a new prefix would have been skipped silently. scripts/ci/tagged-tests.sh run selects the tests that only build with the integration tag, and tagged-tests.sh check, run before every part, fails while a package with tagged tests belongs to no part. tagged-tests.test.sh checks the selection against go test -list on a throwaway module (a verify leaf). natsspike stays in the suite: it pins the nats-server behavior the external NATS topology rests on, and it runs beside internal/cache. Closes #701 Closes #692 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cheap filter in agent-bash-gate.sh strips quotes one line at a time, so a heredoc body, a multi-line quoted string or a comment that mentions `git push` reached the tokenizer, which correctly found no push, and the gate then refused the command as one it couldn't follow. It now refuses only when the tokens hold a `git … push` the walk didn't gate: behind an unknown wrapper, or inside a command substitution. The gate runs before the command line does, so in `git commit … && git push` it judged the commit before the new one. A push that follows a HEAD-moving git command (commit, merge, rebase, reset, checkout, switch, cherry-pick, revert, am, pull) on the same line is now blocked with "push in a separate command". Pushes inside if/while/until/for … do are followed instead of refused, and the matching refspec `:` (or `+:`) is blocked like --all instead of being read as a delete. review-marker.sh uses the hand-back message whenever the final message lacks either the VERDICT or the REVIEWED line, retries the transcript read once, and resolves an abbreviated REVIEWED sha that names exactly one commit. A ship_it without a REVIEWED line, which is what a session started before this change produces because reviewer definitions load at session start, logs how to recover, and the gate's block message says the same and now shows the latest decisions from every worktree's log. pre-push-reviewer pins its commit in step 1 with `git -C <path> rev-parse HEAD`. The docs say the gate checks the commit each refspec publishes (its tip, or HEAD with no refspec), and the remaining copies that described a VERDICT-only, HEAD-keyed marker now describe REVIEWED plus VERDICT and the reviewed commit. The test covers each change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-ups to the integration split. - scripts/ci/integration-parts.sh read only the first INTEGRATION_PARTS line, so a later `+=` or a second assignment changed what make runs but not CI's matrix or the coverage job's wait list. It now refuses anything but a single `INTEGRATION_PARTS := ...` line, and its --check mode, run by check-integration-parts before every part, fails when its list differs from make's own. integration-parts.test.sh covers the shapes it must refuse (a verify leaf). - scripts/ci/tagged-tests.sh missed tagged examples and tests whose signature wraps across lines. It now takes every top-level func named Test*, Fuzz* or Example*, and its test compares the selection with every entry `go test -list` prints, with an example and a wrapped signature among the fixtures. - Docs: AGENTS.md's CI summary, the ci.yml comments that still called e2e the long pole or counted three fragments, development.md's account of the parts and of which tests run in parallel, where a package with both unit and tagged tests goes, a fresh cache suffix per job in the README's add-a-job recipe, estimates flagged in the timing table, and a transition note for purging the retired -integration cache entries. Refs #701 Refs #692 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # Makefile # docs/src/content/docs/development.md
agent-bash-gate.sh looked for an unfollowed push only when it had followed none, so a second push on the same line behind a wrapper or in a command substitution went unchecked once the first was gated. The walk now records each simple command it gates, and every other one is checked. GIT_DIR or GIT_WORK_TREE exported, declared or assigned earlier on the line now makes the push's repository unknown, the same as the inline form. The test requires jq up front, and development.md lists jq among the prerequisites, since make verify now runs it. pr-review-locally names the PR worktree's path in its reviewer prompts, because reviewers pin the worktree their prompt names, else their current directory. The reviewer agents say an unresolvable REVIEWED sha means no marker, and the remaining "marker for HEAD" phrasings say "for the commit being pushed". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The missing-suffix error, the input description and the header comment still allowed reusing another flavor's suffix when the flags match. Two jobs on one key race to save it, which is why the integration parts each take their own. Also restores the blank line the merge dropped before the test-review-gate rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Four gaps in the push gate, each now covered by review-gate.test.sh: - Every check fed the command to `grep -q` through a pipe under pipefail. Past a pipe buffer (64 KB), grep's early exit sent SIGPIPE to the writer and the pipeline read as no match, so a long `gh pr create` without --draft, or a push ahead of a long heredoc body, went unchecked. The checks now read here-strings. - An exported GIT_DIR or GIT_WORK_TREE shared a variable with "directory unknown", so a later absolute cd, pushd or git -C cleared it. It is tracked on its own now; only unset or the end of a subshell clears it. - A HEAD-moving git command counted only when run directly, so `timeout 60 git commit … && git push` or `out=$(git commit …) && git push` had the push judged on the commit before the move. A wrapped or substituted one counts too. - A push in code handed to a shell (bash -c, sh -c, eval, a heredoc or here-string read by a shell) was never seen, and one in a process substitution was dropped as a redirect target. The first now blocks as a push the gate can't follow; the second is followed like any other push. The Claude Code docs now say what a gating reviewer must end its report with for its ship_it to write a marker, matching AGENTS.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three more gaps in the push gate, each covered by review-gate.test.sh: - A substitution in a here-string or a redirect target was dropped with its word, so `grep -q x <<< "$(git -C ../wt-b push)"` or `echo hi > "$(git push …)"` went unchecked, and a commit in one didn't count as a HEAD move. The tokenizer now keeps the text of every $(…) and `…` wherever its word goes, and the push and HEAD-move checks scan all of it. A quoted substitution no longer slips past the cheap pre-filter either. - Inside $(…), a here-string's text was glued to `<<<`, so a push in `$(sh <<< 'git push')` didn't read as one. - Code handed to a shell was scanned without joining backslash-newline continuations, so a push split across lines in `bash -c` or a heredoc read by bash went through. AGENTS.md, the Claude Code docs and the changelog now say the gate reads only the command line, list what it blocks, and name the forms it doesn't follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The last change scanned the text of every $(…) for a push, but with its quotes already stripped, so `n=$(grep -c 'git push' AGENTS.md)`, `git commit -m "$(echo 'explain the git push gate')"` and the like were refused as pushes the gate can't follow. The tokenizer now marks quoted text inside a $(…), and each substitution is checked with that text replaced by a placeholder; only a substitution that hands it to a shell (`$(bash -c '…')`, `$(sh <<<'…')`, `$(eval '…')`) is checked in full. Two forms the wider pre-filter now tokenizes failed to parse: a heredoc inside $(…) closed with `EOF)"` on one line, and an `$'…'` string holding `\'`. Both parse now; an apostrophe in a comment inside $(…) still doesn't, and the docs say such a line is refused. The test for the here-string spacing inside $(…) now uses `<<<'…'` with no space, so it fails without that fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two code paths from the last change had no test: stripping the quote
markers from a word's value, and reading an $'…' string inside $(…).
Each now has cases that fail when it is removed: a substitution's output
handed to eval or bash -c, and an $'…' string in a $(…), pushed or not.
The marker strip also runs in one pass with tr. Under bash 3.2,
${_w//…} took 47 s on a 200 KB substitution holding 200 quoted strings,
past the hook's time limit; it now takes about a second.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - docs/src/content/docs/development.md: kept origin/main's reworded make vulncheck row and this branch's make verify row (adds review-gate hook tests). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Conflicts: - docs/src/content/docs/development.md: make vulncheck row from the merged-in main (govulncheck -scan package); make verify row from this branch, which already carries the review-gate hook tests plus the integration-split fixtures. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
@coderabbitai review |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
📝 SummarySummary by CodeRabbit
WalkthroughThe integration suite is split into app and backends parts. Make targets, test-selection checks, and CI matrix jobs run these parts separately, with per-part coverage artifacts. Several integration tests now run in parallel. ChangesIntegration test partitioning and CI
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ChangesJob
participant IntegrationPartsScript
participant IntegrationMatrix
participant CoverageJob
ChangesJob->>IntegrationPartsScript: Read and validate Makefile parts
IntegrationPartsScript->>ChangesJob: Return part, artifact, and producer lists
ChangesJob->>IntegrationMatrix: Supply part matrix
IntegrationMatrix->>CoverageJob: Upload per-part coverage artifacts
ChangesJob->>CoverageJob: Supply generated artifact and producer lists
Suggested reviewers: Merge Risk: 🔵 Low · up to The integration suite is now split into app and backend parts that run in parallel and are wired consistently into CI and coverage. The current configuration runs every part. However, the safety check would not detect a future edit that drops a part from the list, which would silently skip those tests. Tightening that check is a small follow-up. 🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Two review findings in the push gate and the marker hook, each with a test that fails without the fix: - The gate took local main as the base when it existed, so commits made on local main by mistake, then branched from (`git switch -c feat`), counted as already on the base and were pushed with no review. The base is now origin/main, and local main only when there's no remote-tracking ref; a stale origin/main only gates more. - The marker hook fell back to the hand-back report whenever the final message lacked either parseable line, so a final `VERDICT: iterate` without a REVIEWED line was replaced by an earlier hand-back's ship_it. It now falls back only when the final message has no verdict or says ship_it without naming the commit. The new-reviewer recipe in AGENTS.md and the Claude Code docs now says to pin with `git -C <worktree> rev-parse HEAD`, as the shipped reviewers do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The header of review-marker.sh still said the final message is the report only when it carries both parseable lines. Since the last change it is the report whenever it has a verdict, unless that verdict is ship_it with no REVIEWED line, and a final iterate or block stands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
📚 Docs preview is live → https://aaccb56a-wavehouse-docs.wave-rf.workers.dev
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 2cb0ae6 in the Show a line coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
0868ece3-c232-4f75-8a11-b2e523c0263c
📒 Files selected for processing (19)
.github/actions/setup-env/action.yml.github/workflows/README.md.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefiledocs/src/content/docs/development.mdscripts/ci/integration-parts.shscripts/ci/integration-parts.test.shscripts/ci/tagged-tests.shscripts/ci/tagged-tests.test.shtests/integration/boot_resilience_test.gotests/integration/ingest_outage_test.gotests/integration/mq_nats_test.gotests/integration/query_errors_test.gotests/integration/shard_claims_test.gotests/integration/shard_order_test.gotests/integration/shared_cache_test.go
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Integration tests (app)
- GitHub Check: Coverage
🧰 Additional context used
📚 Code guidelines (5)
.github/workflows/README.md — auto-discovered
docs/src/content/docs/development.md — auto-discovered
CONTRIBUTING.md — auto-discovered
AGENTS.md — auto-discovered
.github/copilot-instructions.md — auto-discovered
📓 Path-based instructions (8)
Source excerpt: **A dedicated `coverage` job applies the consolidated gate, polling — not `needs`-ing — the suites.**
📄 CodeRabbit inference engine (.github/workflows/README.md)
Files:
.github/workflows/ci.yml
Source excerpt: **Integration test needing Docker** → add a subtest under `tests/integration/` (e.g.
📄 CodeRabbit inference engine (docs/src/content/docs/development.md)
Files:
tests/integration/query_errors_test.gotests/integration/mq_nats_test.gotests/integration/shard_order_test.gotests/integration/boot_resilience_test.gotests/integration/ingest_outage_test.gotests/integration/shard_claims_test.gotests/integration/shared_cache_test.go
Source excerpt: Use [Conventional Commits](https://www.conventionalcommits.org/):
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CONTRIBUTING.md
Source excerpt: Create `*_test.go` files in the same package as the code under test.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
tests/integration/query_errors_test.gotests/integration/mq_nats_test.gotests/integration/shard_order_test.gotests/integration/boot_resilience_test.gotests/integration/ingest_outage_test.gotests/integration/shard_claims_test.gotests/integration/shared_cache_test.go
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
AGENTS.md
Source excerpt: **Strict Go formatting**: Use `gofumpt` (a stricter superset of `gofmt`, enforced by CI).
📄 CodeRabbit inference engine (docs/src/content/docs/development.md)
Files:
tests/integration/query_errors_test.gotests/integration/mq_nats_test.gotests/integration/shard_order_test.gotests/integration/boot_resilience_test.gotests/integration/ingest_outage_test.gotests/integration/shard_claims_test.gotests/integration/shared_cache_test.go
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
CONTRIBUTING.mdCHANGELOG.mdAGENTS.mddocs/src/content/docs/development.md
Source excerpt: Run `make fix` to apply them.
📄 CodeRabbit inference engine (docs/src/content/docs/development.md)
Files:
CONTRIBUTING.mdCHANGELOG.mdAGENTS.mddocs/src/content/docs/development.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-10-07T19:05:35.260Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse
## Operating Rules
3. **Every code change updates its docs + `CHANGELOG.md` in the same PR** — a code change without its doc update is incomplete ([§Documentation Sync](#documentation-sync)).
🪛 checkmake (0.3.2)
Makefile
[warning] 856-856: Target body for "test-integration-app" exceeds allowed length of 5 lines (6).
(maxbodylength)
[warning] 865-865: Target body for "test-integration-backends" exceeds allowed length of 5 lines (11).
(maxbodylength)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~75-~75: The official name of this software platform is spelled with a capital “H”.
Context: ...ed tests run in parallel** (Makefile, .github/workflows/ci.yml, `scripts/ci/{integra...
(GITHUB)
[uncategorized] ~75-~75: The official name of this software platform is spelled with a capital “H”.
Context: ...ims,shard_order,shared_cache}_test.go, .github/workflows/README.md, docs/src/content...
(GITHUB)
[style] ~75-~75: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...e same 276 tests and the same coverage. internal/mq's integration tests were picked by a -run name pattern, so one with a new prefix would have been skipped without a word; scripts/ci/tagged-tests.sh now picks the tests that only build with the integration tag, and every part fails while a package with tagged tests belongs to none, or while the parts CI reads from the Makefile differ from make's own list. internal/mq/natsspike stays in the su...
(TOO_LONG_SENTENCE)
AGENTS.md
[uncategorized] ~152-~152: The official name of this software platform is spelled with a capital “H”.
Context: ...me gates CI will run — the CI workflow (.github/workflows/ci.yml) is a job DAG over th...
(GITHUB)
[uncategorized] ~476-~476: The official name of this software platform is spelled with a capital “H”.
Context: ... check); caches are owned end-to-end by .github/actions/setup-env (nested `actions/cac...
(GITHUB)
docs/src/content/docs/development.md
[style] ~351-~351: Since ownership is already implied, this phrasing may be redundant.
Context: ...ternal/cache`'s integration tests start their own containers instead — Redis, Valkey, Dra...
(PRP_OWN)
[style] ~351-~351: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...uite has a 15-second limit per package. For the same reason the external NATS broker's tests (internal/mq/external*_test.go, including its run of the mqtest conformance suite) and the NATS KV lease tests (internal/mq/lease_test.go, including their run of the coordtest conformance suite) carry the integration tag inside internal/mq, and the backends part runs the tests those tagged files declare (scripts/ci/tagged-tests.sh), whatever their names, so the package's untagged tests stay in the unit suite alone. In tests/integration, most of the slo...
(TOO_LONG_SENTENCE)
[style] ~351-~351: Since ownership is already implied, this phrasing may be redundant.
Context: ..., most of the slow tests that bring up their own servers call t.Parallel()` (never one ...
(PRP_OWN)
[uncategorized] ~715-~715: The official name of this software platform is spelled with a capital “H”.
Context: ...lumes | - CI — the aggregator job of .github/workflows/ci.yml. The workflow is a jo...
(GITHUB)
[uncategorized] ~715-~715: The official name of this software platform is spelled with a capital “H”.
Context: ...che policy, how to add a job — lives in [.github/workflows/README.md](https://github.co...
(GITHUB)
.github/workflows/README.md
[typographical] ~3-~3: The word ‘How’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: How ci.yml is shaped and why. This is the canonical reference — the w...
(WRB_QUESTION_MARK)
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...re owned end-to-end by setup-env** ([.github/actions/setup-env](../actions/setup-env...
(GITHUB)
[style] ~55-~55: Since ownership is already implied, this phrasing may be redundant.
Context: ...r, so a part added to the Makefile gets its own job and fragment with no edit to `ci.ym...
(PRP_OWN)
[style] ~55-~55: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ... ci.yml, and can't be left out of CI. A part is a set of packages: scripts/ci/tagged-tests.sh check, which every part runs first, fails while a package with integration-tagged tests is in none, and internal/mq, whose untagged tests are the unit suite's, runs the tests its tagged files declare (tagged-tests.sh run), not a -run name pattern that a new prefix would silently miss (#692). Each part's job runs exactly the com...
(TOO_LONG_SENTENCE)
[style] ~80-~80: Since ownership is already implied, this phrasing may be redundant.
Context: ...es' objects. go.mod is in the key for its own reason — the compiler's build ID keys e...
(PRP_OWN)
[style] ~121-~121: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ....mod/go.sum` or lockfile bump mints a whole new set while the previous one is still...
(EN_REPEATEDWORDS_WHOLE)
🪛 zizmor (1.30.1)
.github/workflows/ci.yml
[warning] 260-260: use GitHub's dedicated self-repository syntax (self-repository): use '$/...' instead of './...'
(self-repository)
🔇 Additional comments (4)
AGENTS.md (1)
152-152: LGTM!Also applies to: 453-453
CONTRIBUTING.md (1)
42-42: LGTM!docs/src/content/docs/development.md (2)
351-351: LGTM!Also applies to: 358-358, 531-531, 541-541
715-715: 🗄️ Data Integrity & IntegrationThe integration matrix and coverage artifact wiring match the claims in the cited files. No actionable discrepancy remains.
check-integration-parts compared the parts list with itself and handed the tagged-package check a fixed set of packages, so dropping a part from INTEGRATION_PARTS left both passing while its test-integration-<part> target ran in no CI job and no local run. integration-parts.sh now requires the list to match the Makefile's test-integration-<part> targets one for one, and the packages the tagged check treats as covered come from the parts listed (INTEGRATION_PKGS_<part>), so a dropped part also uncovers its packages. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@coderabbitai approve The one finding from your review is fixed in 2cb0ae6 (see the thread). — Posted by Claude Code on behalf of @EricAndrechek |
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedChanges approved. Review threads were not changed. |
|
@coderabbitai review |
|
…757) ## Summary - The Makefile exports `GOTOOLCHAIN` derived from `go.mod`'s `go` directive. `auto` means the newer of the local Go and `go.mod`'s, so a newer local Go (or a future runner image) built on a different toolchain than CI and made `make lint-go` panic. `setup-env` exports the same pin to `$GITHUB_ENV` for CI steps that run Go outside `make`, and its comments that called `auto` exact are corrected. The pin is strict, so `go install` in `make tools` also runs on it. - One script, `scripts/ci/go-toolchain.sh`, derives the pin for both the Makefile and setup-env. It prefers go.mod's `toolchain` line and fails with one message unless the result is a full x.y.z version, since `go mod edit -go=1.27` writes a `go` line Go rejects as a toolchain name. A fixture test (`make test-go-toolchain`) is wired into `make verify`. - `development.md` notes the new coupling: raising the `go` directive to a new minor needs a golangci-lint built with that minor or newer (patch bumps do not), bump it with `go get go@1.N.P`, and `make GOTOOLCHAIN=local` overrides the pin. - `pnpm-install` no longer uses `--reporter=silent`, which prints nothing even on failure. It captures pnpm's default output and replays it only on failure, with `--config.confirm-modules-purge=false` so pnpm's purge prompt cannot hang the captured output on a terminal. The capture-and-replay fix suggested in #578 does not work with `silent`, since there is nothing to capture. ## Test plan - [x] `make ci` green locally. - [x] With `GOTOOLCHAIN` unset and local Go 1.27.1, `make lint-go` passes (it panicked before) and a make recipe running `go version` prints `go1.26.6`. - [x] With `minimumReleaseAge` raised temporarily in `pnpm-workspace.yaml`, `make pnpm-install` now prints `ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION` and exits non-zero; the old command printed nothing. Reverted, not committed. - [x] A successful `make pnpm-install` prints 0 bytes. - [x] `make test-go-toolchain`, `make lint-gha`, `make lint-md`. - [x] Pre-push review: code `ship_it`; docs review `ship_it`. - [ ] First CI run: the "Pin and verify Go toolchain" step prints `go1.26.6 linux/amd64`, and Lint, Unit and Integration stay green. Remaining findings: none. ## Related Issues Closes #580 Closes #578 Part of #740 Stacked on #756 (base `ci/integration-split`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
) Repeated pushes to a PR reused no Go build objects. Measured from CI logs (2026-10-06 to 10-08), that only matters on a PR that changes `go.mod`/`go.sum`, so only that case is cached, and only for the two long-pole jobs. - **Measured, typical PR.** The integration suite's `app` part compiles in 5.8–8.5 s from `main`'s entry, against 2.3–3.2 s from a same-tree one. e2e's 4–7 s cover build is hidden behind its SDK build. A PR-scoped cache would buy about 4–5 s per push, which is inside the `app` part's run-to-run spread, so these PRs still save nothing. - **Measured, dependency-changing PR (#730, #742, #753).** Every push recompiles the dependency delta from `main`'s generation: 35–50 s in the integration suite and a 37–49 s cover build in e2e, against about 3 s and 4 s from the PR's own entry. That is about 30–45 s per push on the critical path. Of 129 PR runs since 2026-09-25, 34 came from 9 such PRs, and 25 of those were second or later pushes. - **Classifier.** `scripts/classify-paths.sh` also emits `go-deps`: any `go.mod`/`go.sum` in the change set, in any module, which are the files the cache keys hash. `classify-changes.sh` passes it through only for a cleanly classified pull request and sets it false everywhere it fails closed. The `changes` job exposes it as an output. - **setup-env.** A new `go-cache-pr-save` input, which the `app` part and e2e set only when `go-deps` is true. So a PR that changes no dependency never saves, even while `main` holds no entry for its key: just after a bump lands on `main`, after a failed `main` run, or for a newly opted-in job. With the input set, the job looks up the exact key first, from the PR's scope or `main`'s, so a PR whose dependency change `main` already has hits `main`'s entry and saves nothing. Only on a miss does it restore the newest entry of its flavor (the PR's own, then `main`'s) and save the exact key into the PR's scope. The key is per dependency state, so a PR saves once per dependency change. Merge-queue refs and branches stay restore-only, and nothing is added to `ci.yml` beyond the two inputs. - **Pruning.** `pr-cache-cleanup.yml` gains a `workflow_run` job that runs after every PR's CI run. It runs the new `scripts/ci/prune-pr-build-cache.sh` from the default branch with `actions: write` and deletes every PR-scoped `gobuild-v3` entry that `main` also holds or that a newer entry of the same flavor supersedes. It deduplicates the paged listing before grouping, since a row returned twice would otherwise delete the PR's newest entry, and a fixture test (`make test-prune-pr-build-cache`, a new `make verify` leaf) pins the selection and the failure handling. No PR code holds the write token. The job needs no PR number, so it covers fork PRs too. A PR holds at most about 0.48 GiB of build cache (double that for the minutes before a prune). - **Budget.** The README sizes this for four such PRs open at once, the most open at once in the last two weeks. The worst case at rest is about 11.7 GiB, about 1.7 GiB over the cap, and eviction by last access takes it from `main`'s superseded generation, which nothing reads (inferred). At two such PRs everything fits. `main`'s live entries stay safe up to six: 3.9 + N × (0.5 CodeQL + 0.48 build) ≤ 10 GiB, assuming the superseded generation is the size of the live one. The six build flavors together are 1.02 GiB on `main`; the issue's "about 1.3 GiB per suffix" was the total for all six of a PR's flavors. ## Test plan This branch's diff matches #783's (same 13 files, +450/−67), which CodeRabbit approved; the only line differences are the verify-leaf list and the `make verify` table row, which differ because the base did. Re-run here: the pre-commit `make verify` (which runs both fixture suites below) and a full `make ci`. Items marked "measured on #783" were not repeated; the CI items are observable once this PR targets `main`. - [x] `make lint-gha` (actionlint, which also checks the new input and the `go-deps` output) and `make lint-sh` clean; `make verify` green in the pre-commit hook, including the new leaf. - [x] `make test-classify-paths`: new `go-deps` cases (root and nested modules, look-alike names, mixed with docs, empty, large input); a mutant that matches only the root `go.mod`/`go.sum` fails it (mutant measured on #783). `classify-changes.sh` against a stub `gh`: `go-deps=true` only for a pull request whose file list holds a `go.sum`; false on push, merge group, dispatch and a failed listing. - [ ] (measured on #783) The setup-env step conditions, evaluated for push to `main`, `pull_request` with and without the input, `merge_group` and `workflow_dispatch`: exactly one restore path runs, and only a PR miss with the input set is save-capable. - [ ] (measured on #783) `prune-pr-build-cache.sh --dry-run` against the live cache list selects 6 entries (1058 MiB): copies of `main`'s keys under #731, #754 and #756. It keeps #753's own entries (one per flavor, not on `main`) and never touches `main` or merge-queue scopes. - [x] `make test-prune-pr-build-cache`: deletes copies of `main`'s key and version and keeps a same key with another version; deletes superseded entries and the older of a same-second pair; keeps `-integration-` apart from `-integration-app-`; ignores other families and `main`/merge-queue scopes; keeps a PR's only entry listed twice. Through a stub `gh`: a 404 reads as already gone, a 403 fails after the rest are attempted, and a failed listing fails. Mutants caught (measured on #783): no early dedup (the pre-fix script), key-only match, merged flavors, no prefix filter, `main` scope included, listing failure ignored, 404 treated as an error, 403 swallowed. - [ ] This PR's own run, which changes no dependency: `Detect changes` logs `go-deps=false`, and every Go job, the `app` part and e2e included, uses the plain restore step, with no `Cache saved` for `gobuild-v3` in any post step. - [ ] After merge, the first `PR cache cleanup` run triggered by a PR's CI (`Prune PR build caches`): passes and deletes the entries the dry run listed, if they are still there. - [ ] The first PR that changes `go.mod`/`go.sum` (the next Dependabot go-deps PR): `Detect changes` logs `go-deps=true`; the first push restores `main`'s generation through restore-keys and saves `gobuild-v3-Linux-go-integration-app-<its hash>` and `-e2e-cov-<its hash>` under `refs/pull/<n>/merge`; the second push exact-hits them, its integration compile is about 3 s and e2e reaches its containers about 20 s after `make` starts; the prune leaves one entry per flavor; the close cleanup deletes them. A fork PR's save is unverified: if GitHub refuses it, the job only logs a warning. - [x] `make ci` passes on this branch, after merging `main` (#793 and #788). ## Related Issues Closes #781. Refs #535 (two generations on `main` still leave thin headroom). Part of #740. Supersedes #783. Now based on `main`: its parent, the `-trimpath` PR #793, has merged. It does not itself use `-trimpath` or `make ci-remote`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
The Integration job grew from about 3 to about 8 minutes with the distributed-backend tests and became CI's critical path (#701). This splits the suite into parts that CI runs as parallel jobs, runs the slow self-contained tests in parallel, and selects
internal/mq's integration tests by build tag instead of by name (#692).make test-integrationrunstest-integration-app(tests/integration) andtest-integration-backends(internal/cache,internal/mq/natsspike,internal/mq's tagged tests) together. Theintegrationjob is a matrix over the Makefile'sINTEGRATION_PARTS, which thechangesjob reads (scripts/ci/integration-parts.shrefuses anything but oneINTEGRATION_PARTS := …line, and every part checks it against make's own list) and also turns into the fragment and producer lists the coverage job waits for. CI runs exactly the commands a local run does, a part added to the Makefile gets its own job with no workflow edit, and each part uploadscoverage-integration-<part>for the coverage job to merge. Every Go job passes its own build-cache suffix (-integration-<part>,-unit, …), since two jobs on one key race to save it and a part would otherwise save a cache holding only its own test binaries.tests/integrationthat bring up their own servers or containers (shard claims, the NATS backend, the shared cache, boot resilience, the ClickHouse outage, the query errors against a stopped ClickHouse) callt.Parallel(), and the app part runs with-parallel 8since they mostly wait.TestShardClaims_ScaleOneThreeTwo, the dedupe DynamoDB tests and the tests that share the wired app are untouched.scripts/ci/tagged-tests.sh runprints the tests, fuzz targets and examplesinternal/mq'sintegration-tagged files declare, replacing the-run '^Test(ExternalNATS|NewNATS|NATSPermissions_Refuse|Leases)'pattern, andtagged-tests.sh check, which every part runs first, fails while a package with tagged tests is in no part.scripts/ci/tagged-tests.test.shchecks the selection against every entrygo test -listprints on a throwaway module, andscripts/ci/integration-parts.test.shchecks the parts reading (bothmake verifyleaves).internal/mq/natsspikestays in the suite: it pins the nats-server behavior the external-NATS topology rests on, and it runs besideinternal/cache, so it adds no wall-clock.Test plan
Timings on a 4-vCPU amd64 Linux box (GitHub's runners have 4 vCPUs too), warm build cache:
make cigreen locally.main: the two gotestsum calls took 339s + 58s = 397s (tests/integration336.7s).make test-integration COV_DEFER=1: 221s, all pass.tests/integration143.9s with the same 276 tests (test set diffed against the baseline run); backends part 76s (cache 14s alongside natsspike 17s, theninternal/mq56s, the same 217 + 95 tests).test-integration-appfour more times: 143.9s to 154.9s. One run failed inTestShardClaims_ScaleOneThreeTwoat the one-writer check (test(integration): TestShardClaims_ScaleOneThreeTwo flakes when a table's rows arrive across a handover after the stop #737); it fails the same way onmain(the baseline run hit it), and it runs in the sequential phase, before any parallel test starts.//go:build integrationTestMQNATSSomethingNewininternal/mq, the old pattern lists 60 tests and misses it; the new selection lists 61. Without it, both select the same 60.test-integration-appruns on a fresh 4-vCPU box: all pass (138–160s warm, 283s for the first, cold build).-count=10of the four parallel shard-claim tests together: 40 of 40 pass.tagged-tests.test.sh(it fails against the old name-only pattern),integration-parts.test.sh,make check-integration-parts, shellcheck and actionlint pass.ship_it; docs reviewship_it.Integration tests (app)andIntegration tests (backends)replaceIntegration tests, the coverage job finds both fragments, and the run takes about 3.5–4 minutes (it was 8–9). The first run on each new cache suffix compiles cold.Remaining findings: none.
Related Issues
Closes #701
Closes #692
Part of #740
Stacked on #755 (base
fix/prepush-gate-holes)🤖 Generated with Claude Code