Repository navigation
fix(auth): keep credentials the current build cannot decode on mutation - #53864
SeashoreShi wants to merge 2 commits into
Conversation
`Auth.all()` filters out entries whose JSON shape the running build does not recognize. `set()` and `remove()` both wrote back from `all()`, so a single mutation rewrote auth.json from the filtered map and permanently deleted every undecodable credential — including entries written by another build or by the Desktop app, which also mutates auth.json in the background. Read the raw (undecoded) map for mutations and keep the filtering in `all()` for reads, so undecodable entries survive `set`/`remove` untouched.
github-actions
Bot
added
contributor
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reproduced the bug from #42387 through the CLI. With an entry this build can't read in auth.json, opencode auth logout openai on dev deletes that entry too. With this PR it is kept and only openai is removed. The new test fails on the base and passes here, and typecheck passes.
The fix is small and in the right place: set and remove now write back from the unfiltered file contents, and all()/get() still hide entries they can't read. One side effect: when OPENCODE_AUTH_CONTENT is set, all() now filters those entries too, where before it returned them unchecked. That seems fine and more consistent.
Two small notes on the test, inline.
| // `all()` filters those out for reads, and the old `set`/`remove` wrote back | ||
| // the filtered map, so a single `set` permanently deleted every entry whose | ||
| // JSON shape this build did not recognize (see #42387). | ||
| it.instance("set preserves entries the current build cannot decode", () => |
There was a problem hiding this comment.
This test sits outside the describe("Auth") block (the closing }) comes just before it), so it shows up at the top level with odd indentation. Could you move it inside the block?
Also, the linked issue's reproduction goes through remove() (auth logout). Adding one auth.remove("anthropic") call, then checking that future-provider is still on disk, would cover both write paths without making the test any bigger.
…nd cover remove
Review feedback: the test sat outside `describe("Auth")`, and the issue's
reproduction goes through `remove()` (`auth logout`). Move it inside the block
and exercise both write paths (set + remove).
|
Thanks for the review and for reproducing the Both notes are addressed in 47051df: the test now lives inside the |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
This branch has not been deployed
1 participant
Issue for this PR
Closes #42387
Type of change
What does this PR do?
Auth.all()(packages/opencode/src/auth/index.ts) decodes auth.json and drops entries it cannot decode (Record.filterMap(..., decode, () => undefined)). Bothset()andremove()read throughall()and thenwriteJsonthe result, so a single mutation rewrote auth.json from the filtered map and permanently deleted every undecodable credential — including entries written by a newer/older build or by the Desktop app, which also mutates the file in the background (provider-metadata refresh). This is silent, unrecoverable data loss.The fix separates reading from mutating: a new internal
read()returns the raw undecoded map,all()=read()+ filter (read behaviour unchanged), andset/removewrite back from the raw map, so entries the current build cannot decode are preserved verbatim.How did you verify your code works?
Added a test in
test/auth/auth.test.ts: an undecodable entry sits next to a valid one; afterauth.set(...)andauth.remove(...)the undecodable entry is still on disk untouched, the removed key is gone, andall()still hides the undecodable entry. It fails against the previous code. The reviewer also reproduced theauth logoutpath from the issue and confirmed the fix (and that the new test fails on base / passes here, with typecheck passing).Local note: my checkout could not run this package's tests/typecheck (
effectsubpath/version resolution fails on basedevtoo), so CI is doing the red/green confirmation for my side.Screenshots / recordings
n/a — data-layer change.
Checklist