Uh oh!
There was an error while loading. Please reload this page.
test(e2e): add SeedClient — seed resources by writing canonical documents to etcd - #750
Conversation
Warning Review limit reached
Next review available in:37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesSeed versus Admin characterization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SeedClient
participant AdminClient
participant etcd
participant AISIXApp
participant OpenAIClients
SeedClient->>etcd: Write resource documents
AdminClient->>AISIXApp: Create resources
AISIXApp->>etcd: Persist Admin resources
OpenAIClients->>AISIXApp: Send chat requests
AISIXApp-->>OpenAIClients: Return completion or 403
AdminClient->>AISIXApp: GET stored resources
AISIXApp->>etcd: Read resource documents
AISIXApp-->>AdminClient: Return resource entries
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.ts (1)
192-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated fetch-find-normalize-compare pattern.
The four comparison blocks follow an identical pattern. A helper would reduce duplication and make the test's intent clearer.
♻️ Optional helper extraction
type Entry = { id: string; value: Record<string, unknown> }; +async function assertRoundTripEqual(+ admin: AdminClient,+ path: string,+ seedEntry: Entry,+ adminEntry: Entry,+ varied: string[],+ label: string,+) {+ const entries = await admin.json<Entry[]>("GET", path);+ const seedVal = normalize(find(entries, seedEntry.id, `seed ${label}`).value, varied);+ const adminVal = normalize(find(entries, adminEntry.id, `admin ${label}`).value, varied);+ expect(seedVal).toEqual(adminVal);+}+ describe("seed-vs-admin characterization: direct etcd writes ≡ Admin API writes", () => {Then the test body becomes:
- const pks = await admin.json<Entry[]>("GET", "/admin/v1/provider_keys");- expect(- normalize(find(pks, seedPk.id, "seed provider_key").value, ["display_name"]),- ).toEqual(- normalize(find(pks, adminPk.id, "admin provider_key").value, ["display_name"]),- );-- const models = await admin.json<Entry[]>("GET", "/admin/v1/models");- expect(- normalize(find(models, seedModel.id, "seed model").value, [- "display_name",- "provider_key_id",- ]),- ).toEqual(- normalize(find(models, adminModel.id, "admin model").value, [- "display_name",- "provider_key_id",- ]),- );-- const keys = await admin.json<Entry[]>("GET", "/admin/v1/apikeys");- expect(- normalize(find(keys, seedKey.id, "seed api_key").value, [- "key_hash",- "allowed_models",- ]),- ).toEqual(- normalize(find(keys, adminKey.id, "admin api_key").value, [- "key_hash",- "allowed_models",- ]),- );-- const exporters = await admin.json<Entry[]>("GET", "/admin/v1/observability_exporters");- expect(- normalize(find(exporters, seedExporter.id, "seed exporter").value, ["name"]),- ).toEqual(- normalize(find(exporters, adminExporter.id, "admin exporter").value, ["name"]),- );+ await assertRoundTripEqual(admin, "/admin/v1/provider_keys", seedPk, adminPk, ["display_name"], "provider_key");+ await assertRoundTripEqual(admin, "/admin/v1/models", seedModel, adminModel, ["display_name", "provider_key_id"], "model");+ await assertRoundTripEqual(admin, "/admin/v1/apikeys", seedKey, adminKey, ["key_hash", "allowed_models"], "api_key");+ await assertRoundTripEqual(admin, "/admin/v1/observability_exporters", seedExporter, adminExporter, ["name"], "exporter");🤖 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 `@tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.ts` around lines 192 - 230, Extract the repeated fetch, lookup, normalization, and equality assertion into a reusable helper near the test setup. Have the helper accept the endpoint, seed and admin IDs, descriptive labels, and fields to ignore, then use it for provider keys, models, API keys, and observability exporters in place of the duplicated blocks.
🤖 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.
Nitpick comments:
In `@tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.ts`:
- Around line 192-230: Extract the repeated fetch, lookup, normalization, and
equality assertion into a reusable helper near the test setup. Have the helper
accept the endpoint, seed and admin IDs, descriptive labels, and fields to
ignore, then use it for provider keys, models, API keys, and observability
exporters in place of the duplicated blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc470314-7dba-469b-b81d-b036602e86ae
📒 Files selected for processing (3)
tests/e2e/src/cases/seed-vs-admin-characterization-e2e.test.tstests/e2e/src/harness/index.tstests/e2e/src/harness/seed.ts
…ents to etcd
Add a harness client that seeds provider_keys / models / api_keys /
observability_exporters by writing the canonical resource document
straight to etcd under `<prefix>/<kind>/<id>` — the same write path the
control plane uses in managed mode — instead of going through the Admin
API. The interface mirrors AdminClient's create methods ({id, value}
return, generated id, same provider/adapter defaulting), so existing
call sites can migrate mechanically.
A characterization case pins the equivalence this relies on, through
both lenses:
- behavior: chat succeeds through fully seed-created resources, and
allowed_models authz rejects a seeded caller on a non-allowed model
with 403, exactly like admin-created keys;
- shape: after the store's serde round-trip (admin GET), a seeded
sparse document reads back identical to the admin-created one, field
for field, modulo identity fields and cross-references; api_keys are
additionally compared as raw stored bytes, because their GET view is
a public projection that omits attribution fields; identity fields
are pinned byte-exact.
The direct-write pattern already existed in the harness for resources
the Admin API doesn't expose (rate_limit_policies); this generalizes it
so any case can seed without the Admin API in the write path.
EtcdClient gains a single-key `get` to support the raw-bytes lens.moonming
commented
Jul 10, 2026
Applied the independent review of the first push (force-pushed
Kept as-is by design: the propagation probe's opaque-timeout behavior follows the existing harness convention ( Re-verified after the fixes: new case 2/2, full local suite 133 files / 269 tests green. |
fa31f8a to
38b2676CompareUh oh!
There was an error while loading. Please reload this page.
What
SeedClient: seedsprovider_keys/models/api_keys/observability_exportersby writing the canonical resource document (theschemas/resources/shapes) straight to etcd under<prefix>/<kind>/<id>, instead of POSTing to the Admin API. The interface mirrorsAdminClient's create methods —{id, value}return, generated id, the sameprovider/adapterdefaulting — so call sites can migrate mechanically (admin.createModel({...})→seed.createModel({...})).seed-vs-admin-characterization-e2e.test.ts, pinning the equivalence the migration relies on through two lenses:allowed_modelsauthz rejects a seeded caller on a non-allowed model with403, exactly like an admin-created key. The positive probes double as propagation gates for both front doors.api_keysare additionally compared as raw stored bytes (via a newEtcdClient.get), because their GET view is a public projection that omits attribution fields — without the raw lens, handler-side enrichment of the stored document could hide behind the projection.Why
Writing documents directly is the same front door the control plane uses in managed mode, so cases seeded this way exercise the production write path rather than the Admin API, which only standalone deployments use for writes. The pattern already existed in the harness for resources the Admin API doesn't expose (
rate_limit_policiesinteam-member-ratelimit-e2e); this generalizes it so any case can seed without the Admin API in the write path.Follow-ups land separately: the mechanical sweep of existing cases' seeding, then a wait-condition audit.
Verification
rate_limit,expires_at) carried on both sides.tsc --noEmit: zero errors in the added files (the 5 pre-existingTS18048warnings in untouched cases remain untouched).Review notes
An independent review of the first push was applied before re-push: the
api_keysshape assertion was made non-vacuous (raw-bytes comparison + optional-field fixtures — its GET view is a projection), the round-trip comment now states precisely which lens covers what (store serde round-trip vs the loader's additional JSON-Schema validation, covered by the behavioral probes), and identity fields are pinned byte-exact. One known limitation kept as-is by design: the characterization covers the fourAdminClientkinds; other kinds get their own coverage when their seeding migrates.