ci: split ci into a job dag and isolate docs deploys from pr code#312
Conversation
Replace the monolithic make-ci job with a DAG over the same Makefile targets: changes (fail-closed path classification) gates unit / integration / e2e; build uploads the cover binary + SDK dist + docs dist as artifacts; e2e consumes them via E2E_PREBUILT=1; coverage merges the suites' fragments and applies every gate via make cov. An aggregator job named CI (skipped = pass, failed/canceled = fail) is the ruleset's sole required check. The PR-title gate moves into a "PR title" job (trusted-main checkout, API-read title so re-runs see edits); housekeeping.yml drops to non-required and gains an auto re-run of the failed title job after a fixing edit. Docs deploys run in dedicated jobs that check out trusted main and consume only the static docs-dist artifact — the only jobs that reference CLOUDFLARE_* secrets (closes #305). Per-job least-privilege permissions; per-job Go build-cache partitions via the parameterized setup-env composite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
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:
📝 WalkthroughWalkthroughRestructures CI to a change-driven multi-job DAG: adds a ChangesCI Architecture Restructure and Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
📚 Docs preview is live → https://fcbeaa29-wavehouse-docs.wave-rf.workers.dev
|
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e5286d84-b2bf-43c4-aeef-f89cee960050
📒 Files selected for processing (12)
.claude/hooks/agent-bash-gate.sh.github/actions/setup-env/action.yml.github/workflows/ci.yml.github/workflows/housekeeping.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefiledocs/src/content/docs/claude-code.mddocs/src/content/docs/development.mddocs/wrangler.jsoncscripts/lint-pr-title.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E tests
- GitHub Check: Docs preview
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,json,md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Biome owns JS/TS/JSON linting and formatting; markdownlint owns Markdown style; misspell owns spelling — all under
make lint/make fix
Files:
CONTRIBUTING.mddocs/src/content/docs/claude-code.mdCHANGELOG.mddocs/src/content/docs/development.mdAGENTS.md
docs/src/content/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/**/*.{md,mdx}: Mermaid diagrams in docs must be authored vertically (top-down) to fit the page column width (~46–58rem); useflowchart TB/TDand keep node labels short with<br/>for line breaks
Never place two large Mermaid diagrams side-by-side; wrap comparisons in<div class="diagram-pair">…</div>so each gets full column width
Markdown files in docs must pass markdownlint style checks; usemake lintto verify
Files:
docs/src/content/docs/claude-code.mddocs/src/content/docs/development.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Validate locally before every push by running `make ci` the documented way, using the background Bash tool with `NO_COLOR=1` and avoiding foreground execution
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Ensure every PR-branch push satisfies all pre-push reviewers discovered from `scripts/pre-push-reviewers.sh` by running `/prepush`, which loops until each reviewer returns `ship_it`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Update documentation and `CHANGELOG.md` in the same PR as any code changes; code changes without corresponding doc updates are incomplete
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Address and resolve every review finding with a substantive reply, fix, or tracked issue; never silently drop a review comment and always `@-mention` the bot on its own line
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Create PRs as drafts only via `gh pr create --draft`; the PR title must pass Conventional Commits validation (≤ 72 chars, lowercase subject, no trailing period)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Never force-push or rebase a PR branch; use `git merge origin/main --no-edit` to absorb upstream changes
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Never hand-write markers in `tmp/` or use `--no-verify` on git commits and pushes; if tempted, fix the gate itself instead of bypassing it
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Comment the *why*, not the *what*; avoid comments that merely restate the code, keep comments to 1–2 lines, and match the file's existing density
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: DRY principle: maintain one source of truth; factor duplicated rules into shared helpers and constants rather than duplicating logic
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Leave code neater than you found it within reason; fix small safe improvements (stale comments, typos, misnamed locals, dead code) on your path without major cleanups
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Maintain 80% code coverage project-wide (merged unit + integration + e2e); enforce per-suite minima of unit 70%, integration 12%, e2e 50%, SDK 50%
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Every new function should have corresponding test cases; run `make lint` and `make test` before considering work complete
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: JWT secret or JWKS endpoint must be cryptographically strong in production; the JWT middleware always runs with no enable flag, making token validation the sole gate on elevated access
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: TypeScript SDK (`wavehouse/sdk`) is the canonical client; update it whenever backend changes alter the public API surface (endpoints, event format, auth, query AST, live-query aggregation, pipes, policy)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: When modifying a core package, preserve the named invariant numbers from `docs/src/content/docs/architecture.md` — they are stable cross-references used in code comments and architecture docs
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Add a matching `area/<pkg>` repo label when creating a new internal package (e.g., `area/foo` for `internal/foo/`); `triage.yml` discovers labels at runtime via `gh label list`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Run `make tools` to install team-wide git hooks via `git config core.hooksPath .githooks`; these apply to both humans and Claude Code
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T02:33:17.384Z
Learning: Validate PR titles locally before creation using `scripts/lint-pr-title.sh "<title>"`; the title must be Conventional Commits format (≤ 72 chars, lowercase subject, no trailing period)
📚 Learning: 2026-05-19T18:14:08.727Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 147
File: .claude/settings.json:35-38
Timestamp: 2026-05-19T18:14:08.727Z
Learning: In Claude Code hook scripts (e.g., WaveHouse’s `.claude/hooks/`), don’t rely on `PostToolUse:Agent` for verdict parsing: it exposes subagent output as a structured JSON object at `.tool_response.content[].text`, so regex-based “VERDICT:” parsing is unreliable. Use the `SubagentStop` event instead: parse `VERDICT:` lines from the flat string at `.last_assistant_message`, and use `agent_type` to filter to the intended subagent (since `SubagentStop` has no `matcher` support—do any filtering in the script).
Applied to files:
.claude/hooks/agent-bash-gate.sh
📚 Learning: 2026-05-19T18:26:33.503Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 147
File: .claude/hooks/review-marker.sh:0-0
Timestamp: 2026-05-19T18:26:33.503Z
Learning: In `.claude/hooks/review-marker.sh`, treat `jq` parse errors and “missing-jq” cases as intentional non-fatal marker-writer behavior: use `exit 0` (not `exit 2`). This hook is a marker writer, not an enforcement gate—the absence of `tmp/review-passed-<sha>` is the downstream signal consumed by `agent-bash-gate.sh` and `.githooks/pre-push`. Do not change these `exit` codes or stderr messaging (e.g., ending diagnostics with “— no marker written”), since `exit 2` would incorrectly conflate hook misbehavior with deliberate `iterate`/`block` verdicts.
Applied to files:
.claude/hooks/agent-bash-gate.sh
🪛 checkmake (0.3.2)
Makefile
[warning] 661-661: Target body for "test-e2e" exceeds allowed length of 5 lines (6).
(maxbodylength)
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~79-~79: The official name of this software platform is spelled with a capital “H”.
Context: ...e required CI check's PR title job (.github/workflows/ci.yml): Conventional Commit...
(GITHUB)
docs/src/content/docs/development.md
[uncategorized] ~543-~543: The official name of this software platform is spelled with a capital “H”.
Context: ...tlejob under the requiredCI check (.github/workflows/ci.yml`); validate locally wi...
(GITHUB)
[uncategorized] ~559-~559: The official name of this software platform is spelled with a capital “H”.
Context: ... merge: - CI — the aggregator job of .github/workflows/ci.yml. The workflow is a jo...
(GITHUB)
AGENTS.md
[uncategorized] ~141-~141: 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] ~422-~422: The official name of this software platform is spelled with a capital “H”.
Context: ... commit SHAs with version comments (see .github/workflows/ci.yml, release.yml). New ...
(GITHUB)
[uncategorized] ~422-~422: The official name of this software platform is spelled with a capital “H”.
Context: ...r inline bash or official actions/* / github/* actions when feasible (e.g. the PR-t...
(GITHUB)
[uncategorized] ~429-~429: The official name of this software platform is spelled with a capital “H”.
Context: ...ge on until resolved. - Dependabot (.github/dependabot.yml): opens weekly grouped ...
(GITHUB)
[uncategorized] ~430-~430: The official name of this software platform is spelled with a capital “H”.
Context: ... docs-deploy jobs of the CI workflow (.github/workflows/ci.yml), not Cloudflare'...
(GITHUB)
🪛 zizmor (1.25.2)
.github/workflows/housekeeping.yml
[error] 33-33: overly broad permissions (excessive-permissions): issues: write is overly broad at the workflow level
(excessive-permissions)
[error] 34-34: overly broad permissions (excessive-permissions): pull-requests: write is overly broad at the workflow level
(excessive-permissions)
[error] 35-35: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
[warning] 33-33: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
.github/workflows/ci.yml
[warning] 167-167: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 214-216: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 291-291: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 322-322: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 354-354: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 402-402: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 79-79: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 84-84: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 85-85: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 88-88: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 88-88: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 100-100: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 144-144: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 69-69: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[warning] 129-129: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[warning] 478-481: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 471-471: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
[warning] 673-673: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🔇 Additional comments (23)
Makefile (1)
654-661: LGTM!.claude/hooks/agent-bash-gate.sh (1)
60-60: LGTM!AGENTS.md (1)
141-141: LGTM!Also applies to: 233-233, 422-422, 426-426, 428-428, 430-430, 435-435, 437-437
CHANGELOG.md (1)
12-15: LGTM!CONTRIBUTING.md (1)
79-79: LGTM!docs/src/content/docs/claude-code.md (1)
106-106: LGTM!docs/src/content/docs/development.md (2)
543-554: LGTM!
557-562: LGTM!docs/wrangler.jsonc (1)
6-17: LGTM!scripts/lint-pr-title.sh (1)
2-7: LGTM!Also applies to: 14-14, 22-22
.github/workflows/ci.yml (5)
51-114: LGTM!
116-159: LGTM!
281-391: LGTM!
393-437: LGTM!
705-741: LGTM!.github/workflows/housekeeping.yml (3)
27-35: LGTM!
64-155: LGTM!
157-188: LGTM!.github/actions/setup-env/action.yml (5)
1-39: LGTM!
43-61: LGTM!
63-100: LGTM!
102-137: LGTM!
139-196: LGTM!
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
CodeRabbit review: persist-credentials: false on all nine checkouts, including the secret-bearing docs deploy jobs (#305 defense in depth). The docs-preview comment step's fallback `git fetch origin <sha>` works anonymously on a public repo, so nothing needs the persisted token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reshape the job DAG for wall-clock: every job now hangs directly off the ~8s changes job, nothing waits on another runner's build. - e2e builds its own SDK dist + cover binary (make -j test-e2e) on a warm per-suffix cache instead of waiting ~95s for the build job; the ClickHouse image pulls in the background during setup (integration too). Cache suffix bumped to -e2e-cov so the cover objects actually get saved (the old -e2e key would exact-hit and never re-save). - The consolidated coverage merge + gates move into e2e's tail: the unit/integration fragments upload ~60s before the e2e suite ends, so a short poll + download + make cov replaces a whole runner spin-up (the old coverage job spent 49s of its 60s on setup). - build slims to docs-build (make build-docs, no Go), runs only on docs-affecting changes, and keeps the pnpm/playwright/astro cache saves; compile breakage stays covered by lint/tests/cover-binary link, release builds by goreleaser-validate / publish-dev. Critical path before: build(95s) -> e2e(188s) -> coverage(60s) ~= 5m54s. Predicted after: e2e(~190s incl. gate) ~= 3m15s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Unreleased entry still described the first-commit DAG (build job uploading cover binary/SDK dist, E2E_PREBUILT consumption, a separate coverage job). Rewrite it to match the shipped shape: e2e self-builds and owns the merged coverage gate, build slimmed to docs-build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
681-692:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the production deploy to a GitHub Environment.
Without
environment: production, these Cloudflare credentials cannot be environment-scoped or protected by deployment rules, so the production token stays broader than this hardening pass implies. Attachdocs-deployto a production environment and move the prod secrets there.🛡️ Proposed fix
docs-deploy: name: Docs deploy + environment: + name: production needs: [changes, lint, docs-build, unit, integration, e2e]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 634803fe-5e73-440d-be81-33888834a31e
📒 Files selected for processing (4)
.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mddocs/src/content/docs/development.md
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (3)
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.md: Document all new/modified API endpoints, config options, architecture changes, ingest/event format changes, deployment changes, and build/test process changes in corresponding docs files (see Documentation Sync table)
Author Mermaid diagrams vertically (flowchart TB/TD, direction TB in subgraphs); keep node labels short; use semantic classes (wh, win, pain, fail, infra, neutral, store, client); reserve LR for short chains (≤3–4 nodes); never sit two large diagrams side-by-side
Files:
docs/src/content/docs/development.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every notable code change updates CHANGELOG.md under [Unreleased] in the same PR
Files:
CHANGELOG.md
.github/workflows/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/*.yml: Third-party actions must be pinned to full commit SHAs with version comments; prefer inline bash or official actions/* / github/* actions; never@mainor floating tags
Change-detection jobs use inline gh api, not third-party paths-filter actions; PR-title checks use inline bash calling scripts/lint-pr-title.sh (single source of truth used by local gate, CI, housekeeping)
Files:
.github/workflows/ci.yml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Comment the why, not the what; add comments only when reason is not obvious from code; keep to 1–2 lines; match file's existing density; never write three lines of comment for one line of code; the why usually belongs in commit message/PR/CHANGELOG.md
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: DRY — one source of truth: look for existing helper/type/constant to reuse before duplicating; factor rules into one place every caller reads; scripts/lint-pr-title.sh, scripts/docs-prose.sh, scripts/pre-push-reviewers.sh are canonical sources
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Leave it neater than you found it — within reason: fix stale comments, typos, misnamed locals, dead code on your path; keep cleanups in same spirit/size as change for reviewable diffs; if cleanup is large/risky, open tracking issue instead
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Interface-first: core behaviors are Go interfaces (Cache, Deduplicator, Publisher, Subscriber); standalone vs. future-clustered swap implementations
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Async ingestion: POST /v1/ingest?table={table} returns 200 after optional dedup + MQ publish; ClickHouse writes happen later via StartIngestWorker; NATS full → 503 + Retry-After
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Dead Letter Queue: failed batch inserts publish to WAVEHOUSE_DLQ (dlq.<table>), gated by dlq.enabled; no silent data loss
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Active Sweeper: purges NATS messages that are both ACKed (written to CH) and older than gap window; SSE gap-fill uses DeliverByStartTime, no in-process ring buffer
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Validate locally before every push — run make ci the documented way before pushing; don't use CI as first feedback loop
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: A PR-branch push needs every pre-push reviewer satisfied — run /prepush, which discovers reviewers from scripts/pre-push-reviewers.sh, runs ones the change needs in parallel (fresh context), skips any with nothing to do on the record, and loops until each returns ship_it
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Every code change updates its docs + CHANGELOG.md in the same PR — a code change without doc update is incomplete
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Address and resolve every review finding — substantive reply, fix it or track it in an issue, `@-mention` the bot, then resolve; never silently drop one
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Drafts only; valid title — gh pr create --draft (never gh pr ready/approve); PR title must pass Conventional-Commits gate (≤ 72 chars, check with scripts/lint-pr-title.sh); never hand-write markers or use --no-verify
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Never force-push or rebase a PR branch — to absorb upstream main, git merge origin/main
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Every new function should have corresponding test cases; run make lint and make test before considering work complete
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Coverage target: 80% project-wide (CI enforces threshold.total in .testcoverage.yml); per-suite minima: unit 70%, integration 12%, e2e 50%, sdk 50%; aim for 80%+ on new code
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T03:33:39.691Z
Learning: Dependency vulnerability scanning: govulncheck ./... runs in CI on every push/PR; Dependabot opens weekly grouped PRs for outdated Go modules and GitHub Actions; no auto-merge (every bump needs human admin review)
🪛 LanguageTool
docs/src/content/docs/development.md
[uncategorized] ~559-~559: The official name of this software platform is spelled with a capital “H”.
Context: ... merge: - CI — the aggregator job of .github/workflows/ci.yml. The workflow is a jo...
(GITHUB)
AGENTS.md
[uncategorized] ~141-~141: 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)
[style] ~426-~426: Since ownership is already implied, this phrasing may be redundant.
Context: ...he merged gate costs seconds instead of its own runner), docs-build (`make build-docs...
(PRP_OWN)
[uncategorized] ~429-~429: The official name of this software platform is spelled with a capital “H”.
Context: ...ge on until resolved. - Dependabot (.github/dependabot.yml): opens weekly grouped ...
(GITHUB)
[uncategorized] ~430-~430: The official name of this software platform is spelled with a capital “H”.
Context: ... docs-deploy jobs of the CI workflow (.github/workflows/ci.yml), not Cloudflare'...
(GITHUB)
🔇 Additional comments (5)
AGENTS.md (1)
141-141: LGTM!Also applies to: 426-426, 430-430
CHANGELOG.md (1)
14-14: LGTM!docs/src/content/docs/development.md (1)
543-553: LGTM!.github/workflows/ci.yml (2)
78-118: LGTM!Also applies to: 126-163, 734-764
478-673: LGTM!
actions/setup-go spent ~9s per Go job downloading a toolchain the runner can already resolve: the image's go + GOTOOLCHAIN=auto fetch go.mod's pinned version into ~/go/pkg/mod/golang.org/toolchain, which the gobuild cache already persists — so warm runs download nothing. Key bumped to v2 (exact-key-hit saves would otherwise never absorb the toolchain), with unversioned prefixes kept as transitional fallbacks. Run timings after the previous commit: push -> all green 3m34s (was 5m54s); e2e path = 38s setup + 133s suite (19s builds+container, 100s vitest, 10s teardown). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hot path Three structural changes squeeze the critical path toward the slowest single test file instead of the suite sum: - e2e sharding: scripts/e2e-shards.sh runs concurrent orchestrators, each with its own ClickHouse testcontainer + wavehouse-cov (per-shard scratch paths via E2E_SHARD; shared GOCOVERDIR — covcounters are pid-stamped; TS shard coverage nyc-merged back into ts-e2e/). Within one server the files must stay sequential (#214 shared policy state); across servers they need not. Suite wall-clock ~100s -> ~40s (slowest shard = ingest.test.ts). Local default stays one orchestrator; E2E_KEEP_CH=1 lets CI leave container teardown to the reaper. Shard env vars are regexp-validated (gosec G702/G703/G706 taint). - e2e has no needs: it classifies the change set itself via the new shared scripts/ci-classify-changes.sh (also used by the changes job) and starts the moment the run is created; the aggregator cross-checks the two classifications so drift fails closed instead of silently skipping the merged coverage gate. - docs-preview polls for the docs-dist artifact instead of needs-waiting on docs-build, so its trusted-main setup overlaps the build; docs-build drops the duplicate check-docs pass (lint runs it in parallel) and fetches shallow on PRs (lastUpdated accuracy only ships from main). Validated locally: 3-shard run green twice (~1m04 wall), merged e2e coverage gate passes (covdata from 3 binaries + nyc-merged TS shards). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
go
Maintainability pass over the restructured CI; behavior-preserving on the hot path (steady-state wall-clock unchanged). - .github/workflows/README.md is THE architecture doc: mermaid DAG, seven design invariants (sole required check, poll-not-needs, dual classifier + drift guard, edge-free e2e, shard rationale, trust domains, setup-env-owned caches), cache inventory + key-versioning policy, timing reference, add-a-job recipe, debugging guide, and the transition TODOs. AGENTS.md / development.md trim to summaries that point at it; ci.yml's header does too. - No bash programs in YAML: workflow logic moves to a shellchecked scripts/ci/ namespace — classify-changes.sh (moved), wait-artifact.sh (generic poll, replaces e2e's inline loop), docs-preview-comment.sh (the 120-line sticky-comment builder; the calling step guards for the one-PR window where main lacks the script), timing-summary.sh. The deploy jobs keep short inline glue on purpose: they execute only main-resolved files, and inline YAML is the reviewed surface. - setup-env owns caches end-to-end via nested actions/cache (automatic post-job save on exact-key miss) — every per-job save step in ci.yml is gone, and save/restore can no longer drift. Trade-off documented: failed jobs don't save; restore-keys cushion the next run. - New non-gating "Timing summary" job writes a per-job wall-clock table to every run's Summary page (continue-on-error; not in the aggregator's needs, so zero critical-path cost). - make verify gains lint-sh (shellcheck v0.11.0, checksum-verified fetch via scripts/install-shellcheck.sh) and lint-gha (actionlint v1.7.12, go-install pattern) — the scripts and workflows now gate like any other source. Pre-existing findings fixed across scripts/otel/*, scripts/ci-marker.sh (justified disable: the expand-now trap is deliberate), and triage.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 994d9eec-b2ff-4707-9927-1c04619fa4a1
📒 Files selected for processing (20)
.github/actions/setup-env/action.yml.github/workflows/README.md.github/workflows/ci.yml.github/workflows/triage.ymlAGENTS.mdCHANGELOG.mdMakefiledocs/src/content/docs/development.mdscripts/ci-marker.shscripts/ci/classify-changes.shscripts/ci/docs-preview-comment.shscripts/ci/timing-summary.shscripts/ci/wait-artifact.shscripts/e2e-shards.shscripts/install-shellcheck.shscripts/orchestrator/main.goscripts/otel/aspire.shscripts/otel/grafana.shscripts/otel/otel-front.shtests/e2e/sdk/vitest.config.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
scripts/**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Use shellcheck for Bash script linting via make lint-sh
Files:
scripts/ci-marker.shscripts/otel/otel-front.shscripts/otel/aspire.shscripts/otel/grafana.shscripts/install-shellcheck.shscripts/ci/wait-artifact.shscripts/ci/classify-changes.shscripts/ci/timing-summary.shscripts/e2e-shards.shscripts/ci/docs-preview-comment.sh
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/**/*.yml: Use actionlint for GitHub Actions workflow validation via make lint-gha
Third-party GitHub Actions must be pinned to full commit SHAs with version comments (never@mainor floating tags)
Prefer inline bash or official actions/* / github/* actions over third-party actions in workflows
Files:
.github/workflows/triage.yml.github/workflows/ci.yml
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use npm workspaces with pnpm (≥ 11.1) and Node 22 LTS for TypeScript/JavaScript projects
Files:
tests/e2e/sdk/vitest.config.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for linting and formatting TypeScript/JavaScript files
Files:
tests/e2e/sdk/vitest.config.ts
tests/e2e/sdk/vitest.config.ts
📄 CodeRabbit inference engine (AGENTS.md)
Configure E2E test files to run sequentially (maxWorkers: 1) in vitest.config.ts due to shared global policy state
Files:
tests/e2e/sdk/vitest.config.ts
**/*.{md,go,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use misspell for spelling validation
Files:
tests/e2e/sdk/vitest.config.tsAGENTS.mdCHANGELOG.mddocs/src/content/docs/development.mdscripts/orchestrator/main.go
**/*.{go,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Comment the why, not the what; add comments only when the reason isn't obvious from code; keep comments to 1–2 lines
Files:
tests/e2e/sdk/vitest.config.tsscripts/orchestrator/main.go
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use markdownlint for Markdown style linting
Files:
AGENTS.mdCHANGELOG.mddocs/src/content/docs/development.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Update CHANGELOG.md under [Unreleased] for every notable code change in the same PR
Files:
CHANGELOG.md
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.md: Author Mermaid diagrams vertically (flowchart TB/TD) to fit the docs column width (~46–58rem); reserve LR for short chains only
Use…wrapper for side-by-side Mermaid diagram comparisons to stack them vertically for readability
Keep Mermaid node labels short; use
for second line rather than one long line
Use semantic Mermaid node classes (wh, win, pain, fail, infra, neutral, store, client) instead of inline colors
Mermaid diagrams are click-to-zoom on the site; fine detail is recoverable but diagrams should be legible inline
Files:
docs/src/content/docs/development.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code with gofumpt
Wrap errors with fmt.Errorf("context: %w", err)
Use structured logging with log/slog with JSON handler
Use goimports for Go import organization
Use govulncheck for Go vulnerability scanning
Never use global state; pass dependencies explicitly via constructor injection
Files:
scripts/orchestrator/main.go
.github/workflows/ci.yml
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/ci.yml: CI workflow logic lives in scripts/ci/*.sh gated by make lint-sh; never add inline YAML logic except trusted-main deploy jobs
Read .github/workflows/README.md before editing ci.yml for DAG architecture, cache policy, and job-addition recipe
CI's required status check is the aggregator job named 'CI'; fails on failed/cancelled needs, treats skipped as passing, cross-checks change classifiers
Caches are owned by .github/actions/setup-env (nested actions/cache with auto post-job saves); never add save steps to ci.yml
Artifact consumers poll via scripts/ci/wait-artifact.sh instead of needs-waiting; long-pole e2e job has no needs and classifies change set itself
E2E job runs as concurrent orchestrator shards via scripts/e2e-shards.sh; applies merged coverage gate via make cov at tail
Docs site builds via CI's docs-build job on a runner with cached Chromium (produces docs/dist/ artifact); deploy jobs run from trusted main checkout only
Docs deploy skips when no docs-affecting files changed and on fork PRs; requires CLOUDFLARE_API_TOKEN + CLOUDFLARE_ACCOUNT_ID secrets
Files:
.github/workflows/ci.yml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Run make ci before every push with NO_COLOR=1 and background execution to validate locally
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Run /prepush command to invoke all pre-push reviewers in parallel for PR branches before pushing
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Create PRs with gh pr create --draft (never gh pr ready for agents); only humans transition to ready-for-review
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: PR title must pass Conventional Commits gate: <type>(optional-scope)(optional-!): <subject> with ≤72 chars, lowercase-first, no trailing period
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Validate PR title before creating with scripts/lint-pr-title.sh "<title>"
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Never force-push or rebase a PR branch; use git merge origin/main to absorb upstream changes
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Every code change must update corresponding docs in the same PR; code without doc updates is incomplete
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Address every review finding with a substantive reply, fix it, track it in an issue with mention, then resolve the thread
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Maintain DRY — one source of truth; reuse existing helpers, types, constants before duplicating logic
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Leave code neater than you found it — fix small safe things in passing (stale comments, typos, misnamed locals, dead code) within reason
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Point Kubernetes probes at /livez and /readyz; SDK/online-checks at /v1/health; never use deprecated /health and /ready aliases
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Add area/<pkg> repo label for new internal package (e.g. area/foo for internal/foo/) so triage workflow routes issues automatically
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T05:20:28.679Z
Learning: Cloudflare Workers Builds must stay disconnected from wavehouse-docs Worker (manual CI deploy via wrangler avoids double-deploy on Workers Builds image without Chromium)
🪛 checkmake (0.3.2)
Makefile
[warning] 708-708: Target body for "test-e2e" exceeds allowed length of 5 lines (9).
(maxbodylength)
🪛 LanguageTool
.github/workflows/README.md
[typographical] ~3-~3: The word ‘How’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...itecture How ci.yml is shaped and why. This is the canonical reference — the w...
(WRB_QUESTION_MARK)
[style] ~58-~58: Since ownership is already implied, this phrasing may be redundant.
Context: ...r to finish. Instead a consumer does its own setup in parallel and calls [`script...
(PRP_OWN)
[uncategorized] ~104-~104: The official name of this software platform is spelled with a capital “H”.
Context: ...owned end-to-end by setup-env** ([.github/actions/setup-env](../actions/setup-env...
(GITHUB)
[style] ~154-~154: 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: ... add cache save steps (invariant 7). 4. Need a build product from another job? Uploa...
(EN_REPEATEDWORDS_NEED)
AGENTS.md
[uncategorized] ~426-~426: The official name of this software platform is spelled with a capital “H”.
Context: ...closed); caches are owned end-to-end by .github/actions/setup-env (nested `actions/cac...
(GITHUB)
docs/src/content/docs/development.md
[uncategorized] ~559-~559: The official name of this software platform is spelled with a capital “H”.
Context: ... merge: - CI — the aggregator job of .github/workflows/ci.yml. The workflow is a jo...
(GITHUB)
[uncategorized] ~559-~559: 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)
🔇 Additional comments (64)
scripts/install-shellcheck.sh (1)
1-37: LGTM!scripts/ci-marker.sh (1)
22-22: LGTM!.github/workflows/triage.yml (1)
43-43: LGTM!Also applies to: 115-116, 155-156
scripts/otel/aspire.sh (1)
29-29: LGTM!scripts/otel/grafana.sh (1)
29-29: LGTM!scripts/otel/otel-front.sh (1)
27-27: LGTM!scripts/ci/timing-summary.sh (1)
1-38: LGTM!.github/workflows/README.md (1)
1-184: LGTM!.github/actions/setup-env/action.yml (5)
46-64: LGTM!
83-101: LGTM!
114-128: LGTM!
130-151: LGTM!
153-186: LGTM!.github/workflows/ci.yml (12)
1-45: LGTM!
55-77: LGTM!
85-122: LGTM!
125-144: LGTM!
154-198: LGTM!
204-227: LGTM!
229-259: LGTM!
276-366: LGTM!
389-533: LGTM!
541-586: LGTM!
594-634: LGTM!
642-670: LGTM!scripts/ci/classify-changes.sh (4)
24-28: LGTM!
29-41: LGTM!
45-51: LGTM!
52-59: LGTM!scripts/ci/wait-artifact.sh (3)
1-30: LGTM!
34-42: LGTM!
43-60: LGTM!Makefile (7)
176-187: LGTM!
387-401: LGTM!
478-478: LGTM!
569-577: LGTM!
701-716: LGTM!
896-900: LGTM!
943-953: LGTM!scripts/e2e-shards.sh (7)
33-37: LGTM!
39-53: LGTM!
55-59: LGTM!
61-65: LGTM!
67-84: LGTM!
86-93: LGTM!
95-104: LGTM!scripts/orchestrator/main.go (8)
23-40: LGTM!
69-75: LGTM!
101-115: LGTM!
117-130: LGTM!
148-160: LGTM!
183-183: LGTM!
234-264: LGTM!
340-340: LGTM!tests/e2e/sdk/vitest.config.ts (1)
30-33: LGTM!docs/src/content/docs/development.md (4)
337-337: LGTM!
542-543: LGTM!
553-553: LGTM!
559-559: LGTM!scripts/ci/docs-preview-comment.sh (5)
1-31: LGTM!
33-49: LGTM!
51-74: LGTM!
76-88: LGTM!
90-103: LGTM!
Scope reduction per review: the cleverness that bought seconds wasn't worth the complexity, and CI's e2e should run exactly what a developer runs. - e2e sharding removed: orchestrator and vitest config revert to main, scripts/e2e-shards.sh deleted, Makefile test-e2e back to the plain single-orchestrator recipe (E2E_PREBUILT/E2E_SHARDED/E2E_KEEP_CH knobs gone). The design and measured numbers live on under "Deferred optimizations" in .github/workflows/README.md, pointing at ed1db1c for a working implementation if the suite's wall-clock bites again. - e2e is changes-gated like the other suites again: the self-classify step, `ran` output, aggregator drift check, and per-step conditions are gone (was worth ~5s). - docs-preview needs docs-build again (its poll only paid off when e2e ran sharded); docs-build runs plain `make build-docs` with full history (DOCS_SKIP_CHECK and the PR-shallow fetch ternary removed). - CodeRabbit fixes: classify-changes.sh upgraded to `set -euo pipefail` (gh calls stay soft — empty output already fails closed); docs-preview-comment.sh uses $marker in the jq filter and fails loudly when the comment API calls fail. The persist-credentials and product-casing comments were already addressed or point at file-path false positives; the ClickHouse :latest pin matches the existing pins in both test suites and stays a separate follow-up. Expected wall-clock: ~3m push -> green (gives back ~60s vs sharded; keeps the structural wins: coverage-gate fold, self-built e2e inputs, no setup-go, owned caches, image prefetch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
removed
go
The queue re-tests each PR against current main at landing time (the merge-group ref), which is the real version of what the ruleset's "require branches to be up to date" rule only approximated — and it removes the manual update-branch -> wait -> merge loop after every sibling merge. The trigger is inert until the ruleset gains its merge_queue rule; that flip must happen AFTER this lands on main (documented in the README's transition notes, prepared payload in tmp/ruleset-merge-queue.json): enabling it first would strand every other queued PR with no CI run on its merge group. merge_group runs flow through the existing DAG: the classifier treats them like pushes (full suite, never skips code checks; docs grep still gates docs-build), while title (validated on the PR), docs-preview (PR-scoped), and docs-deploy (push-scoped) sit out as skips the aggregator already counts as passes. The queue never pushes to the PR branch, so require_last_push_approval is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review-round refinements to the CI-DAG PR: - Split the consolidated coverage gate out of e2e's tail into its own `coverage` job (needs unit+integration+e2e). Decouples the gate from the e2e suite's pass/fail and lets the DAG barrier wait on whichever suite finishes last (integration occasionally outlasts e2e) with no in-run polling — exactly like local `make ci`'s final `make cov` step. Each suite now uploads a `coverage-<suite>` fragment; the new job adds `coverage` to both the aggregator's and docs-deploy's `needs` so a coverage failure blocks merge and prod deploys. Drops wait-artifact.sh. - Extract the change classifier's pure core into scripts/classify-paths.sh (file list on stdin -> code/docs), unit-tested by scripts/classify-paths.test.sh (`make test-classify-paths`, a verify leaf). scripts/ci/classify-changes.sh becomes a thin CI wrapper (API fetch + fail-closed policy). One allowlist, now regression-tested. - Move the PR-title check into scripts/ci/check-pr-title.sh (shellcheck- gated), with a transition guard since the `title` job runs from the trusted-main checkout where the script isn't present until merge. - Scale the pre-push hook to the change set via the shared classifier: a code change still needs the full `make ci` marker; a docs/prose-only push needs only the `make verify` marker (the suites CI skips for those too). Fail-closed to `make ci` when the push can't be classified. Docs (README, AGENTS, CONTRIBUTING, development.md, claude-code.md, CHANGELOG) updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- AGENTS.md per-suite coverage minima were stale (unit 70 / integration 12 / e2e 50) — correct to the actual .testcoverage.yml floors (unit 80 / integration 20 / e2e 60; sdk/ts-total 50). Flagged by docs-reviewer. - Bump the push->green wall-clock from the pre-coverage-job ~3m45s estimate to the measured ~4m (4m11s on the first run with the standalone coverage job: e2e ~170s + coverage ~62s tail + ~4s aggregator). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The coverage job needed [unit, integration, e2e], but a `needs` edge is a scheduling barrier — GitHub won't pick up a runner, check out, restore caches, or `pnpm install` until the needed jobs finish. That serialized the job's ~50s of setup onto the critical path after the last suite, for nothing: the setup doesn't depend on the suites' results, only the merge does. Switch to `needs: changes` only and poll for the three coverage fragments with scripts/ci/wait-artifact.sh (recovered — it was deleted when this job was first split out). Setup now overlaps the suites; the merge fires within ~10s of e2e finishing. Critical-path tail: ~10s, not ~50s (~3m15s push->green, down from ~4m). Because the poll now runs for e2e's whole duration (the long pole), it's hardened against transient API blips: each `gh api` is guarded so a single 5xx/rate-limit skips one iteration instead of reddening a green run; only a persistent outage past --timeout (30 min, to outwait e2e's 25-min cap plus queue skew) fails. The producer-check still fast-fails a genuinely failed/cancelled e2e. The coverage runner idle-polls while the suites run (free on public runners). The aggregator and docs-deploy keep every suite in `needs` directly, so a suite failure still reds the gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Cloudflare preview deploy doesn't validate anything merge-correctness depends on — `docs-build` validates the build (and stays a required need), and the preview deploys from trusted `main`, so it doesn't even exercise a PR's own deploy config. Gating on it only added latency (its Cloudflare deploy is variable, 1.5–3min, and was the run's co-long-pole on slow days) and Cloudflare-flakiness exposure to the required check. Drop `docs-preview` from the `CI` aggregator's `needs` (now non-gating, like `timing`). It still runs and reports its own "Docs preview" check — red on failure, with the sticky URL on success — but no longer delays or reds the required `CI` check. The required gate now concludes reliably on the e2e+coverage+docs-build path (~3m15s) regardless of the deploy. Production `docs-deploy` (post-merge main push) stays gating. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/ci/wait-artifact.sh (1)
21-30: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Add numeric validation for
--timeoutparameter.If a non-numeric value is passed (e.g.,
--timeout abc), the arithmetic expansion at line 41 will fail with a cryptic bash error. Adding a simple validation would provide clearer feedback.✨ Optional improvement
[ -n "$artifacts" ] || { echo "wait-artifact: --artifacts is required" >&2; exit 2; } +[[ "$timeout" =~ ^[0-9]+$ ]] || { echo "wait-artifact: --timeout must be a positive integer" >&2; exit 2; }Makefile (1)
391-394:⚠️ Potential issue | 🟡 MinorBroaden
SHELL_SOURCESsolint-shincludes shell scripts without a.shsuffix
SHELL_SOURCES = $(shell git ls-files '*.sh')only lints files ending in.sh, so tracked hooks like.githooks/pre-commitand.githooks/pre-push(shell shebang, no.shextension) are currently missed bylint-sh. The “every tracked shell script” claim is therefore inaccurate.SHELL_SOURCES = $(shell git ls-files '*.sh') .PHONY: lint-sh lint-sh: $(SHELLCHECK) $(call run,shellcheck,$(SHELLCHECK) -x -P SCRIPTDIR $(SHELL_SOURCES),)Update
SHELL_SOURCESto select shell scripts by shebang (or include.githooks/*explicitly), e.g. usinggit grep -lfor^#!.*(ba)?sh/^#!.*sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3567a93a-bac2-4b7f-9970-c5f7c0717594
📒 Files selected for processing (15)
.githooks/pre-push.github/workflows/README.md.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefiledocs/src/content/docs/claude-code.mddocs/src/content/docs/development.mdscripts/ci-marker.shscripts/ci/check-pr-title.shscripts/ci/classify-changes.shscripts/ci/wait-artifact.shscripts/classify-paths.shscripts/classify-paths.test.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Docs PR titles in CHANGELOG.md must match code PR titles (Conventional Commits) when both exist; the CHANGELOG must accurately reflect the merged commits
Files:
CHANGELOG.md
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.{md,mdx}: Mermaid diagrams in docs must be authored vertically (top-down) to fit the content column width (~46–58rem); keep node labels short and use semantic node classes
Never place two large Mermaid diagrams side-by-side; wrap comparisons in<div class="diagram-pair">…</div>to stack them vertically
Docs prose indocs/src/content/docs/**/*.{md,mdx}(and governance docs like README.md, CONTRIBUTING.md, SECURITY.md, CODE_OF_CONDUCT.md, SUPPORT.md, clients/ts/README.md) must be reviewed for accuracy, runnable examples, clarity, and code↔docs sync before merge
Files:
docs/src/content/docs/claude-code.mddocs/src/content/docs/development.md
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
.github/workflows/**/*.yml: Third-party GitHub Actions must be pinned to full commit SHAs with version comments (never@mainor floating tags); prefer inline bash or official actions//github/ actions when feasible
CI logic must live inscripts/ci/*.sh(gated by make lint-sh), not inline YAML, except in trusted-main deploy jobs where inline is the trust boundary
Files:
.github/workflows/ci.yml
.github/workflows/ci.yml
📄 CodeRabbit inference engine (AGENTS.md)
Do not remove the
merge_group:trigger from CI workflows; the merge queue replaces the old require-up-to-date rule and dropping it causes queued PRs to hang
Files:
.github/workflows/ci.yml
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Validate locally before every push — run `make ci` the documented way
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Run `/prepush` to discover pre-push reviewers from `scripts/pre-push-reviewers.sh`, run needed reviewers in parallel (fresh context), skip any with nothing to do on the record, and loop until each reaches `ship_it`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Every code change must update its docs and `CHANGELOG.md` in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Address and resolve every review finding with substantive reply, fix it or track it in an issue, `@-mention` the bot, then resolve; never silently drop findings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Create PRs with `gh pr create --draft` (never `gh pr ready`/approve); PR title must pass Conventional-Commits gate (≤ 72 chars, check with `scripts/lint-pr-title.sh`)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Never force-push or rebase a PR branch — use `git merge origin/main` to absorb upstream main
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Never hand-write markers or use `--no-verify` — fix the gate instead if tempted
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: DRY — one source of truth. Before adding logic, look for existing helpers/types/constants to reuse; before duplicating rules, factor into one place
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Leave the codebase neater than you found it within reason — fix small, safe things in passing (stale comments, typos, misnamed locals, dead code)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Maintain 80% project-wide code coverage (CI enforces via `.testcoverage.yml`); per-suite minima: unit 80%, integration 20%, e2e 60%, sdk 50%
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Run `make lint` and `make test` before considering work complete in Go
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Add a new `area/<pkg>` repo label for each new `internal/<pkg>/` package to enable automatic issue triage routing
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: When adding a new API endpoint, update `docs/src/content/docs/api.md` and `README.md` (if user-facing) in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: When changing architecture or adding a package, update `docs/src/content/docs/architecture.md` and `AGENTS.md` in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: When changing ingest or event format, update `docs/src/content/docs/api.md` and `docs/src/content/docs/deployment.md` (CH schema) in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: When changing deployment or Docker, update `docs/src/content/docs/deployment.md` and compose files in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: When changing build or test process, update `docs/src/content/docs/development.md` and `Makefile` in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Every notable code change must be documented in `CHANGELOG.md` under `[Unreleased]` in the same PR
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Config struct tags in `internal/config/config.go` must stay in sync with `docs/src/content/docs/configuration.md`, `config.yaml`, and compose env blocks
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: `EventMessage` JSON tags must stay in sync with `docs/src/content/docs/api.md` event format, SSE examples, and ClickHouse INSERT columns
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Route registrations in `internal/api/router.go` must stay in sync with `docs/src/content/docs/api.md` endpoint list
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Handler error responses must stay in sync with `docs/src/content/docs/api.md` error tables
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Use Conventional Commits format for PR titles: `<type>(optional-scope)(optional-!): <subject>` with types from `{feat,fix,docs,refactor,test,chore,ci,deps,build,perf,revert,style}`; ≤ 72 chars, lowercase subject, no trailing period
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: TypeScript SDK (wavehouse/sdk) is the canonical client; sync it when backend public API changes (new endpoints, auth changes, event format changes, AST changes, query aggregation changes, pipes changes, policy changes, schema changes)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Do not use Dependabot auto-merge; every dependency update requires human admin review via the `required_reviewers` rule
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-06-10T19:24:48.560Z
Learning: Preserve named invariants when touching their packages; refer to them via the stable invariant index in comments (numbers are stable, cross-referenced from code comments and architecture.md)
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.mdCONTRIBUTING.mdAGENTS.mddocs/src/content/docs/claude-code.mddocs/src/content/docs/development.md
📚 Learning: 2026-06-10T15:01:59.729Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: .github/workflows/ci.yml:232-237
Timestamp: 2026-06-10T15:01:59.729Z
Learning: In this repo’s CI workflow (ci.yml), treat `clickhouse/clickhouse-server:latest` in the workflow’s prefetch steps (`docker pull -q clickhouse/clickhouse-server:latest`) as an intentional canary: the `:latest` tag is meant to mirror the tag that testcontainers resolves at runtime. Do not flag it as a supply-chain concern during CI workflow reviews as long as it’s used specifically for this “latest mirrors testcontainers runtime” prefetch purpose. If the workflow pins a different tag/digest for a different reason, or uses `latest` outside of this prefetch/canary pattern, then it may warrant scrutiny.
Applied to files:
.github/workflows/ci.yml
🪛 LanguageTool
AGENTS.md
[uncategorized] ~426-~426: The official name of this software platform is spelled with a capital “H”.
Context: ...assing); caches are owned end-to-end by .github/actions/setup-env (nested `actions/cac...
(GITHUB)
.github/workflows/README.md
[style] ~212-~212: 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: ... add cache save steps (invariant 6). 4. Need a build product / data from another job...
(EN_REPEATEDWORDS_NEED)
[style] ~215-~215: Since ownership is already implied, this phrasing may be redundant.
Context: ...he producer), or — when this job has its own setup to overlap and sits on the critic...
(PRP_OWN)
docs/src/content/docs/development.md
[uncategorized] ~576-~576: The official name of this software platform is spelled with a capital “H”.
Context: ... merge: - CI — the aggregator job of .github/workflows/ci.yml. The workflow is a jo...
(GITHUB)
[uncategorized] ~576-~576: 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)
[style] ~580-~580: Consider using a more formal/concise alternative here.
Context: ...oval of the most recent push by someone other than its author, resolution of all review th...
(OTHER_THAN)
[style] ~580-~580: Since ownership is already implied, this phrasing may be redundant.
Context: ... bypass these requirements when merging their own PR (e.g. a trivial .github change) bu...
(PRP_OWN)
[uncategorized] ~580-~580: The official name of this software platform is spelled with a capital “H”.
Context: ...en merging their own PR (e.g. a trivial .github change) but still cannot push directly...
(GITHUB)
🔇 Additional comments (22)
scripts/ci/wait-artifact.sh (3)
34-66: LGTM!
45-62: LGTM!
67-72: LGTM!.github/workflows/README.md (1)
1-257: LGTM!AGENTS.md (1)
1-439: LGTM!CONTRIBUTING.md (1)
40-40: LGTM!Also applies to: 79-79
docs/src/content/docs/claude-code.md (1)
26-26: LGTM!Also applies to: 40-40, 44-44, 149-160, 201-207, 213-213
docs/src/content/docs/development.md (5)
288-315: LGTM!
350-353: LGTM!
550-552: LGTM!
559-571: LGTM!
573-583: LGTM!Makefile (5)
181-187: LGTM!
399-401: LGTM!
403-409: LGTM!
486-486: LGTM!Also applies to: 882-882
927-935: LGTM!scripts/ci/classify-changes.sh (1)
1-61: LGTM!scripts/ci-marker.sh (1)
51-56: LGTM!.githooks/pre-push (1)
1-75: LGTM!.github/workflows/ci.yml (1)
1-643: LGTM!scripts/ci/check-pr-title.sh (1)
26-28: Resolve Dependabot author-format concern: In this repo, Dependabot PRs haveauthor.loginset to"app/dependabot"(and the"author:dependabot"search returns none), so the exemption"$author" == "app/dependabot"will match and skip the length check as intended.
CodeRabbit caught that the prose/meta allowlist in scripts/classify-paths.sh matched `.github/pull_request_template` (lowercase) while the repo's file is `.github/PULL_REQUEST_TEMPLATE.md` (uppercase) — inconsistent with the sibling `.github/ISSUE_TEMPLATE/` entry, which is already uppercase. In practice the `.*\.md$` entry masked it (the template is `.md`), but the pattern was dead as written. Match the real casing. Also cover the GitHub meta paths in scripts/classify-paths.test.sh — `labeler.yml` and `ISSUE_TEMPLATE/config.yml` are not `.md`, so they exercise their own allowlist entries rather than `.md$`, and would have caught this class of casing bug. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
## Summary Post-merge cleanup for #312 — removes the transition scaffolding it carried, now that it's landed on `main`. - **`ci.yml`** — the `PR title` and docs-preview-comment steps no longer guard on `scripts/ci/{check-pr-title,docs-preview-comment}.sh` existing (the `if [ ! -f … ]; then … exit 0; fi` skip-with-notice). Both scripts are on `main` now, so the steps call them directly. Confirmed present on `origin/main` (the trusted-main checkout these steps use). - **`.github/actions/setup-env/action.yml`** — drops the two unversioned v1 `gobuild-` restore-key fallbacks, keeping the `gobuild-v2-*` ones. Verified `gobuild-v2-*` caches exist for every suffix (`-lint`/`-unit`/`-integration`/`-e2e-cov`/`-cov`) on `main`, so no cold-cache regression. - **`.github/workflows/README.md`** — removes the "Transition notes" section and the cache-table v1 TODO. ## The ruleset flip (done out-of-band) The fourth transition item — the **merge-queue ruleset flip** — was applied directly via the API (it's a branch-protection change, not a code change). The `main branch protection` ruleset now has: - the `merge_queue` rule (SQUASH, ALLGREEN grouping, `min_entries_to_merge: 1`, up to 5 speculative builds), and - `strict_required_status_checks_policy: false` (drops the "require branches up to date" requirement — the merge queue's integration re-test replaces it). So **this PR will itself land through the merge queue** ("Merge when ready"). ## Test plan - [x] `actionlint` + `markdownlint` + `shellcheck` clean (`make lint-gha lint-md lint-sh`) - [x] `make ci` green locally - [x] de-guarded scripts confirmed present + executable on `origin/main` (`git cat-file -e origin/main:scripts/ci/check-pr-title.sh`) - [x] `gobuild-v2-*` caches confirmed present for every suffix on `main` (no cold-cache regression from dropping v1) - [x] no dangling references to the removed guards / Transition-notes section anywhere in the tree 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Splits the monolithic CI job (one runner serially executing
make ci) into a job DAG over the same Makefile targets, so localmake cistays the dev mirror while CI spreads across free 4-core public-repo runners:lint(make verify) ·unit(make test-unit test-ts) ·integration(make test-integration) ·e2e(make -j test-e2e— builds its own SDK dist + cover binary on a warm cache, then runs the suite exactly like a local run). Each suite runs withCOV_DEFER=1and uploads acoverage-<suite>fragment.coverage(needs: [unit, integration, e2e]) downloads the three fragments, runsmake cov(merge + every threshold gate), and posts the coverage summary — exactly like localmake ci's final step. A dedicated job (not folded into e2e's tail) so the gate is decoupled from the e2e suite's pass/fail and the DAG barrier waits on whichever suite finishes last (integration occasionally outlasts e2e), with no in-run artifact polling.docs-build(make build-docs) →docs-distartifact, only when a docs-affecting file changed.changesjob (inlinegh api, no third-party paths-filter) classifies the diff fail-closed: docs/prose-only PRs skip the Go/SDK suites; pushes, dispatches, merge-group runs, and API hiccups run everything.CIis the ruleset's sole required check: fails on any failed/canceled job, counts skipped jobs as passing — so path/event-filtered jobs never orphan the gate, and future job changes never require ruleset edits. Both the aggregator anddocs-deploykeepcoverageinneeds, so a coverage-gate failure blocks merge and prod deploys.PR titlejob under the aggregator (samescripts/lint-pr-title.sh, run from a trustedmaincheckout; reads the title from the API so re-runs see edits).housekeeping.ymldrops to non-required and keeps the fork-writable parts: labels, the sticky explainer comment, and a nudge that re-runs the failed title job when an edit fixes the title.docs-preview/docs-deploy(closes Harden docs-preview deploy: isolate CLOUDFLARE_API_TOKEN from the PR-built toolchain #305): deploys check out trustedmain— wrangler, worker source, and config never resolve from the PR tree — consume only the staticdocs-distartifact, and are the only jobs referencingCLOUDFLARE_*. Previews publish right afterdocs-build; production (docs-deploy) gates on the full pipeline includingcoverage.contents: writeis gone; onlydocs-previewholds a write scope, for the sticky comment). Fork PRs run the full secretless pipeline.setup-envis parameterized per job and exposes cache keys as outputs (saves can't drift from restores); the Go build cache is partitioned per job (-lint/-unit/-integration/-e2e-cov/-cov) with cross-suffix restore fallbacks, so each suite stays warm.make verifygainslint-sh(shellcheckv0.11.0, checksum-verified install viascripts/install-shellcheck.sh) andlint-gha(actionlintv1.7.12); workflow logic lives inscripts/ci/*.sh(classify-changes.sh,docs-preview-comment.sh,timing-summary.sh), not inline YAML — except in the trusted-main deploy jobs where inline is the trust boundary. A non-gatingTiming summaryjob writes a per-job wall-clock table to every run's Summary page.The full architecture — DAG diagram, design invariants, cache policy, add-a-job recipe, and the measured-but-deferred optimizations (e2e sharding among them) — lives in
.github/workflows/README.md; read it before editingci.yml.Merge queue: the workflow also runs on
merge_group, so approved PRs can land through a merge queue that re-tests each PR against currentmainat landing time (replacing the old "require branches up to date" rule). The trigger is inert until the ruleset flip, which happens post-merge (see README "Transition notes").Transition sequencing: the
main branch protectionruleset (15353356)required_status_checksis already[{"context":"CI"}]only (everything else untouched; verified via GET). Until this PR merges, title enforcement is advisory —PR housekeepingreds a bad title but can't block; once merged, thePR titlejob under the requiredCIaggregator makes it blocking again. Open PRs already produce a check namedCI, so nothing strands.Test plan
make lint-gha(actionlint) +make lint-sh(shellcheck) +make lint-md+make lint-proseclean on the reshaped workflow and all scriptsmerge_group/push run the full suite;title/docs-preview/docs-deployare event-filtered and the aggregator counts their skips as passescoveragejob (push → all green); confirmDocs deployskips withCIgreen on PRs and the preview URL postsdocs-deployto prod; a docs-only PR skips the Go suites withCIgreenRelated Issues
Closes #305