Consolidate low-signal tests from recent feature work - #801
Merged
Merged
Conversation
… workflow Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- service.node.test.ts: de-pin description copy; fold note-preserve into global workflow - admin-feature-flags.node.test.ts: merge HTTP lifecycle into one test; de-pin error prose - admin-feature-flag-capabilities.node.test.ts: single MCP wiring workflow with audit - e2e/admin-feature-flags.spec.ts: one journey for global toggle + per-user override Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Merge overlapping node and e2e tests into fewer workflow-oriented cases. De-pin brittle UI copy and error prose; keep structural assertions (status, data-testid, package ids, cache headers). Fold profile.frame tests into profile.node.test.ts and delete the standalone frame file. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Drop redundant account-pickers e2e and profile OG workerd smoke coverage already exercised by client filter units and node renderer tests. Merge billing-config/stripe-client micro-cases into workflow tests and fold passkey label helpers into one contract test. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kody-bot
changed the title
kody-bot
marked this pull request as ready for review
July 20, 2026 09:12
Contributor
|
🔎 Preview deployed: https://kody-pr-801.kody-a99.workers.dev Worker: Mocks:
|
cursor Bot
pushed a commit
that referenced
this pull request
Jul 27, 2026
The value-name builders, the legacy JSON parse helpers, and the second config schema existed only to serve callers that no longer exist. The value-name assertions in integration-save tests were the last references, and a test whose only subject is dead code is not coverage. One integration config schema remains, the one with an inline clientId, so there is no longer a 'with client id' variant to disambiguate. The migration test now builds its fixtures as literal historical JSON, which is what a migration test should assert against anyway rather than keeping a production schema alive to describe its own input. Also drop the E2E integration seeder. It has had no callers since #801 removed its last one, so it was already dead; rewriting it against the new tables would only create something to maintain. The _integration: prefix stays in the platform-reserved value guard so a shadowing value cannot be created later. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kody-bot
pushed a commit
that referenced
this pull request
Jul 27, 2026
…ables (#979) * feat(integrations): add user_oauth_apps and user_integrations tables Integration config lived in the values store as _integration:<name> JSON blobs, with the OAuth client id in a separate plain user value reached through a clientIdValueName pointer. Model an OAuth app (client credentials plus provider endpoints) separately from a connection (one connected account). Production has one Google app serving four connected accounts, so rotating client credentials was four writes with four chances to half-finish; it is now one. Composite (user_id, slug) keys and a composite foreign key keep per-user isolation structural rather than conventional. No token, refresh token, or client secret column exists in either table: those stay in secret_entries and are referenced by name. Backfill dedupes on the full app tuple rather than just the credential pair, so connections merge into one app only when they agree on every app-level field. Divergent rows split into separate apps instead of silently inheriting one row's endpoints. Fail-closed CHECK(0) assertions abort the transaction unless every migratable row produced exactly one connection with a resolvable app. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * feat(integrations): read capabilities from D1 and add oauth app surface integration_save/get/list/delete keep their names and flat output shape so user package code and search are unaffected; clientIdValueName becomes clientId now that the id is stored inline. Add integration_oauth_app_list and integration_oauth_app_rotate_credentials so credential rotation is a single write across every connection sharing an app. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(search): build integration entities from D1 instead of value rows The integration search entity derived candidates by scanning every user value and JSON-parsing the ones with an _integration: prefix. It now queries the integrations service directly. Entity type, {name}:integration ref format, and the indexed document field set are unchanged, so ranking and agent-facing behavior do not move. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(runtime): resolve integration client ids inline createAuthenticatedFetch and the OpenAPI integration-auth path each read the client id back out of the values store through clientIdValueName. The id now arrives on the config, which removes a sandbox round-trip and the 'Client ID value not found' failure mode from the token refresh path. Host allowlist enforcement, secret placeholder construction, and the 401-refresh-retry are unchanged. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * feat(account): group connections by oauth app and stop reading values in connect The connect flow reached into the values store from the browser: it rebuilt the _integration: value name client-side, read the config with value_get, read the client id with a second value_get, and wrote it back with value_set. It now goes through GET /account/integrations.json?name=. The integrations page groups connections under the OAuth app they share, so four Google accounts read as four accounts on one app rather than four unrelated integrations. Tokens continue to land only in the secret store. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(values): hide platform-reserved value prefixes from agents value_list only filtered isReservedValueName, an empty Set, so agents saw platform plumbing mixed into the user's own config. The account UI already hid these prefixes via its own duplicated list. Move the guard into the values layer, filter it from value_list, reject writes to those prefixes from value_set with a pointer to the right capability, and stop leaking _openapi: rows into generic value search. Internal saveValue is deliberately unguarded because platform code still writes _openapi: rows through it. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * docs(integrations): document the oauth app model and cover deletion inventory Both new tables join the account deletion and export inventory, connections before apps to respect the ON DELETE RESTRICT direction. The guardrail tests apply live migrations and fail on any uncovered user_id column. Add the integrations primitive, which the taxonomy never had, and correct docs/guides/oauth.md, which claimed integrations were stored as _integration:<name> values. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(integrations): delete the values-era config helpers The value-name builders, the legacy JSON parse helpers, and the second config schema existed only to serve callers that no longer exist. The value-name assertions in integration-save tests were the last references, and a test whose only subject is dead code is not coverage. One integration config schema remains, the one with an inline clientId, so there is no longer a 'with client id' variant to disambiguate. The migration test now builds its fixtures as literal historical JSON, which is what a migration test should assert against anyway rather than keeping a production schema alive to describe its own input. Also drop the E2E integration seeder. It has had no callers since #801 removed its last one, so it was already dead; rewriting it against the new tables would only create something to maintain. The _integration: prefix stays in the platform-reserved value guard so a shadowing value cannot be created later. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): preserve non-canonical integration value names in the backfill A value named literally `_integration:` passed the migratable filter and produced an empty app slug and connection name, then had its source row deleted. The old parseIntegrationValueName rejected empty and non-canonical names, so this was a regression against the previous validation. Require a non-empty canonical suffix in every copy of the migratable predicate, so those rows survive as values instead. Capture, delete, and remain predicates stay identical, which is the property that guarantees the delete can never outrun the insert. Also make the staging tables restart-safe, and cover the cases that matter if this ever goes wrong: an assertion firing must leave every _integration:* row in place, and an integration whose client id value is missing must be neither migrated nor deleted. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): store canonical use_pkce and canonicalize app slugs The backfill wrote use_pkce = 0 for an explicit usePkce: false, but the application layer can never produce that row: PKCE-off is already the default for confidential flow, so normalizeIntegrationConfig omits the field and writes NULL. findOauthAppByAppTuple compares with IS, so a migrated 0 never matched a freshly normalized NULL and an app that should have been reused was not, letting a reconnect create a duplicate app or move a connection off the app its siblings share. The backfill now applies the same omit-when-default rule, so there is one canonical on-disk representation. The round-trip test could not catch this because toIntegrationConfig normalizes on read, so the config compared equal while the stored row did not; the new test asserts stored row values directly. Also canonicalize slugs on every oauth app path. They were only trimmed, so an agent passing Google got 'not found' while integration_get('Google') resolved, contradicting the rule that no lookup depends on caller casing. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): persist the setup client id and share one app-resolution path The setup step held the entered client id only in session storage, so abandoning the flow before token exchange lost it and a later reconnect showed an empty field. Re-entering a client id means a trip back to the provider's dashboard, so setup now persists the app row up front. A connectionless app is a valid intermediate state: the FK points from connection to app, not the reverse. Reusing a matched app no longer rewrites its provider. That field is derived from the incoming connection name, so saving an unrelated connection with an identical app tuple relabeled the app its siblings share. Reuse is now purely additive, and a connection moved off its old app takes any orphaned app row with it. Both write paths now go through one resolveOrCreateOauthApp, so the usePkce, token-exchange-style, client-secret, and slug-allocation rules cannot drift between setup and connect. The first draft of the setup fix reimplemented all of them in the app layer, which is how two of the reviewer findings on this branch happened in the first place. Also correct docs that named requiredHosts instead of required_hosts_json, implied every credential lives in secret_entries when client_id is inline, and described app matching as credentials plus endpoints rather than the full app tuple. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): resolve setup prefill by provider family, not slug guess The connect prefill assumed an app's slug equals the connection name. App resolution dedupes, so setting up a second account persists under the shared app's slug: google-calendar lands on the google app. The fallback looked for a slug named google-calendar, found nothing, and skipped the prefill — leaving the original regression in place for exactly the multi-account case this change exists to support. Resolution is now connection, then exact slug, then provider family. A family can legitimately hold different client ids (spotify and spotify-family do), so prefill only happens when every candidate agrees. Guessing would surface as an opaque provider error at token exchange rather than as a visibly wrong field. Lives in the service rather than the account loader, since the app layer holding its own copy of app resolution is what produced the earlier provider-rewrite and duplicate-app bugs. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): write canonical column values in the backfill The backfill stored several columns exactly as they came out of the legacy JSON, while the application layer normalizes before writing. Where those columns sit in the app tuple, that mismatch means findOauthAppByAppTuple cannot match a migrated row, so a reconnect allocates a fresh app and strands its siblings on the old slug. extra_authorize_params_json now sorts keys, and scope_separator drops the default single space, both of which are compared in the tuple. Also aligned token_exchange_style, client_secret_secret_name, authorize_url, required_hosts_json, and scopes_json with their normalizers, and left a note that SQL BINARY ordering matches localeCompare for the lowercase keys OAuth providers actually use. This is the third instance of the same class after use_pkce, so the tests now assert stored column values rather than round-tripped config: reading through toIntegrationConfig normalizes and hides exactly this defect. Verified by dry-running the migration over the real production blobs: 19 connections collapse to 15 apps, every config matches what the application layer produces from the same input, all client-id values survive, and re-saving a shared-app sibling afterward reuses its app. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * fix(integrations): agree field by field on provider-family prefill The family fallback treated candidates as interchangeable when they shared a client id, then prefilled everything from whichever app won. Sharing a client id does not imply sharing the rest: github and github-kent hold the same client id but different client-secret names, so a new github-* setup would have been handed the wrong secret name and one app's endpoints. Each field is now prefilled only when every app in the family agrees on it. That keeps the field users actually resent re-entering while never inventing a client-secret name they did not choose. Refusing to prefill at all would be safe but discards the shared client id in a real case, and picking a winner is what produced the wrong default. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(account): drop the unguarded value actions from secrets.json The connect flow was the only caller of value_get and value_set on /account/secrets.json, and it now reads integration config from D1 through its own endpoint. What remained was an authenticated read/write path over arbitrary user values with no reserved-name check, so it could still write legacy _integration: blobs or corrupt a live _openapi: binding while the MCP capability and the account values UI both refuse those prefixes. Removing them is better than adding a third copy of the guard: /account/values already owns value CRUD and applies it. No caller remains anywhere in the client, handlers, or e2e. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
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
2 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.
Reduce maintenance weight from agent-authored micro-tests added with recent billing, feature-flag, community social, blog, passkey, and picker work.
Summary
Notable removals / merges
e2e/account-pickers.spec.ts(client filter units cover search matching).profile-og-image.workers.test.tsandprofile.frame.node.test.ts(folded/covered elsewhere).System recap — composes existing primitives (low risk)
Mode: recap · Base:
main· Head:cursor/test-quality-guidelines-6460Classification: composes — test-only cleanup; no primitives added or changed.
Primitives touched
billingfeature-flagsrbaccommunity-listingsapp-uisaved-packagesSystem map
Test-only consolidation across recently touched surfaces; production contracts unchanged.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
Invariants
No
userIdisolation or auth contract changes.