feat(ci): group e2e tests into functional suites with opt-in extended PR runs - #1197
Draft
jasonmadigan wants to merge 1 commit into
Draft
feat(ci): group e2e tests into functional suites with opt-in extended PR runs#1197jasonmadigan wants to merge 1 commit into
jasonmadigan wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThis PR replaces the two-tier Ginkgo focus/skip E2E test selection with a functional suite label system. New ChangesFunctional E2E selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
2 times, most recently
from
June 25, 2026 13:12
ae349e1 to
9a092f8
Compare
jasonmadigan
marked this pull request as ready for review
June 25, 2026 16:02
![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
🧹 Nitpick comments (1)
.github/workflows/e2e-on-demand.yaml (1)
49-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve comment-triggered suites through the shared router.
This step re-implements the aggregate-suite mapping that
build/suite-router.shalready owns. That duplication will drift the next time a suite or alias changes, and then/test-e2ecomments will stop matching PR routing behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-on-demand.yaml around lines 49 - 81, The suite-to-make-target mapping in the Resolve make target step is duplicated logic that should be routed through the shared suite router instead. Update the workflow to use build/suite-router.sh for resolving comment-triggered suites so aliases and suite names stay consistent with the existing routing logic, and keep the e2e-on-demand resolve step focused on consuming that shared output rather than reimplementing the mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yaml:
- Around line 278-299: The sticky comment lookup in the github-script block only
calls issues.listComments once, so it can miss the existing `<!-- e2e-suite-bot
-->` comment when there are more than 30 comments. Update the comment retrieval
logic to use `github.paginate` with `github.rest.issues.listComments`, then run
the existing `comments.find(...)` check against the full result set so
`issues.updateComment` is used instead of creating duplicates. Keep the rest of
the marker-based update/create flow unchanged.
In `@build/e2e.mk`:
- Around line 8-10: The PR-suite size description is stale and no longer matches
the updated curated gate. Update the inline documentation around E2E_SUITE_PR in
build/e2e.mk so the `pr` suite is described as ~31 specs, and make sure the
neighboring `pr-extended`/suite help text stays consistent with the new count.
Use the existing suite labels and comments in this section as the source of
truth so `make help` and the inline docs agree.
In `@build/suite-router.sh`:
- Around line 76-106: The pattern order in suite selection is too general in two
places, causing first-match Bash semantics to skip the intended specific cases.
In the main case block in suite-router.sh, move the TLS-specific
internal/broker/upstream/tls* rule ahead of the broader
internal/broker/upstream/* rule so TLS files map correctly; in the tests/e2e/*
nested case, place *url_elicitation* before *elicitation* so URL-elicitation
tests are not captured by the generic match. Keep the rest of the suggestions
logic unchanged.
In `@docs/ci.md`:
- Around line 79-88: Update the “How the Suite Router Works” section to point
readers to the shared routing logic in suite-router.sh instead of the workflow
YAML, and make clear that the PR workflow invokes that script. Use the existing
suite-router flow description and the known symbols like E2E_KNOWN_SUITES and
the PR workflow entrypoint to keep the docs aligned with the actual
implementation.
---
Nitpick comments:
In @.github/workflows/e2e-on-demand.yaml:
- Around line 49-81: The suite-to-make-target mapping in the Resolve make target
step is duplicated logic that should be routed through the shared suite router
instead. Update the workflow to use build/suite-router.sh for resolving
comment-triggered suites so aliases and suite names stay consistent with the
existing routing logic, and keep the e2e-on-demand resolve step focused on
consuming that shared output rather than reimplementing the mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 407ae73c-23da-45a6-aa34-9700fa889266
📒 Files selected for processing (20)
.claude/rules/e2e-tests.md.github/workflows/e2e-auth.yaml.github/workflows/e2e-nightly.yaml.github/workflows/e2e-on-demand.yaml.github/workflows/e2e.yamlbuild/e2e.mkbuild/known-suites.txtbuild/suite-router.shdocs/ci.mdtests/e2e/CLAUDE.mdtests/e2e/auth_policy_test.gotests/e2e/custom_tls_test.gotests/e2e/elicitation_test.gotests/e2e/happy_path_test.gotests/e2e/multi_gateway_test.gotests/e2e/oauth_protected_resource_test.gotests/e2e/tool_discovery_test.gotests/e2e/tool_validation_test.gotests/e2e/url_elicitation_test.gotests/e2e/user_specific_list_test.go
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-auth.yaml
- .github/workflows/e2e-nightly.yaml
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
2 times, most recently
from
June 25, 2026 17:05
9c17867 to
dc310af
Compare
coderabbitai
Bot
added
the
review-effort/large
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yaml:
- Around line 242-246: The Read known suites step in the e2e workflow is joining
only spaces, so the newline-separated output from known-suites.txt can still
leak into suites and break the $GITHUB_OUTPUT entry. Update the shell logic in
the known suite processing block to normalize newline-separated values before
wrapping them in backticks and comma-joining, using the existing Read known
suites step and the known id as the place to fix it.
In `@docs/ci.md`:
- Around line 54-58: The fenced command block in the CI docs is missing a
language hint, triggering MD040. Update the markdown fence around the /test-e2e
examples to use an explicit text/plain code language, keeping the existing
command content unchanged. Locate the block in the docs CI section that shows
/test-e2e discovery, /test-e2e pr-extended, and /test-e2e full.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7db8025b-4d7c-46d8-b799-22cab2df5d83
📒 Files selected for processing (20)
.claude/rules/e2e-tests.md.github/workflows/e2e-auth.yaml.github/workflows/e2e-nightly.yaml.github/workflows/e2e-on-demand.yaml.github/workflows/e2e.yamlbuild/e2e.mkbuild/known-suites.txtbuild/suite-router.shdocs/ci.mdtests/e2e/CLAUDE.mdtests/e2e/auth_policy_test.gotests/e2e/custom_tls_test.gotests/e2e/elicitation_test.gotests/e2e/happy_path_test.gotests/e2e/multi_gateway_test.gotests/e2e/oauth_protected_resource_test.gotests/e2e/tool_discovery_test.gotests/e2e/tool_validation_test.gotests/e2e/url_elicitation_test.gotests/e2e/user_specific_list_test.go
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-auth.yaml
- .github/workflows/e2e-nightly.yaml
✅ Files skipped from review due to trivial changes (12)
- tests/e2e/elicitation_test.go
- tests/e2e/tool_validation_test.go
- tests/e2e/oauth_protected_resource_test.go
- build/known-suites.txt
- tests/e2e/custom_tls_test.go
- .claude/rules/e2e-tests.md
- tests/e2e/url_elicitation_test.go
- tests/e2e/auth_policy_test.go
- tests/e2e/multi_gateway_test.go
- tests/e2e/user_specific_list_test.go
- tests/e2e/tool_discovery_test.go
- tests/e2e/happy_path_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- build/e2e.mk
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
from
June 26, 2026 09:13
dc310af to
af0075e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-on-demand.yaml:
- Around line 43-47: The shared-suite checkout in the e2e-on-demand workflow is
reading build/known-suites.txt from the default branch instead of the PR ref, so
suite validation can be out of sync with the later e2e-tests checkout. Update
the checkout step in the workflow to resolve and use the pull request head ref
before validating against known-suites.txt, and apply the same ref handling
anywhere else in the workflow that reads the suite catalog.
In @.github/workflows/e2e.yaml:
- Around line 221-225: The failure log collection step only runs the first debug
target because the shell exits on a non-zero return from ci-debug-logs, so
update the run block in the “Collect logs on failure” step to allow both make
ci-debug-logs and make ci-debug-test-servers-logs to execute even if one fails.
Use the existing step name and the two make targets as the key symbols when
adjusting the failure handling.
In `@build/suite-router.sh`:
- Line 20: The suite validation in suite-router.sh is using a word-match style
check against KNOWN_SUITES, which incorrectly accepts partial matches inside
hyphenated names. Update the membership logic in the suite lookup/validation
paths (including the checks around known-suites.txt, the suite dispatch, and the
label parsing branches) to require exact suite-name equality against the full
list of allowed suites. Use the existing suite-loading and validation symbols in
suite-router.sh so invalid inputs like auth, gateway, or specific are rejected
unless they exactly match a known suite name.
- Around line 76-93: The routing case in suite-router.sh is too broad in the
`case` statement and matches `internal/router/*` before the more specific router
branches. Reorder the `case` patterns so the specific
`internal/router/session*`, `internal/router/auth*`, and
`internal/router/elicit*` checks are evaluated before the generic
`internal/router/*|internal/ext_proc/*` catch-all, preserving the intended
`suggestions` handling in the `suggestions+=` blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3110980d-b979-4dfb-8472-19f4ff62b619
📒 Files selected for processing (20)
.claude/rules/e2e-tests.md.github/workflows/e2e-auth.yaml.github/workflows/e2e-nightly.yaml.github/workflows/e2e-on-demand.yaml.github/workflows/e2e.yamlbuild/e2e.mkbuild/known-suites.txtbuild/suite-router.shdocs/ci.mdtests/e2e/CLAUDE.mdtests/e2e/auth_policy_test.gotests/e2e/custom_tls_test.gotests/e2e/elicitation_test.gotests/e2e/happy_path_test.gotests/e2e/multi_gateway_test.gotests/e2e/oauth_protected_resource_test.gotests/e2e/tool_discovery_test.gotests/e2e/tool_validation_test.gotests/e2e/url_elicitation_test.gotests/e2e/user_specific_list_test.go
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-nightly.yaml
- .github/workflows/e2e-auth.yaml
✅ Files skipped from review due to trivial changes (11)
- tests/e2e/oauth_protected_resource_test.go
- build/known-suites.txt
- tests/e2e/tool_validation_test.go
- tests/e2e/custom_tls_test.go
- tests/e2e/url_elicitation_test.go
- tests/e2e/auth_policy_test.go
- tests/e2e/user_specific_list_test.go
- tests/e2e/multi_gateway_test.go
- tests/e2e/tool_discovery_test.go
- .claude/rules/e2e-tests.md
- tests/e2e/elicitation_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/happy_path_test.go
- build/e2e.mk
- docs/ci.md
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
from
June 26, 2026 13:23
af0075e to
e315cb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yaml:
- Around line 70-76: Add a manual workflow input named suite under
on.workflow_dispatch so users can choose a suite when dispatching the workflow,
and make sure the Route suites step exports that value into DISPATCH_SUITE.
Update the e2e workflow around workflow_dispatch and the router step that runs
build/suite-router.sh so the suite selection is forwarded instead of always
falling back to the default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61eebca3-e8e9-4a98-a3d6-11eb13bdf161
📒 Files selected for processing (20)
.claude/rules/e2e-tests.md.github/workflows/e2e-auth.yaml.github/workflows/e2e-nightly.yaml.github/workflows/e2e-on-demand.yaml.github/workflows/e2e.yamlbuild/e2e.mkbuild/known-suites.txtbuild/suite-router.shdocs/ci.mdtests/e2e/CLAUDE.mdtests/e2e/auth_policy_test.gotests/e2e/custom_tls_test.gotests/e2e/elicitation_test.gotests/e2e/happy_path_test.gotests/e2e/multi_gateway_test.gotests/e2e/oauth_protected_resource_test.gotests/e2e/tool_discovery_test.gotests/e2e/tool_validation_test.gotests/e2e/url_elicitation_test.gotests/e2e/user_specific_list_test.go
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-auth.yaml
- .github/workflows/e2e-nightly.yaml
✅ Files skipped from review due to trivial changes (11)
- tests/e2e/oauth_protected_resource_test.go
- build/known-suites.txt
- tests/e2e/tool_validation_test.go
- tests/e2e/user_specific_list_test.go
- tests/e2e/custom_tls_test.go
- tests/e2e/auth_policy_test.go
- tests/e2e/elicitation_test.go
- tests/e2e/multi_gateway_test.go
- tests/e2e/tool_discovery_test.go
- docs/ci.md
- tests/e2e/happy_path_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- .claude/rules/e2e-tests.md
- tests/e2e/url_elicitation_test.go
- build/e2e.mk
Member
Author
|
Rebasing after #1225 |
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
from
July 3, 2026 13:02
e315cb0 to
e1b4360
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
build/suite-router.sh (3)
80-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
internal/router/*catch-all still shadows the more specific router branches.Line 81 (
internal/router/*|internal/ext_proc/*) matches before the laterinternal/router/session*(Line 90),internal/router/auth*(Line 93), andinternal/router/elicit*(Line 96) patterns, so those specific suggestions (security+sessions,auth-policy,url-elicitation/elicitation) never fire for router-path changes — confirmed by the shellcheck SC2221/SC2222 hints on Lines 81/90.🔧 Proposed fix
case "$f" in - internal/router/*|internal/ext_proc/*) - suggestions+=(routing sessions security) - ;; internal/broker/upstream/tls*) suggestions+=(tls) ;; internal/broker/upstream/*|internal/broker/broker.go) suggestions+=(core discovery prompts) ;; internal/broker/session*|internal/router/session*|internal/broker/jwt*) suggestions+=(sessions security) ;; internal/broker/auth*|internal/router/auth*) suggestions+=(auth-policy trusted-headers) ;; internal/broker/elicit*|internal/router/elicit*) suggestions+=(url-elicitation elicitation) ;; + internal/router/*|internal/ext_proc/*) + suggestions+=(routing sessions security) + ;; internal/controller/*|api/*)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/suite-router.sh` around lines 80 - 104, The pattern order in suite-router.sh causes the generic internal/router/* branch in the case statement to match before the more specific internal/router/session*, internal/router/auth*, and internal/router/elicit* branches. Reorder the case arms in the routing suggestions logic so the specific router patterns are checked first and the broader internal/router/*|internal/ext_proc/* fallback comes later, ensuring the intended sessions/security, auth-policy, and url-elicitation/elicitation suggestions are emitted.
19-24: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
is_known_suitelikely never matches, silently no-oping the entire named-suite system.
grep -Fxq "$1" "$KNOWN_SUITES_FILE"requires$1to equal an entire line of the file. Ifknown-suites.txtis a single space-separated line (as flagged in prior review discussion), no individual suite name will ever match a full line — everye2e/<suite>label and every named suite request silently drops, andbuild_targetfalls back totest-e2e-pr. This defeats the PR's core objective of opt-in named-suite execution.🔧 Proposed fix (if file is a single space-separated line)
is_known_suite() { - grep -Fxq "$1" "$KNOWN_SUITES_FILE" + grep -qFx "$1" <(tr ' ' '\n' < "$KNOWN_SUITES_FILE") }Please confirm the actual file layout:
#!/bin/bash fd known-suites.txt -x cat -A {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/suite-router.sh` around lines 19 - 24, The is_known_suite check is treating known-suites.txt as line-based input, so named suites may never match if the file is stored as a single space-separated list. Update is_known_suite to match the actual known-suites.txt layout by splitting on whitespace (or by normalizing the file to one suite per line), and verify build_target uses the same format when checking suite names so named-suite execution does not fall back to test-e2e-pr.
26-27: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
INFRA_SUITESis declared but never used, and no infra-setup wiring exists for those suites.Neither this script nor
e2e.yaml/e2e-on-demand.yamlinvoke any auth/cert-manager/multi-gateway infra setup (both only runmake ci-setup). Onceis_known_suiteis fixed, labels likee2e/auth-policyore2e/tlswill route tomake test-e2e-suite SUITE=auth-policyagainst a cluster missing the required infra (previously provisioned by the now-deletede2e-auth.yaml), causing those runs to fail.Either wire up
ci-auth-setup/ci-cert-manager-setupwhen an infra suite is requested, or explicitly gate infra suites out of label/comment triggers and document them as maintainer-only until infra setup is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/suite-router.sh` around lines 26 - 27, The suite routing logic currently exposes INFRA_SUITES in suite-router.sh, but nothing in suite-router.sh, e2e.yaml, or e2e-on-demand.yaml actually provisions the required auth/cert-manager/multi-gateway infrastructure. Update the dispatch path around is_known_suite and the suite-trigger handling so infra suites either invoke the correct setup targets (such as ci-auth-setup and ci-cert-manager-setup) before make test-e2e-suite, or are explicitly excluded from label/comment-triggered runs until that wiring exists. Use INFRA_SUITES and the e2e suite entrypoints as the key places to adjust..github/workflows/e2e-on-demand.yaml (2)
43-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill validating suites against the default branch, not the PR ref.
This sparse checkout has no
ref, so it pullsbuild/known-suites.txtfrom the default branch. A suite added/renamed only in this PR will fail validation here even thoughe2e-testslater checks out the PR head at Line 163. This was previously flagged and reportedly addressed, but the current code still lacks a ref — likely reintroduced by the rebase.🔧 Proposed fix
+ - name: Get PR ref + id: pr-ref + uses: actions/github-script@v7 + with: + script: | + const pr = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.issue.number + }); + core.setOutput('ref', pr.data.head.ref); + - name: Checkout (for shared suite list) uses: actions/checkout@v4 with: + ref: ${{ steps.pr-ref.outputs.ref }} sparse-checkout: build/known-suites.txt sparse-checkout-cone-mode: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-on-demand.yaml around lines 43 - 47, The “Checkout (for shared suite list)” step is still reading build/known-suites.txt from the default branch instead of the PR ref. Update the actions/checkout@v4 configuration in that step to explicitly check out the current PR commit/HEAD, matching the ref used later by e2e-tests, so suite validation uses the same revision as the tests. Keep the fix localized to the checkout step that uses sparse-checkout and sparse-checkout-cone-mode.
108-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBacktick-wrap of
known-suites.txtlikely collapses into one span, not per-suite backticks.
sed 's/.*/&/'wraps each line in backticks. Ifknown-suites.txtis a single space-separated line (as strongly indicated by review discussion onsuite-router.sh), the rendered message becomes one big backtick span (`core routing sessions ...`) instead of individually-quoted suite names. Same root cause as the sed issue one2e.yamlLine 245.🔧 Proposed fix (if file is space-separated)
- echo "suites=$(sed 's/.*/`&`/' build/known-suites.txt | paste -sd, - | sed 's/,/, /g')" >> "$GITHUB_OUTPUT" + echo "suites=$(tr ' ' '\n' < build/known-suites.txt | sed 's/.*/`&`/' | paste -sd, - | sed 's/,/, /g')" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-on-demand.yaml around lines 108 - 112, The “Read known suites” step is wrapping the entire `known-suites.txt` line in one backtick span because `sed 's/.*/`&`/'` operates per line, not per suite. Update the `known` step in the e2e-on-demand workflow to split the space-separated suite list into individual suite names first, then backtick-wrap each suite before joining them for `GITHUB_OUTPUT`. Use the existing `known` step and `build/known-suites.txt` as the targets to adjust..github/workflows/e2e.yaml (3)
204-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
make test-e2e-pralways runs before the suite loop, doubling specs labeled bothprand a named suite.Any spec with
Label("sessions", "pr")runs once in the unconditionaltest-e2e-prpass and again in thesessionsloop iteration — doubles runtime and risks state collisions for ordered/serial suites.🔧 Proposed fix
if [ "$E2E_MAKE_TARGET" = "test-e2e-suite" ]; then - # run pr suite first, then each requested suite - make test-e2e-pr E2E_PROCS=4 + pr_requested=false for suite in $(echo "$E2E_SUITES" | tr -d '[]"' | tr ',' '\n'); do - [ "$suite" = "pr" ] && continue + [ "$suite" = "pr" ] && { pr_requested=true; continue; } [ -z "$suite" ] && continue echo "--- running suite: $suite ---" if [ "$suite" = "pr-extended" ]; then make test-e2e-pr-extended E2E_PROCS=4 else make test-e2e-suite SUITE="$suite" E2E_PROCS=4 fi done + [ "$pr_requested" = "true" ] || make test-e2e-pr E2E_PROCS=4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml around lines 204 - 216, The e2e workflow currently runs make test-e2e-pr unconditionally before iterating E2E_SUITES, which causes specs tagged for both pr and a named suite to execute twice. Update the shell logic in the e2e workflow step so the initial pr run only happens when needed, or skip the pr suite in the loop in a way that avoids re-running the same specs; use the existing make test-e2e-pr, make test-e2e-pr-extended, and make test-e2e-suite branches as the places to adjust.
242-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame single-line backtick-wrap issue as
e2e-on-demand.yaml.
sed 's/.*/&/'on a single space-separated line produces one giant backtick span instead of per-suite backticks in the sticky bot comment.🔧 Proposed fix (if file is space-separated)
- list=$(sed 's/.*/`&`/' build/known-suites.txt | paste -sd, - | sed 's/,/, /g') + list=$(tr ' ' '\n' < build/known-suites.txt | sed 's/.*/`&`/' | paste -sd, - | sed 's/,/, /g')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml around lines 242 - 246, The Read known suites step in the workflow is wrapping the entire space-separated suite list in one backtick span instead of quoting each suite individually, which breaks the sticky bot comment formatting. Update the shell pipeline in the known step to split the contents of build/known-suites.txt into separate suite tokens before adding backticks, then join them with commas so each suite name is rendered as its own inline code fragment. Use the known step and the suite list generation logic as the place to fix this.
22-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
workflow_dispatchstill has nosuiteinput, soDISPATCH_SUITEis always empty.
suite-router.sh'sworkflow_dispatchbranch callsparse_suite_input "${DISPATCH_SUITE:-}", but nothing here defines or passesDISPATCH_SUITE. Manual dispatch always resolves to the defaultprsuite regardless of intent — same gap flagged previously, still unresolved in this diff.🔧 Proposed fix
workflow_dispatch: + inputs: + suite: + description: 'Suite to run (empty for default pr gate)' + required: false + type: string ... - name: Route suites id: router env: EVENT_TYPE: ${{ github.event_name }} PR_LABELS: ${{ steps.labels.outputs.labels || '' }} CHANGED_FILES: ${{ steps.changes.outputs.files || '' }} + DISPATCH_SUITE: ${{ github.event.inputs.suite || '' }} run: bash build/suite-router.shAlso applies to: 70-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml around lines 22 - 24, The manual dispatch flow is still missing a suite selector, so DISPATCH_SUITE is always empty and workflow_dispatch falls back to the default suite. Update the e2e workflow’s workflow_dispatch trigger to define a suite input and pass that value through to the job/environment so suite-router.sh receives a real DISPATCH_SUITE. Ensure the workflow_dispatch branch that calls parse_suite_input uses the provided input instead of an empty fallback, so manual runs can choose the intended suite.
🧹 Nitpick comments (1)
build/suite-router.sh (1)
1-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a small
bats/shell test suite for this router.Two of the bugs flagged above (
is_known_suiteexact-match, case-ordering shadow) are exactly the class of regression a handful of unit tests aroundis_known_suite,parse_labels, andinfer_suggestionswould have caught immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/suite-router.sh` around lines 1 - 211, Add a small shell/bats test suite for the suite router to cover the regression-prone helpers in suite-router.sh. Focus tests on is_known_suite exact matching, parse_labels behavior for e2e/* labels versus exact suite names, and infer_suggestions output for representative changed-file paths so future case-ordering or matching bugs are caught early. Use the existing function names is_known_suite, parse_labels, and infer_suggestions as the entry points for the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-on-demand.yaml:
- Around line 34-81: The workflow currently duplicates suite resolution instead
of using the shared router logic. In the e2e-on-demand job, replace the inline
Parse command/Resolve make target case handling with a call to
build/suite-router.sh, passing the parsed SUITE as COMMENT_SUITE and consuming
the router’s make_target and suite_name outputs. Keep the Parse command step
only for extracting the comment suite, and wire the job to use the router’s
existing issue_comment handling rather than maintaining a second resolver.
---
Duplicate comments:
In @.github/workflows/e2e-on-demand.yaml:
- Around line 43-47: The “Checkout (for shared suite list)” step is still
reading build/known-suites.txt from the default branch instead of the PR ref.
Update the actions/checkout@v4 configuration in that step to explicitly check
out the current PR commit/HEAD, matching the ref used later by e2e-tests, so
suite validation uses the same revision as the tests. Keep the fix localized to
the checkout step that uses sparse-checkout and sparse-checkout-cone-mode.
- Around line 108-112: The “Read known suites” step is wrapping the entire
`known-suites.txt` line in one backtick span because `sed 's/.*/`&`/'` operates
per line, not per suite. Update the `known` step in the e2e-on-demand workflow
to split the space-separated suite list into individual suite names first, then
backtick-wrap each suite before joining them for `GITHUB_OUTPUT`. Use the
existing `known` step and `build/known-suites.txt` as the targets to adjust.
In @.github/workflows/e2e.yaml:
- Around line 204-216: The e2e workflow currently runs make test-e2e-pr
unconditionally before iterating E2E_SUITES, which causes specs tagged for both
pr and a named suite to execute twice. Update the shell logic in the e2e
workflow step so the initial pr run only happens when needed, or skip the pr
suite in the loop in a way that avoids re-running the same specs; use the
existing make test-e2e-pr, make test-e2e-pr-extended, and make test-e2e-suite
branches as the places to adjust.
- Around line 242-246: The Read known suites step in the workflow is wrapping
the entire space-separated suite list in one backtick span instead of quoting
each suite individually, which breaks the sticky bot comment formatting. Update
the shell pipeline in the known step to split the contents of
build/known-suites.txt into separate suite tokens before adding backticks, then
join them with commas so each suite name is rendered as its own inline code
fragment. Use the known step and the suite list generation logic as the place to
fix this.
- Around line 22-24: The manual dispatch flow is still missing a suite selector,
so DISPATCH_SUITE is always empty and workflow_dispatch falls back to the
default suite. Update the e2e workflow’s workflow_dispatch trigger to define a
suite input and pass that value through to the job/environment so
suite-router.sh receives a real DISPATCH_SUITE. Ensure the workflow_dispatch
branch that calls parse_suite_input uses the provided input instead of an empty
fallback, so manual runs can choose the intended suite.
In `@build/suite-router.sh`:
- Around line 80-104: The pattern order in suite-router.sh causes the generic
internal/router/* branch in the case statement to match before the more specific
internal/router/session*, internal/router/auth*, and internal/router/elicit*
branches. Reorder the case arms in the routing suggestions logic so the specific
router patterns are checked first and the broader
internal/router/*|internal/ext_proc/* fallback comes later, ensuring the
intended sessions/security, auth-policy, and url-elicitation/elicitation
suggestions are emitted.
- Around line 19-24: The is_known_suite check is treating known-suites.txt as
line-based input, so named suites may never match if the file is stored as a
single space-separated list. Update is_known_suite to match the actual
known-suites.txt layout by splitting on whitespace (or by normalizing the file
to one suite per line), and verify build_target uses the same format when
checking suite names so named-suite execution does not fall back to test-e2e-pr.
- Around line 26-27: The suite routing logic currently exposes INFRA_SUITES in
suite-router.sh, but nothing in suite-router.sh, e2e.yaml, or e2e-on-demand.yaml
actually provisions the required auth/cert-manager/multi-gateway infrastructure.
Update the dispatch path around is_known_suite and the suite-trigger handling so
infra suites either invoke the correct setup targets (such as ci-auth-setup and
ci-cert-manager-setup) before make test-e2e-suite, or are explicitly excluded
from label/comment-triggered runs until that wiring exists. Use INFRA_SUITES and
the e2e suite entrypoints as the key places to adjust.
---
Nitpick comments:
In `@build/suite-router.sh`:
- Around line 1-211: Add a small shell/bats test suite for the suite router to
cover the regression-prone helpers in suite-router.sh. Focus tests on
is_known_suite exact matching, parse_labels behavior for e2e/* labels versus
exact suite names, and infer_suggestions output for representative changed-file
paths so future case-ordering or matching bugs are caught early. Use the
existing function names is_known_suite, parse_labels, and infer_suggestions as
the entry points for the tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7574a68f-8a22-44c7-8c81-2d31058e6819
📒 Files selected for processing (19)
.claude/rules/e2e-tests.md.github/workflows/e2e-auth.yaml.github/workflows/e2e-nightly.yaml.github/workflows/e2e-on-demand.yaml.github/workflows/e2e.yamlbuild/e2e.mkbuild/known-suites.txtbuild/suite-router.shdocs/ci.mdtests/e2e/CLAUDE.mdtests/e2e/auth_policy_test.gotests/e2e/custom_tls_test.gotests/e2e/elicitation_test.gotests/e2e/happy_path_test.gotests/e2e/multi_gateway_test.gotests/e2e/oauth_protected_resource_test.gotests/e2e/tool_discovery_test.gotests/e2e/tool_validation_test.gotests/e2e/user_specific_list_test.go
💤 Files with no reviewable changes (2)
- .github/workflows/e2e-auth.yaml
- .github/workflows/e2e-nightly.yaml
✅ Files skipped from review due to trivial changes (10)
- .claude/rules/e2e-tests.md
- tests/e2e/oauth_protected_resource_test.go
- tests/e2e/user_specific_list_test.go
- tests/e2e/auth_policy_test.go
- tests/e2e/elicitation_test.go
- build/known-suites.txt
- docs/ci.md
- tests/e2e/multi_gateway_test.go
- tests/e2e/tool_discovery_test.go
- tests/e2e/happy_path_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/tool_validation_test.go
- tests/e2e/custom_tls_test.go
- build/e2e.mk
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
6 times, most recently
from
July 3, 2026 15:05
c6bb537 to
2010a3c
Compare
trepel
reviewed
Jul 8, 2026
| GinkgoWriter.Printf("all %d concurrent calls used backend session: %s\n", concurrency, firstSession) | ||
| }) | ||
|
|
||
| It("[Happy] concurrent tool calls across different backends should each reach the correct upstream", func() { |
Contributor
There was a problem hiding this comment.
@jasonmadigan why this test was removed? Is it because we run the tests in parallel now so explicit test like this one is no longer needed?
Contributor
|
Several tests that were previously marked Serial have lost that marker, was that intentional? These tests does some scaling, pod restarting etc so I would think that they need to be marked Serial. |
trepel
reviewed
Jul 8, 2026
| testResources = nil | ||
| }) | ||
|
|
||
| It("[Happy,URLElicitation] URL elicitation triggers on missing token for elicitation-capable client; server without tokenURLElicitation is unaffected", func() { |
Contributor
There was a problem hiding this comment.
Labels are missing for all tests in this file, I would expect smt along the lines of It("[Happy,URLElicitation] Full round-trip: token page submit then retry succeeds", Label("url-elicitation", "pr"), func() { here. Same for other tests in this file.
trepel
reviewed
Jul 8, 2026
| // nightly-only: discover_tools shares applyAuthorizedCapabilitiesFilter | ||
| // with tools/list, which the PR-gate JWT filtering spec exercises | ||
| It("[Full] respects auth filtering in discover_tools", Label("discovery"), func() { | ||
| SetupTrustedHeadersAuth(ctx, k8sClient) |
Contributor
There was a problem hiding this comment.
I think that SetupTrustedHeadersAuthInNamespace(ctx, k8sClient, toolDiscNamespace, toolDiscExtName) needs to be used here. If I got it right the separate namespace is used for tool discovery tests and SetupTrustedHeadersAuth does the setup in SystemNamespace.
Contributor
There was a problem hiding this comment.
If indeed so, you might need to fix the deferred cleanup since the deployment might be already gone here: https://github.com/jasonmadigan/mcp-gateway/blob/1195-e2e-functional-suites/tests/e2e/builders.go#L51-L52
trepel
reviewed
Jul 8, 2026
| @@ -579,7 +581,8 @@ var _ = Describe("Tool Discovery", Ordered, func() { | |||
| Expect(respBody).To(ContainSubstring(`"error"`), "discover_tools should return a JSON-RPC error when disabled") | |||
Contributor
There was a problem hiding this comment.
Not related to this PR but while reviewing it I noticed that I was not getting JSON_RPC error here, just standard "Tool not found"
jasonmadigan
marked this pull request as draft
July 8, 2026 20:44
Member
Author
|
@trepel I think this isn't quite ready for a review |
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
2 times, most recently
from
July 14, 2026 16:10
9557f0e to
f82569f
Compare
… PR runs specs carry Ginkgo suite labels across 13 named suites; the PR gate narrows to a curated pr label set with pr-extended and full tiers. suites run on demand via /test-e2e comments and e2e/<suite> labels through a shared suite router. e2e-auth and e2e-nightly workflows are deleted, with nightly consolidated into e2e.yaml's schedule trigger. workflows harden untrusted input handling: env var indirection for comment bodies, actor, tag names, inputs and step outputs, pinned SHA checkout for on-demand runs, and suite name validation. review fixes: tool discovery auth uses the namespaced setup with tolerant cleanup, the cross-backend concurrency spec is restored, Serial markers return to shared-infrastructure specs, and the duplicate url elicitation file is dropped in favour of labelling the isolated specs. Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
jasonmadigan
force-pushed
the
1195-e2e-functional-suites
branch
from
July 14, 2026 16:18
f82569f to
769a90b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Organises e2e tests into named functional suites. The required PR gate narrows to a curated ~30-spec happy-path set (
prlabel), with broader coverage (~61 specs) available aspr-extended. Named suites can be triggered on demand via/test-e2e <suite>comments ande2e/<suite>labels.What changed
Label()across 13 suitestest-e2e-pr,test-e2e-pr-extended,test-e2e-suite SUITE=Xinbuild/e2e.mkbuild/suite-router.shfor CI suite selection and changed-file inferencee2e-on-demand.yamlexpanded fromhappy|fullto all named suites; shell injection fix (comment body via env var, not inline interpolation)e2e.yamle2e-auth.yamlande2e-nightly.yaml, consolidated nightly schedule intoe2e.yamldocs/ci.mdwith full suite referenceLabels
Apply these labels to a PR to trigger the corresponding e2e suite on top of the default
prgate:e2e/coree2e/routinge2e/sessionse2e/discoverye2e/promptse2e/auth-policye2e/trusted-headerse2e/elicitatione2e/url-elicitatione2e/user-specific-liste2e/tlse2e/multi-gatewaye2e/securitye2e/pr-extendede2e/fullUsage
See
docs/ci.mdfor full reference.Closes #1195