fix(db): keep tokenExpiresAt when a connection is created - #11368
fix(db): keep tokenExpiresAt when a connection is created#11368ntdatt812 wants to merge 2 commits into
Conversation
|
The red shard is not from this diff.
This PR adds no route and touches no spec — the diff is one string on a create-path allowlist in
Green here, and these are the ones that would have caught a mistake in this change: The new case is in shard 3 and passes there. Locally: |
|
Re-homed to |
|
Great catch and exemplary validation — the round-trip test plus your own mutation check is exactly how this should be done, and we independently confirmed the bug still exists on release/v3.8.51 and that no equivalent fix landed there. One structural blocker: your branch sits on top of release/v3.8.50 tip, so the PR currently carries ~19 unrelated v3.8.50-only commits toward v3.8.51. Please rebase your two commits onto the current release/v3.8.51 tip — we verified zero conflicts (both touched files are byte-identical between the two bases) and the diff will collapse to just the allowlist string, the test, and the changelog fragment. |
`createProviderConnection` copies optional fields onto the row through an allowlist. `tokenExpiresAt` was not on it and the initial object does not set it either, so `_insertConnectionRow` always bound `conn.tokenExpiresAt || null` — NULL on every insert. That makes diegosouzapw#5326 a no-op end to end. `buildOAuthConnectionCreatePayload` mirrors the computed expiry into `tokenExpiresAt` precisely so the dashboard badge does not flash "Token Expired" before the first background refresh, and the value was discarded one layer down. The update path already carries it, so a connection only gained the field after its first refresh. The existing diegosouzapw#5326 test asserts the payload object and never persists it, which is why it stayed green throughout. The new case creates the connection and reads it back through `getProviderConnections`.
745303a to
041a436
Compare
|
Rebased onto the current The branch had been cut before v3.8.51 was branched, so this PR was showing 21 commits / 131 files — nineteen of them other people's already-merged work — and the merge had gone dirty. It is now 2 commits / 3 files, and mergeable again. The defect is still present on the new base. "accessToken",
"refreshToken",
"expiresAt",
"tokenType",
"scope",
"idToken",with no Re-verified after the rebase, Mutation — deleted the single The two pre-existing assertions stay green under the mutation, and that is the whole reason this survived: they check the object Restored, and the diff is back to +52/-5 across the three files. |
|
CI came back red on 9 checks after the rebase. All nine are the base, and the rebase is what pulled them in — the branch had been sitting on a pre-v3.8.51 cut where they did not exist yet. The failing set is identical on unrelated PRs cut from the same base.
Unit Tests fast-path (1/4) — the eight failing tests here are byte-for-byte the ones failing on #11470, which contains none of this diff: Agent-card skill counts, a hard-lease inventory sweep, a golden translate-path snapshot and a minimax-m3 metadata ledger. None of them reads No new ESLint warnings fails with Stale entries in This PR's own tests pass in CI, from the fast-path shard logs on this head: The third is the new one, and it is the one that goes red the moment I have deliberately not folded a base fix into this PR — the diff stays 3 files. Happy to send the suppressions prune separately if that is useful. |
2 participants
Summary
createProviderConnectioncopies optional fields onto the row through an allowlist (src/lib/db/providers.ts:583).tokenExpiresAtis not on it, and the initial object at:568does not set it either — so_insertConnectionRowalways bindsas NULL on every insert, however good the payload was.
That makes #5326 a no-op end to end.
buildOAuthConnectionCreatePayloadmirrors the computed expiry intotokenExpiresAt(connectionPersistence.ts:105) precisely so the dashboard badge does not flash "Token Expired" before the first background refresh — and the value is discarded one layer down. The update path already carries it (providers.ts:762), so a connection only gained the field after its first refresh.One string on the allowlist.
Related Issues
Validation
npm run lint— 0 net new on the changed file (4 pre-existingno-unused-vars, same before and after)release/v3.8.50,dafb4ae80)Written test-first: the new case fails on the base with
tokenExpiresAt: null, and passes with the one-string change. Mutation-checked, after confirming the edit applied — removing the string again gives 2 pass / 1 fail, exactly the new case.Tests Added Or Updated
tests/unit/oauth-connection-tokenexpiresat-5326.test.ts— one case that persists the payload withcreateProviderConnectionand reads it back throughgetProviderConnections.The two existing cases are unchanged. Worth noting why they did not catch this: both assert the payload object and neither touches the database, so the create path could drop the field for as long as it did with the regression test green. The file now covers both halves.
Coverage Notes
src/lib/db/providers.ts— the added allowlist entry is exercised by the round-trip case, and the mutation confirms it is the line under test rather than incidental.Reviewer Notes
connectionPersistence.ts:146and the four route sites spread...tokenData+expiresAtbut omittokenExpiresAt, which looks like the same omission on update — but both consumers prefertokenExpiresAtand the create path is now correct, so I would rather that be judged separately than widened into this PR.