feat(integrations): model OAuth apps and connections as first-class tables - #979
Merged
Merged
Conversation
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>
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>
… 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>
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>
… 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>
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>
…nventory 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>
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>
…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>
…able-design-c822 # Conflicts: # tools/migration-ledger.json Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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:
📝 WalkthroughWalkthroughThe change introduces dedicated OAuth app and connection storage, migrates legacy integration values, updates OAuth and refresh flows to store client IDs directly, adds OAuth-app MCP capabilities, integrates connections into search and UI flows, centralizes reserved-value handling, and documents the architecture. ChangesOAuth integrations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectOauthRoute
participant AccountSecretsHandler
participant IntegrationService
participant SecretStore
User->>ConnectOauthRoute: complete OAuth authorization
ConnectOauthRoute->>AccountSecretsHandler: submit clientId and token metadata
AccountSecretsHandler->>SecretStore: save access and refresh tokens
AccountSecretsHandler->>IntegrationService: upsert OAuth app and connection
IntegrationService-->>ConnectOauthRoute: return stored connection config
sequenceDiagram
participant MCPClient
participant OAuthAppCapability
participant IntegrationService
participant OAuthAppRepository
MCPClient->>OAuthAppCapability: rotate shared OAuth app credentials
OAuthAppCapability->>IntegrationService: validate and rotate credentials
IntegrationService->>OAuthAppRepository: update app client fields
OAuthAppRepository-->>IntegrationService: return updated app
IntegrationService-->>OAuthAppCapability: return public app and connections
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
🔎 Preview deployed: https://kody-pr-979.kody-a99.workers.dev Worker: Mocks:
|
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>
![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 (7)
packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql (1)
101-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMigration file is
0101but every internal identifier and message says0100.Staging table names (
__migration_0100_source,__migration_0100_app_groups) and all assertion messages ("aborting 0100.") reference the wrong migration number, which will mislead anyone debugging a failed run. Test names also say0100.Also applies to: 357-361
🤖 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 `@packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql` around lines 101 - 104, Update all internal staging-table identifiers, assertion messages, and test names in this migration from 0100 to 0101, including the DROP TABLE statements and the “aborting 0100” messages. Keep the migration logic unchanged and ensure every reference consistently matches migration 0101.packages/worker/src/integrations/service.ts (1)
214-249: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffApp and connection writes are not atomic.
upsertOauthAppandupsertIntegrationConnectionare separate round trips; a failure in between leaves an orphan app row (or an app updated without its connection). Considerdb.batch([...])so both statements commit together.🤖 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 `@packages/worker/src/integrations/service.ts` around lines 214 - 249, Update the upsertOauthApp and upsertIntegrationConnection flow to execute both database writes in a single atomic db.batch operation, ensuring neither an orphan app nor a partially updated integration remains if either statement fails. Preserve the existing row values and subsequent getIntegration verification.packages/worker/src/app/handlers/account-secrets.ts (1)
641-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidation failure discards all Zod issue detail.
safeParsefailures surface only a generic string, so a misconfigured connect request is undiagnosable. Logparsed.error.issues(paths/codes only — no secret values are present in this config) before throwing.♻️ Proposed change
if (!parsed.success) { + console.error('Invalid OAuth integration configuration.', { + userId: input.userId, + issues: parsed.error.issues.map((issue) => ({ + path: issue.path, + code: issue.code, + })), + }) throw new Error('OAuth integration configuration is invalid.') }🤖 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 `@packages/worker/src/app/handlers/account-secrets.ts` around lines 641 - 643, Update the parsed.success failure branch to log parsed.error.issues, preserving the issue paths and codes without exposing secret values, before throwing the existing generic configuration error in the surrounding OAuth configuration validation flow.packages/worker/src/app/handlers/account-secrets.node.test.ts (1)
411-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates what is verified.
upsertIntegrationis mocked, so no app reuse happens here — the test only asserts the handler forwards two configs sharing aclientId. Actual reuse is covered inservice.node.test.ts; rename to something like "forwards both connections with the same client id to the integrations service".🤖 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 `@packages/worker/src/app/handlers/account-secrets.node.test.ts` around lines 411 - 479, Rename the test around the two connect_oauth calls to describe forwarding both connections with the same clientId, rather than reusing an existing OAuth app. Keep the existing assertions and test behavior unchanged, since mockModule.upsertIntegration only verifies the handler passes both configurations.packages/worker/src/integrations/types.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Zod 4’s
z.url()schema.
packages/worker/package.jsondepends on Zod^4.3.6, wherez.string().url()is deprecated in favor ofz.url(). Update these URL fields toz.url()and keep.nullable()where needed.🤖 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 `@packages/worker/src/integrations/types.ts` around lines 16 - 18, Update the URL schemas in the relevant type definition: replace z.string().url() with Zod 4’s z.url() for tokenUrl, authorizeUrl, and apiBaseUrl, preserving nullable() on the latter two fields.packages/worker/client/routes/connect-oauth.tsx (1)
1322-1350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct unit test for
toStoredIntegrationConfig.This new exported function has non-trivial normalization logic (URL trimming,
requiredHostsdedup/sort, conditionalauthorization/tokenExchangeStyle) but the provided test file only exercisesparseStoredIntegrationConfig, which is a separate code path. A dedicated test would catch regressions in the server→client integration payload mapping used for OAuth reconnects.🤖 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 `@packages/worker/client/routes/connect-oauth.tsx` around lines 1322 - 1350, Add direct unit coverage for the exported toStoredIntegrationConfig function, using an integration payload that verifies URL and secret-name trimming, requiredHosts normalization, boolean/null usePkce handling, and conditional tokenExchangeStyle and authorization mapping. Include assertions for omitted optional fields and preserve existing parseStoredIntegrationConfig tests.packages/worker/src/mcp/tools/search-detail.ts (1)
129-152: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueIntegration detail always hits the DB, unlike the sibling
valuebranch.The
valuebranch above first checksinput.searchRows.userValueRowsbefore falling back to a livegetValuecall, but this integration branch always issues a livegetJoinedIntegrationDB call even wheninput.searchRows.userIntegrationRows(visible in theOptionalSearchRowsResult/test fixtures) may already contain the joined integration. If callers commonly pass pre-loaded search rows, this is an avoidable round trip per detail lookup.If freshness of
clientId/secret names isn't a hard requirement here, consider mirroring thevaluebranch's cache-then-fallback pattern.🤖 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 `@packages/worker/src/mcp/tools/search-detail.ts` around lines 129 - 152, Update the integration detail branch around getJoinedIntegration to first reuse a matching entry from input.searchRows.userIntegrationRows, then call getJoinedIntegration only when no cached row exists. Preserve the existing not-found error and subsequent toIntegrationConfig/response construction for both cached and fallback paths.
🤖 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 `@docs/contributing/architecture/data-storage.md`:
- Around line 286-294: Update the `user_integrations` description to name the
storage column `required_hosts_json` instead of the API-shaped `requiredHosts`,
and narrow the credential-placement statement so it says only secret credential
values belong in `secret_entries`; preserve that `client_id` remains inline in
`user_oauth_apps`.
In `@docs/guides/oauth.md`:
- Around line 119-121: Update the OAuth app matching documentation near the
`user_oauth_apps` description to state that deduplication uses the complete
app-level configuration, including flow/PKCE settings, token exchange style,
scope separator, and extra authorize parameters, in addition to client
credentials and provider endpoints.
In `@packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql`:
- Around line 179-181: Update the value_entries join conditions in the
migration’s capture and delete paths to safely extract $.clientIdValueName from
unvalidated JSON, avoiding errors for malformed values. Treat missing or invalid
clientIdValueName as non-migratable consistently across capture, delete, and
assertion checks.
In `@packages/worker/src/integrations/service.ts`:
- Around line 125-160: Update the app reuse logic around matchedApp and existing
so provider is derived from the slug being retained, not the new config name.
When reusing matchedApp, rebuild or override appRowFields.provider using
matchedApp.slug before upsertOauthApp; apply the same correction in the existing
branch using existing.app.slug, while preserving all other matched row values.
---
Nitpick comments:
In `@packages/worker/client/routes/connect-oauth.tsx`:
- Around line 1322-1350: Add direct unit coverage for the exported
toStoredIntegrationConfig function, using an integration payload that verifies
URL and secret-name trimming, requiredHosts normalization, boolean/null usePkce
handling, and conditional tokenExchangeStyle and authorization mapping. Include
assertions for omitted optional fields and preserve existing
parseStoredIntegrationConfig tests.
In `@packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql`:
- Around line 101-104: Update all internal staging-table identifiers, assertion
messages, and test names in this migration from 0100 to 0101, including the DROP
TABLE statements and the “aborting 0100” messages. Keep the migration logic
unchanged and ensure every reference consistently matches migration 0101.
In `@packages/worker/src/app/handlers/account-secrets.node.test.ts`:
- Around line 411-479: Rename the test around the two connect_oauth calls to
describe forwarding both connections with the same clientId, rather than reusing
an existing OAuth app. Keep the existing assertions and test behavior unchanged,
since mockModule.upsertIntegration only verifies the handler passes both
configurations.
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 641-643: Update the parsed.success failure branch to log
parsed.error.issues, preserving the issue paths and codes without exposing
secret values, before throwing the existing generic configuration error in the
surrounding OAuth configuration validation flow.
In `@packages/worker/src/integrations/service.ts`:
- Around line 214-249: Update the upsertOauthApp and upsertIntegrationConnection
flow to execute both database writes in a single atomic db.batch operation,
ensuring neither an orphan app nor a partially updated integration remains if
either statement fails. Preserve the existing row values and subsequent
getIntegration verification.
In `@packages/worker/src/integrations/types.ts`:
- Around line 16-18: Update the URL schemas in the relevant type definition:
replace z.string().url() with Zod 4’s z.url() for tokenUrl, authorizeUrl, and
apiBaseUrl, preserving nullable() on the latter two fields.
In `@packages/worker/src/mcp/tools/search-detail.ts`:
- Around line 129-152: Update the integration detail branch around
getJoinedIntegration to first reuse a matching entry from
input.searchRows.userIntegrationRows, then call getJoinedIntegration only when
no cached row exists. Preserve the existing not-found error and subsequent
toIntegrationConfig/response construction for both cached and fallback paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73806228-173f-4c8e-9b1f-552205b82940
📒 Files selected for processing (67)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/index.mddocs/contributing/architecture/integrations.mddocs/contributing/architecture/primitives.yamldocs/guides/oauth.mddocs/use/search.mde2e/d1-utils.tspackages/worker/client/routes/account-integrations.tsxpackages/worker/client/routes/account-values.tsxpackages/worker/client/routes/connect-oauth.node.test.tspackages/worker/client/routes/connect-oauth.tsxpackages/worker/client/routes/integration-filter.node.test.tspackages/worker/client/routes/integration-filter.tspackages/worker/migrations/0101-user-oauth-apps-and-integrations.sqlpackages/worker/src/app/account-data-targets.tspackages/worker/src/app/account-integrations-data.tspackages/worker/src/app/account-values-data.tspackages/worker/src/app/handlers/account-integrations.node.test.tspackages/worker/src/app/handlers/account-integrations.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/app/handlers/account-values.node.test.tspackages/worker/src/app/loader-data.tspackages/worker/src/integrations/migration.node.test.tspackages/worker/src/integrations/repo.tspackages/worker/src/integrations/service.node.test.tspackages/worker/src/integrations/service.tspackages/worker/src/integrations/types.tspackages/worker/src/mcp/capabilities/integrations/domain.tspackages/worker/src/mcp/capabilities/integrations/integration-delete.tspackages/worker/src/mcp/capabilities/integrations/integration-get.tspackages/worker/src/mcp/capabilities/integrations/integration-list.tspackages/worker/src/mcp/capabilities/integrations/integration-oauth-app-list.tspackages/worker/src/mcp/capabilities/integrations/integration-oauth-app-rotate-credentials.tspackages/worker/src/mcp/capabilities/integrations/integration-save.node.test.tspackages/worker/src/mcp/capabilities/integrations/integration-save.tspackages/worker/src/mcp/capabilities/integrations/integration-shared.tspackages/worker/src/mcp/capabilities/integrations/oauth-app-shared.tspackages/worker/src/mcp/capabilities/meta/search.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.node.test.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/capabilities/values/value-capabilities.node.test.tspackages/worker/src/mcp/capabilities/values/value-list.tspackages/worker/src/mcp/capabilities/values/value-set.tspackages/worker/src/mcp/execute-modules/authenticated-fetch.node.test.tspackages/worker/src/mcp/execute-modules/kody-runtime-utils.node.test.tspackages/worker/src/mcp/execute-modules/kody-runtime-utils.tspackages/worker/src/mcp/tools/integration-package-suggestions.node.test.tspackages/worker/src/mcp/tools/search-core.tspackages/worker/src/mcp/tools/search-descriptors.tspackages/worker/src/mcp/tools/search-detail.node.test.tspackages/worker/src/mcp/tools/search-detail.tspackages/worker/src/mcp/tools/search-entity-plugin.tspackages/worker/src/mcp/tools/search-entity-plugins/integration.tspackages/worker/src/mcp/tools/search-entity-plugins/value.tspackages/worker/src/mcp/tools/search-entity-registry.node.test.tspackages/worker/src/mcp/tools/search-format-types.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-handler.node.test.tspackages/worker/src/mcp/tools/search-loaders.tspackages/worker/src/mcp/tools/search-types.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/values/service.node.test.tspackages/worker/src/mcp/values/value-name-guards.node.test.tspackages/worker/src/mcp/values/value-name-guards.tspackages/worker/tsconfig-client.jsontools/migration-ledger.json
💤 Files with no reviewable changes (1)
- e2e/d1-utils.ts
Comment on lines
+179
to
+181
| INNER JOIN value_entries cid | ||
| ON cid.bucket_id = e.bucket_id | ||
| AND cid.name = json_extract(e.value, '$.clientIdValueName') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import sqlite3
db = sqlite3.connect(':memory:')
db.execute("CREATE TABLE t(name TEXT, value TEXT)")
db.execute("INSERT INTO t VALUES ('_integration:bad', 'not json')")
try:
print(db.execute(
"SELECT 1 FROM t a JOIN t b ON b.name = json_extract(a.value,'$.x') "
"WHERE json_valid(a.value)").fetchall())
except Exception as exc:
print('error:', exc)
PYRepository: kentcdodds/kody
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching migration name:\n'
fd -a '0101-user-oauth-apps-and-integrations\.sql$' . | sed 's#^\./##'
file="$(fd '0101-user-oauth-apps-and-integrations\.sql$' . | head -n 1)"
if [ -n "${file:-}" ]; then
printf '\nLine count:\n'
wc -l "$file"
printf '\nOccurrences of relevant symbols:\n'
rg -n "json_extract\(|json_valid\(|clientIdValueName|_integration:" "$file" || true
printf '\nRelevant sections:\n'
sed -n '160,190p;395,448p' "$file" | cat -n
fi
printf '\nSearch for alias-equivalence tests:\n'
fd -a 'migration\.node\.test\.ts$' . | sed 's#^\./##'
testfiles="$(fd 'migration\.node\.test\.ts$' . | grep -F 'packages/worker' || true)"
if [ -n "${testfiles:-}" ]; then
for f in $testfiles; do
printf '\n--- %s ---\n' "$f"
rg -n "json_valid|json_extract|clientIdValueName|WHERE|predicate" "$f" -C 2 || true
done
fiRepository: kentcdodds/kody
Length of output: 33909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sqlite3
for predicate in [
"WHERE json_valid(value)",
"WHERE json_valid(value) AND name = json_extract(value,'$.x')",
"WHERE json_extract(value,'$.x') IS NULL",
"WHERE name = json_extract(value,'$.x')",
]:
db = sqlite3.connect(':memory:')
db.execute("CREATE TABLE t(name TEXT, value TEXT)")
db.execute("INSERT INTO t VALUES ('_integration:bad', 'not json')")
try:
rows = db.execute(f"SELECT name FROM t JOIN t ON value = value {predicate}").fetchall()
print(predicate, "rows:", rows, "last_error:", db.execute("select 'ok'").fetchone())
except Exception as exc:
print(predicate, "error:", type(exc).__name__, str(exc), db.execute("select 'ok'").fetchone() if 'conn' in locals() else None)
PYRepository: kentcdodds/kody
Length of output: 545
🌐 Web query:
SQLite json_extract JSON path invalid JSON value behavior returns NULL or error documentation
💡 Result:
In SQLite, the json_extract function behaves as follows regarding invalid JSON input and missing paths: 1. Invalid JSON Input: The standard json_extract(X, P) function raises an error if the input JSON (X) is not well-formed [1][2]. If you require a "null-if-error" behavior instead of an error, SQLite provides the json_nextract(X, P) function, which returns NULL if the input is not well-formed JSON [2]. 2. Missing Paths: If the JSON document is well-formed but the specified path (P) does not exist within it, json_extract returns NULL [3][4]. The distinction between these behaviors is important for error handling. Because json_extract is strict regarding the validity of the JSON input, you can use the json_valid function to check the input document before processing if you wish to avoid errors with untrusted data [2][4]. In contrast, the newer SQLite operators -> and ->> have different default behaviors: - The -> operator returns NULL if the left-hand side is not well-formed JSON [2][5]. - The ->> operator raises an error if the left-hand side is not well-formed JSON, mirroring the behavior of json_extract [2][5].
Citations:
- 1: https://www.sqlite.org/json1.html
- 2: https://sqlite.org/src/doc/json-enhancements/doc/json-enhancements.md
- 3: https://sqlite.org/forum/forumpost/1b658a32d0
- 4: https://www.dbpro.app/learn/sqlite/sqlite-json-extract-function
- 5: https://sqlite.org/src/artifact/7f67a2e75de23958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/worker/src/integrations/migration.node.test.ts"
python3 - <<'PY'
from pathlib import Path
text = Path("packages/worker/src/integrations/migration.node.test.ts").read_text()
start = text.index("test('0100 capture/delete/remain predicates stay alias-equivalent'")
print(text[start:text.find("});\n", start)+3])
PYRepository: kentcdodds/kody
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path("packages/worker/src/integrations/migration.node.test.ts"),
Path("packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql"),
]:
text = path.read_text()
for needle in ["test('0100 capture/delete/remain predicates stay alias-equivalent'", "WHERE json_valid(e.value)", "json_extract(e.value, '$.clientIdValueName')", "json_extract(value_entries.value, '$.clientIdValueName')"]:
print(f"{path}: {needle!r} ->", text.find(needle))
PY
rg -n "json_nextract|->>|->|json_valid|json_extract\\(value_entries\\.value|json_extract\\(e\\.value" packages/worker/migrations/0101-user-oauth-apps-and-integrations.sqlRepository: kentcdodds/kody
Length of output: 3751
Guard json_extract on unvalidated values in the JSON path checks.
json_valid(...) only guards the later SELECT filters, so a valid _integration:* row whose JSON has a missing $.clientIdValueName member is dropped during the capture/delete/assert paths. The same join condition also exposes malformed JSON in the ON clause. Use a non-erroring extract for this member lookup, or otherwise treat missing clientIdValueName as non-migratable in both the capture and delete paths.
🤖 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 `@packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql` around
lines 179 - 181, Update the value_entries join conditions in the migration’s
capture and delete paths to safely extract $.clientIdValueName from unvalidated
JSON, avoiding errors for malformed values. Treat missing or invalid
clientIdValueName as non-migratable consistently across capture, delete, and
assertion checks.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/integrations/service.node.test.ts (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
applyMigrationsBeforehelper.This function is identical to the one already defined in
packages/worker/src/integrations/migration.node.test.ts. Consider extracting it to a shared test-support module (e.g. alongsidecreateD1FromSqlite) to avoid drift between the two copies.🤖 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 `@packages/worker/src/integrations/service.node.test.ts` around lines 27 - 33, Remove the duplicate applyMigrationsBefore helper from service.node.test.ts and extract or reuse a shared test-support implementation alongside createD1FromSqlite. Update both migration.node.test.ts and service.node.test.ts to import the shared helper while preserving its existing filtering, sorting, and execution behavior.
🤖 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 `@packages/worker/src/integrations/service.node.test.ts`:
- Around line 27-33: Remove the duplicate applyMigrationsBefore helper from
service.node.test.ts and extract or reuse a shared test-support implementation
alongside createD1FromSqlite. Update both migration.node.test.ts and
service.node.test.ts to import the shared helper while preserving its existing
filtering, sorting, and execution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb4b231a-8678-4e6f-aa54-0898c61113c1
📒 Files selected for processing (5)
packages/worker/migrations/0101-user-oauth-apps-and-integrations.sqlpackages/worker/src/integrations/migration.node.test.tspackages/worker/src/integrations/service.node.test.tspackages/worker/src/integrations/service.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/migration-ledger.json
- packages/worker/src/integrations/service.ts
- packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql
…lution 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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/app/handlers/account-secrets.ts (1)
457-482: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded
saveIntegrationConfigthrow after secrets are already persisted.
saveIntegrationConfigcan throw, andhandleConnectOauthActionalso hascatchat line 497 only forbuildConnectOauthHostApprovalLinks, not forsaveIntegrationConfig. SincesaveSecretpersists the access/refresh token secrets before this call, add a try/catch aroundsaveIntegrationConfigand return the validation failure as a clean JSON error to avoid orphaned secrets and a propagated exception.🤖 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 `@packages/worker/src/app/handlers/account-secrets.ts` around lines 457 - 482, The handleConnectOauthAction flow must guard the saveIntegrationConfig call because secrets are persisted before it runs. Wrap saveIntegrationConfig in try/catch, and on failure return the handler’s established validation-failure JSON response instead of propagating the exception; keep the existing successful integrationName flow unchanged.
🤖 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.
Outside diff comments:
In `@packages/worker/src/app/handlers/account-secrets.ts`:
- Around line 457-482: The handleConnectOauthAction flow must guard the
saveIntegrationConfig call because secrets are persisted before it runs. Wrap
saveIntegrationConfig in try/catch, and on failure return the handler’s
established validation-failure JSON response instead of propagating the
exception; keep the existing successful integrationName flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d856a58-0ac4-4376-a837-9d4763acf20b
📒 Files selected for processing (11)
docs/contributing/architecture/data-storage.mddocs/guides/oauth.mdpackages/worker/client/routes/connect-oauth.node.test.tspackages/worker/client/routes/connect-oauth.tsxpackages/worker/src/app/account-integrations-data.tspackages/worker/src/app/handlers/account-integrations.node.test.tspackages/worker/src/app/handlers/account-secrets.node.test.tspackages/worker/src/app/handlers/account-secrets.tspackages/worker/src/integrations/service.node.test.tspackages/worker/src/integrations/service.tspackages/worker/src/mcp/capabilities/integrations/integration-save.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/guides/oauth.md
- packages/worker/src/app/account-integrations-data.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/client/routes/connect-oauth.node.test.ts
- packages/worker/src/app/handlers/account-integrations.node.test.ts
- packages/worker/src/integrations/service.ts
- packages/worker/src/mcp/capabilities/integrations/integration-save.node.test.ts
- packages/worker/client/routes/connect-oauth.tsx
… 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>
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>
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>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8148d67. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/src/integrations/service.node.test.ts (2)
861-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name overstates what it covers.
Only one app exists here, so
findOauthAppForProviderSetupreturns via the single-candidate shortcut (candidates.length === 1), never reachingmergeOauthAppFamilyPrefill. Consider renaming to reflect the sole-family-member path, or seeding a second agreeing google app so the merge path is actually exercised.🤖 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 `@packages/worker/src/integrations/service.node.test.ts` around lines 861 - 893, The test name claims to cover family prefill merging, but its single Google app triggers the single-candidate shortcut instead. Update the test to seed a second agreeing Google app so findOauthAppForProviderSetup exercises mergeOauthAppFamilyPrefill, or rename the test to accurately describe the single-family-member path.
957-965: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd cross-user isolation coverage for
findOauthAppForProviderSetup.Every new test uses a single
userId, so nothing pins the per-user scoping of the exact-slug and provider-family lookups. Since prefill returnsclientId, a regression that drops theuser_idpredicate fromlistOauthAppsByProviderwould leak another user's client id into a setup form undetected. Extend this test to seed an app under a different user and assertnull.As per coding guidelines, "Scope every read and write path by
userId... to prevent cross-user data sharing."🧪 Proposed test addition
test('findOauthAppForProviderSetup returns null for a brand-new provider', async () => { const { env } = createEnv() + await upsertIntegration({ + env, + userId: 'user-other', + config: { ...baseGoogleConfig, name: 'linear-other' }, + }) const found = await findOauthAppForProviderSetup({ env, userId: 'user-empty', name: 'linear', }) expect(found).toBeNull() })🤖 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 `@packages/worker/src/integrations/service.node.test.ts` around lines 957 - 965, Extend the test for findOauthAppForProviderSetup to create a matching OAuth app for a different user, then invoke the lookup with user-empty and assert it still returns null. Ensure the seeded app exercises the exact-slug or provider-family lookup and includes a clientId so cross-user leakage is detected.Source: Coding guidelines
packages/worker/src/integrations/service.ts (1)
455-460: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare
extraAuthorizeParamsin a key-order-independent way.
JSON.stringify({ access_type, prompt })differs fromJSON.stringify({ prompt, access_type }), so logically identical stored parameter maps can fail equality and prevent the prefill from using that field. Use a sorted-key comparison instead.♻️ Order-independent comparison
-function sameExtraAuthorizeParams( - left: Record<string, string>, - right: Record<string, string>, -) { - return JSON.stringify(left) === JSON.stringify(right) -} +function sameExtraAuthorizeParams( + left: Record<string, string> | null, + right: Record<string, string> | null, +) { + if (left === right) return true + if (!left || !right) return false + const leftKeys = Object.keys(left).sort() + const rightKeys = Object.keys(right).sort() + return ( + leftKeys.length === rightKeys.length && + leftKeys.every((key, index) => + rightKeys[index] === key && left[key] === right[key], + ) + ) +}🤖 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 `@packages/worker/src/integrations/service.ts` around lines 455 - 460, Update sameExtraAuthorizeParams to compare the key/value entries of left and right independently of insertion order, using sorted keys before comparison. Preserve equality for maps with identical parameters regardless of key order, while still returning false when keys or values differ.
🤖 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 `@packages/worker/src/integrations/service.node.test.ts`:
- Around line 861-893: The test name claims to cover family prefill merging, but
its single Google app triggers the single-candidate shortcut instead. Update the
test to seed a second agreeing Google app so findOauthAppForProviderSetup
exercises mergeOauthAppFamilyPrefill, or rename the test to accurately describe
the single-family-member path.
- Around line 957-965: Extend the test for findOauthAppForProviderSetup to
create a matching OAuth app for a different user, then invoke the lookup with
user-empty and assert it still returns null. Ensure the seeded app exercises the
exact-slug or provider-family lookup and includes a clientId so cross-user
leakage is detected.
In `@packages/worker/src/integrations/service.ts`:
- Around line 455-460: Update sameExtraAuthorizeParams to compare the key/value
entries of left and right independently of insertion order, using sorted keys
before comparison. Preserve equality for maps with identical parameters
regardless of key order, while still returning false when keys or values differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb3601d3-89c0-4714-b728-bca460c86fe3
📒 Files selected for processing (12)
packages/worker/client/routes/connect-oauth.node.test.tspackages/worker/client/routes/connect-oauth.tsxpackages/worker/migrations/0101-user-oauth-apps-and-integrations.sqlpackages/worker/src/app/account-integrations-data.node.test.tspackages/worker/src/app/account-integrations-data.tspackages/worker/src/app/handlers/account-integrations.node.test.tspackages/worker/src/app/loader-data.tspackages/worker/src/integrations/migration.node.test.tspackages/worker/src/integrations/repo.tspackages/worker/src/integrations/service.node.test.tspackages/worker/src/integrations/service.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/worker/src/app/loader-data.ts
- tools/migration-ledger.json
- packages/worker/src/integrations/migration.node.test.ts
- packages/worker/src/app/account-integrations-data.ts
- packages/worker/src/app/handlers/account-integrations.node.test.ts
- packages/worker/migrations/0101-user-oauth-apps-and-integrations.sql
- packages/worker/client/routes/connect-oauth.tsx
- packages/worker/client/routes/connect-oauth.node.test.ts
- packages/worker/src/integrations/repo.ts
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>
7 tasks
This was referenced Jul 27, 2026
This was referenced Aug 5, 2026
This was referenced Aug 19, 2026
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.
OAuth integration config lived in the user values store as
_integration:<name>JSON blobs, with the OAuth client id in a separate plain user value reached through aclientIdValueNamepointer. This replaces that with two first-class D1 tables in a single behavior-preserving migration.Why
Integrations were already a first-class primitive everywhere except storage — they have a capability domain, their own search entity type, an account page, an OAuth connect flow, a runtime fetch helper, and a named security invariant. Remote connectors, MCP client servers, webhooks, secrets, jobs, and packages all have tables; integrations were the last thing pretending to be config strings. That had concrete costs: listing integrations scanned the user's entire value set and JSON-parsed the matching rows, nothing stopped
value_setfrom writing a garbage body over a live_integration:github, and agents callingvalue_listsaw platform plumbing mixed into the user's own config.The app/connection seam came out of the production data rather than a guess. Measured against the account being migrated, 16 client-id value names held only 14 distinct client ids:
github/github-kentalias, and so dox/x-kodykoala. Three groups share one OAuth app. Across all three, the fields that are always identical are exactly the app-level ones, and the only fields that ever differ are scopes, required hosts, and token secret names — so that is where the split goes.The payoff is that rotating client credentials is one write. Four Google connections shared one client id and one client secret, so rotation used to be four writes with four chances to half-finish.
Migration
One transaction: create both tables, insert apps, insert connections, delete the source value rows, assert. No expand/contract, no dual-read, no value mirrors — every reader is in this repo and it is a single Worker deploy, so correctness is proven by verifying equivalence rather than by carrying a compatibility window.
Two decisions worth reviewing:
token_urlorflow— grouping on the full tuple splits those into separate apps instead of silently overwriting them.CHECK (0)assertions run before the delete. Every migratable row must have produced exactly one connection with a resolvable app. Any violation aborts the transaction with every_integration:*row still in place.Expected result: 19 connections → 15 apps (google 4→1, x 2→1, github stays 2 because they share a client id but reference different client secrets, plus 11 singletons).
The 16
<provider>-client-idvalues are copied intouser_oauth_apps.client_idand deliberately left in place — they are ordinary user values that may be referenced from package code, and quietly deleting someone's data to tidy a table is the one genuinely breaking move available here.Compatibility
integration_save/_get/_list/_deletekeep their names and their flat output shape, so user package code and the search contract are unaffected. The one intended change isclientIdValueName→clientId. Search keeps theintegrationentity type, the{name}:integrationref format, and the same indexed document field set.Security
No token, refresh token, or client secret column exists in either table; those stay in
secret_entriesand are referenced by name. The two host gates remain independent — a connection'srequiredHosts(checked before any token is attached) and each secret's ownallowedHosts(enforced by the fetch gateway) — andfetch-gateway.ts,integration-host-allowlist.ts, andsecrets/allowed-hosts.tsare untouched. The client id is stored inline and returned to the browser, which is intentional: it appears in authorize URLs and was already a plaintext user value.This also removes an unguarded authenticated read/write path over arbitrary user values (
value_get/value_seton/account/secrets.json), which the connect flow was the last caller of.Verification
npm run validatepasses. Beyond that, I dry-ran the migration over the real production blobs locally — seeding the actual 19_integration:*values and 16 client-id values, applying the migration exactly as D1 wraps it, and asserting the result. It produces 19 connections on 15 apps with the expected grouping, every migrated config byte-identical to what the application layer produces from the same input, every_integration:*row deleted, all 16 client-id values preserved, and — importantly — re-saving a shared-app sibling afterward reuses its app rather than allocating a duplicate. That last assertion is what catches the canonicalization class of bug below. The dry run was not committed because its fixtures contain real client ids.Two independent reviewers audited the diff with a hard-to-reverse lens, and both AI reviewers on the PR are now clean. Their findings were all real and are fixed:
_integration:was migratable, produced an empty slug, and got deletedd31ec816— non-empty canonical suffix required in every copy of the predicateuse_pkce = 0where the app layer can only writeNULL, so app reuse silently failed11fe3263providerfrom the incoming connection name9a3759c79a3759c798b865feextra_authorize_params_jsonandscope_separatorstored non-canonically, both in the app tuple80b6c786github/github-kentcould get each other's client-secret name8148d678Three of those were the same class — the backfill storing a representation the application layer cannot produce — so migration tests now assert stored column values rather than round-tripped config, because reading through
toIntegrationConfignormalizes and hides exactly that defect.Deliberately not in scope
Moving
_openapi:*bindings out of the values store (they are refreshable snapshots with a 900 KB cap and may not want D1 at all), an account UI for managing OAuth apps beyond the grouped list, and three data-quality items the audit surfaced:githubandgithub-kentsharing a client id with two different client secrets, the redundantx-kodykoala-client-id, andlinkedinstoringhttps://api.linkedin.comwhere a bare host belongs. Also four connections (groupme, linkedin, slack, telegram) declare arefreshTokenSecretNamefor a secret that was never written because the provider returned no refresh token; migrated as-is rather than silently changed. Tracked in a follow-up issue.System recap — adds a new primitive (high risk)
Mode: recap · Base:
main@1cacfbb8· Head:7f6e5170Classification: adds — introduces the
integrationsprimitive, which the taxonomy did not previously have, and moves a storage layer underneath five existing primitives.Primitives touched
integrationsuser_oauth_apps+user_integrationsd1-app-db0101adds both tables and deletes the source value rowsmcp-serverclientIdValueName→clientId; adds two oauth-app capabilitiesvaluesvalue_listhides platform prefixes;value_setrejects writes to themapp-uicapabilities-executecreateAuthenticatedFetchresolves the client id inlineopenapi-bindingsSystem map
Integration config moves out of the values store into D1, and every reader — MCP capabilities, search, the execute runtime, and the connect flow — re-points at the new service.
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
per-user-isolation— both tables use a composite(user_id, slug)primary key and a composite(user_id, app_slug)foreign key, so a connection structurally cannot reference another user's OAuth app. The backfill groups byuser_id, and the client-id lookup joins within one bucket.integration-host-allowlist—requiredHostsstays per-connection and is still asserted before a token is attached. Not merged into the app row, and not collapsed with each secret'sallowedHosts.no-secrets-in-chat— neither table has a token or client-secret column; only names.Summary by CodeRabbit