Uh oh!
There was an error while loading. Please reload this page.
chore(review): land the open review-thread fixes from the model-kind batch - #969
Conversation
…batch - tests/e2e: both new suites adopt the standard readiness gate (caller key seeded last, authenticate GET /v1/models, no catch-all) instead of probing the behavior under test; stale probe/slot comments corrected. - loader.rs: un-fuse the doc comments of merge_partial_compat_fields and warn_partial_compat_deduped (the WARN-dedup paragraph sat on the wrong function). - CLAUDE.md: correct the AGENTS.md path (crates/aisix-proxy/), and state the DP shape mapping (embedding = block on the direct shape) plus the wildcard resolved-row identity rule explicitly. - Cargo.toml comments: attach the right GHSA ids to the 0.101 connector removal; narrow the jsonwebtoken aws_lc_rs note to the version-unification claim.
📝 WalkthroughWalkthroughThe change updates E2E resource setup and readiness checks. It also aligns repository guidance and comments with current model, dependency, crypto-backend, and warning-deduplication details. ChangesE2E readiness and documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score:🔵 Low · up to The updated E2E readiness gates still need explicit handling for HTTP and transport failures and confirmation that all seeded resources are included before readiness is reported; otherwise tests may show misleading timeouts or run against incomplete configuration. This is a bounded test-integration risk that is mergeable with explicit owner follow-up. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Aligns review-thread fixes with repository conventions without changing runtime behavior.
Changes:
- Reworks E2E readiness gates to use last-seeded API keys and
/v1/models. - Corrects loader and dependency comments.
- Expands model-kind guidance, though one identity rule remains inaccurate.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/e2e/src/cases/wildcard-identity-e2e.test.ts | Makes readiness independent of wildcard behavior. |
tests/e2e/src/cases/semantic-member-gates-e2e.test.ts | Uses the standard readiness gate and corrects timing text. |
crates/aisix-proxy/Cargo.toml | Clarifies crypto dependency unification. |
crates/aisix-etcd/src/loader.rs | Associates documentation with the correct function. |
CLAUDE.md | Updates model-kind review guidance and path references. |
Cargo.toml | Corrects legacy connector advisory references. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
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 `@tests/e2e/src/cases/semantic-member-gates-e2e.test.ts`:
- Around line 328-332: Update both readiness gates in
tests/e2e/src/cases/semantic-member-gates-e2e.test.ts:328-332 and
tests/e2e/src/cases/wildcard-identity-e2e.test.ts:123-127 to consume or cancel
every fetch response body. Retry only on HTTP 401; throw or otherwise surface
all other HTTP statuses, transport failures, and response-processing errors so
waitConfigPropagation does not mask them as a timeout.
🪄 Autofix
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: 9eeac5d2-ee12-4c6a-9300-4c400d26e721
📒 Files selected for processing (6)
CLAUDE.mdCargo.tomlcrates/aisix-etcd/src/loader.rscrates/aisix-proxy/Cargo.tomltests/e2e/src/cases/semantic-member-gates-e2e.test.tstests/e2e/src/cases/wildcard-identity-e2e.test.ts
Uh oh!
There was an error while loading. Please reload this page.
…family; release gate sockets - CLAUDE.md: the row-keying statement now names inline buckets, metric labels and health keys only — usage-event requested_model and model_name policy conditions intentionally keep the caller-supplied string. - e2e gates: consume the /v1/models response body between polls.
… billing/throttling)
Uh oh!
There was an error while loading. Please reload this page.
What
Lands the open review-thread items from the model-kind batch (#957/#958/#959/#960/#963/#964) that were valid against the merged code. No behavior change — e2e robustness, doc comments, and rule-text accuracy.
Changes
e2e readiness gates (
semantic-member-gates-e2e.test.ts,wildcard-identity-e2e.test.ts): both new suites now follow the suite convention (tests/e2e/AGENTS.md) — every resource is seeded first, the caller key last, and readiness isGET /v1/modelsreturning 200 with that key. The old gates probed the exact behavior under test (a semantic route fallback / a wildcard chat that also consumed the shared rpm bucket) and swallowed transport errors in a catch-all, so a dispatch defect would surface as abeforeAllpropagation timeout instead of a named test failure. Stale comments about the probe-consumed rpm slot and a "1s probe interval" (the config sets the 5s schema minimum) corrected. The gates consume the response body between polls to release the socket. Both suites pass locally against a freshly built binary (8/8, twice).loader.rsdoc un-fusing: the "WARN once per (kind, field-set)…" paragraph belonged towarn_partial_compat_dedupedbut sat fused abovemerge_partial_compat_fields, describing a dedup cache that function does not have. Each function now carries its own comment.CLAUDE.mdaccuracy (Model Kinds Stay in Lockstep): the per-target invariant reference now uses the real pathcrates/aisix-proxy/AGENTS.md; a new paragraph states the shape mapping (the five kinds are the cross-plane taxonomy;model_one_ofimplements four dispatch shapes withembeddingas a block on the direct shape) and pins the wildcard identity rule scoped to what #959 changed — inline rate-limit buckets, Prometheus metric labels, and health keys use the resolved row'sdisplay_name, while usage-eventrequested_modelandmodel_namepolicy conditions intentionally keep the caller-supplied string.Cargo.toml comment corrections: the 0.101-connector removal rationale now cites its actual advisories (GHSA-xgp8-3hg3-c2mh / GHSA-965h-392x-2mh5 — GHSA-82j2-j2ch-gfr8 is the separate rustls-webpki 0.103 lock bump), and the jsonwebtoken
aws_lc_rsnote claims version unification rather than "no second crypto stack" (the feature does activate aws-lc-rs's optionaluntrusteddependency).PR-batching rule (rider): a new
## PR BatchingCLAUDE.md section — one PR per session by default; this repo is agent-developed end-to-end and CodeRabbit bills/throttles per PR, so follow-up work rides the session's open PR instead of opening new ones (this commit is itself an instance).Tests
The two touched suites are themselves the coverage:
pnpm vitest runon both files against a rebuilt binary — 8/8 pass. Everything else is comments/docs, per the repo's test-exception note.🤖 Generated with Claude Code
Independent audit
Cold audit: all five angles CLEAN — the revision-order gate argument was verified against the harness seed path (sequential awaited puts) and the etcd supervisor's ordered single-stream apply; every factual claim in the changed comments was checked against the code and the GitHub advisory data. One LOW comment nit (the alignment-loop comment claimed a pre-consumed window that no longer exists) is applied.