Repository navigation
fix(opencode): carry the user's sign-ins into the isolated database - #780
AliRezaei-Code wants to merge 3 commits into
Conversation
[Critical impact] The change copies existing provider credentials into the database used by OpenCode's isolated harness. Fix the credential source path and read the source tables in one transaction before merging. FindingsFix with agent prompt### Issue 1
src/local/native_store.rs:699
`prepare_opencode` uses `opencode_db(NativeStore::Legacy)` as the credential source, but that path honors `OPENCODE_DB`. OpenCode's sign-in commands ignore that variable, as this change explains. Users who set `OPENCODE_DB` therefore copy from their session database instead of the database holding their sign-ins, leaving the harness signed out.
Resolve the credential source separately, without the `OPENCODE_DB` override.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
### Issue 2
src/local/opencode_db.rs:228-231
`inherit_credentials` reads each source table separately without a source transaction. If the user's OpenCode process adds and selects an account after `account` is read but before `account_state` is read, the copy saves the new `active_account_id` without its account row. OpenCode can then remain signed out until another copy repairs it.
The destination transaction cannot prevent this mismatch. Read all four source tables from one read transaction so the account and its pointer come from the same saved state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryCopies OpenCode sign-ins into the existing isolated database during preparation.
Reviews (2) · Last reviewed commit: "fix(opencode): carry the user's sign-ins..." · Reviewed by Greptile |
OpenCode keeps provider credentials in the credential table of its own database and does not follow OPENCODE_DB to find them: opencode auth list prints the same accounts with and without it. orx runs OpenCode against an isolated database so sessions never touch the user's history, which therefore started with no credentials at all - the served API reported no connected integration, the harness read as signed out while the user was plainly signed in, and its catalog collapsed to the anonymous free models. Signing in from the harness could not repair it either, because that login writes back to the user's own database. Copy the credential, account, account_state and control_account tables from the user's database on every prepare. One-way: the user's database is opened read-only and never written. Present rows are updated in place, never deleted and re-inserted, because a replacement that half-succeeds would drop a working sign-in; presence is each table's own primary key, read from the schema (control_account is keyed by (email, url), account_state by its singleton id). A table whose key the two schemas disagree on is skipped rather than guessed. Update policy follows what each table can answer. Where time_updated exists the user's copy wins only when strictly newer, so a token the isolated server refreshed mid-turn is not reverted while a deliberate re-login still lands. account_state carries no timestamp, so it is mirrored: switching accounts must not leave the isolated store pointing at the old one. Only shared columns are copied, so schema drift cannot fail startup, and an unreadable or locked user database is a warning rather than an outage. Verified against a real installation: 13 stored sign-ins imported, harness authenticated true, models 9 -> 670.
b73690a to
99902ff
Compare
|
Both findings were valid, and both are fixed in the current head ( P1 — account rows stayed stale. Replacement required
A table whose key the two schemas disagree on is skipped rather than guessed. P2 — delete-then-insert could drop a working sign-in. There is no New tests: |
| // locked user database must not take the agent offline, and the resulting | ||
| // signed-out state is what the harness reports anyway. | ||
| if let Err(error) = | ||
| opencode_database::inherit_credentials(&db, &opencode_db(NativeStore::Legacy)) |
There was a problem hiding this comment.
Sign-ins use the wrong database
prepare_opencode uses opencode_db(NativeStore::Legacy) as the credential source, but that path honors OPENCODE_DB. OpenCode's sign-in commands ignore that variable, as this change explains. Users who set OPENCODE_DB therefore copy from their session database instead of the database holding their sign-ins, leaving the harness signed out.
Resolve the credential source separately, without the OPENCODE_DB override.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/native_store.rs
Line: 699
Comment:
**Sign-ins use the wrong database**
`prepare_opencode` uses `opencode_db(NativeStore::Legacy)` as the credential source, but that path honors `OPENCODE_DB`. OpenCode's sign-in commands ignore that variable, as this change explains. Users who set `OPENCODE_DB` therefore copy from their session database instead of the database holding their sign-ins, leaving the harness signed out.
Resolve the credential source separately, without the `OPENCODE_DB` override.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let reader = open_readonly(source)?; | ||
| let mut writer = Connection::open(database)?; | ||
| writer.busy_timeout(std::time::Duration::from_secs(5))?; | ||
| let transaction = writer.transaction()?; |
There was a problem hiding this comment.
inherit_credentials reads each source table separately without a source transaction. If the user's OpenCode process adds and selects an account after account is read but before account_state is read, the copy saves the new active_account_id without its account row. OpenCode can then remain signed out until another copy repairs it.
The destination transaction cannot prevent this mismatch. Read all four source tables from one read transaction so the account and its pointer come from the same saved state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/opencode_db.rs
Line: 228-231
Comment:
**Active account has no row**
`inherit_credentials` reads each source table separately without a source transaction. If the user's OpenCode process adds and selects an account after `account` is read but before `account_state` is read, the copy saves the new `active_account_id` without its account row. OpenCode can then remain signed out until another copy repairs it.
The destination transaction cannot prevent this mismatch. Read all four source tables from one read transaction so the account and its pointer come from the same saved state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.The copy reads the user's own OpenCode database, so a path that is only right on one OS makes it a silent no-op on the others — and those are exactly the builds a contributor on macOS cannot click through. `xdg-basedir`, which OpenCode resolves its data directory through, has no Windows branch: the database lives under `%USERPROFILE%\.local\share\opencode` on Windows exactly as it does on macOS and Linux, never under `%APPDATA%`. Resolution is now one named function instead of an inline expression, asserted on every runner rather than only on the platform it describes, and it also pins the two relocations OpenCode honours (`XDG_DATA_HOME` and a custom `OPENCODE_DB` filename) so a user who moved their data is read from the file they actually use. The same crate and the same embedded `ui/dist` serve the CLI, the web dashboard, the macOS bundle, the Linux AppImage and the Windows app — whose launcher execs `orx.exe app` — so this path is the only place the platforms could disagree.
Two things the first pass got wrong. The credential source was `opencode_db(NativeStore::Legacy)`, which honours `OPENCODE_DB`. OpenCode does not honour it for credentials: a user who points that variable at another database for their own sessions still signs in to the default one, so the copy would read their session database and leave the harness exactly as signed out as before. `opencode_credential_db` is now that default path, resolved the same way on every platform, while session lookup keeps following the override — a session may legitimately live where the user put it. The table reads were independent, so a concurrent sign-in could commit `account` between the reads and let `account_state` name an account the copy never saw. All four tables are now read inside one read transaction, so they agree with each other. Tests pin both: the credential path ignores `OPENCODE_DB` while the session path follows it.
|
Both new findings were valid; both are fixed in P1 — sign-ins read from the wrong database. Correct, and it is the same trap this PR documents one layer up.
P1 —
|
|
Thanks for the fixes. I verified the current head ( Tip: You can customize Greptile's behavior for this repo with |
|
This one looks computed against the parent commit ( |
Closes #781
The bug
The OpenCode harness reports Signed out while OpenCode is signed in, and its catalog collapses to the anonymous free models.
orx runs OpenCode against an isolated database (
OPENCODE_DB=…/agents/opencode/opencode.db) so sessions never touch the user's own history. But OpenCode keeps provider credentials in thecredentialtable of its own database and does not followOPENCODE_DBto find them —opencode auth listprints the same accounts with and without it. So the isolated database starts with none,/api/integrationreports no connection,authenticatedis false, and onlyopencode/*-freemodels survive.The suggested repair (
OPENCODE_DB=… opencode auth login, also what the Sign in button runs) cannot fix it either: that login writes back to the user's own database, so the isolated one stays empty.Measured on a real installation:
credentialrows in the isolated DBauthenticatedThe fix
Copy
credential,account,account_stateandcontrol_accountfrom the user's database on every prepare.OPENCODE_DBoverride. That variable is what OpenCode ignores for credentials — honouring it here would read the user's session database and reproduce the bug. Session lookup keeps following it, because a session may legitimately live where the user put it.accountbetween the reads could otherwise leaveaccount_statepointing at a row that was never copied.control_accountis keyed by(email, url),account_stateby its singletonid. A table whose key the two schemas disagree on is skipped rather than guessed.time_updatedexists the user's copy wins only when strictly newer, so a token the isolated server refreshed mid-turn is not reverted while a deliberate re-login still lands;account_statecarries no timestamp, so it is mirrored — switching accounts must not leave the isolated store pointing at the old one.Every model stays selectable — nothing is filtered.
Platforms — this is not a macOS-only fix
The clients are one product on one crate: the CLI, the web dashboard (
orx up), the macOS bundle (whose executable isorx), the Linux AppImage and the Windows app (whose launcher execsorx.exe app). All of them run this code and serve the same embeddedui/dist, so the copy runs everywhere without a per-platform branch.The one place the platforms could disagree is where the user's OpenCode database lives, so that is one named function with tests that run on every runner:
$XDG_DATA_HOMEif set, else~/.local/share/opencode/opencode.db.xdg-basedir, which OpenCode resolves its data directory through, has no Windows branch: the database is under%USERPROFILE%\.local\share\opencode, not%APPDATA%.the_users_database_sits_where_opencode_puts_it_on_every_platformasserts the path contains.local/shareand noAppData, and runs on the Linux and Windows CI jobs as well.XDG_DATA_HOMEis followed. A customOPENCODE_DBfilename is followed for sessions only — credentials always come from the default database, which is what OpenCode does.Because
src/**selects therust,linux,macosandwindowschecks inscripts/ci-changes.mjs, this PR builds the Windows and Linux apps as well as the Rust suite.Verification
cargo fmt --all --check— cleancargo clippy --all-targets --locked -- -D warnings— cleancargo test --locked(with the two skips CI uses) — 1291 passed, 17 integration passedcargo test --bin orx opencode_database— 21 passed, 9 of them for the copy: no revert of a refreshed token, re-login replaces,account_statemirrored,control_accountcomposite-key update, update keeps columns the source lacks, V1 source without credential tables, missing source, column driftcargo test --bin orx path_tests— 3 passed: macOS/Windows/Linux resolution,XDG_DATA_HOMErelocation, and theOPENCODE_DBsplit between credentials and sessionsauthenticated: true, 670 models; repeated on a fresh data directory from the built macOS app:credentialrows0 → 13,authenticated: true, 671 models🤖 Generated with OpenCode