fix(accounts): stop a load-time drop from deleting an account - #87
fix(accounts): stop a load-time drop from deleting an account#87iceteaSA wants to merge 4 commits into
Conversation
An OAuth account whose state entry lacks a usable refresh token was removed from the config file without any deletion code running. normalizeAccount returns null for such an entry, normalizeStorage filters it out silently, and mutateAccounts - re-reading fresh under its lock and behaving exactly as designed - then writes the roster it just loaded. The account is gone, no deletion path executed, and nothing was logged, so the loss leaves no trace to diagnose from. Reproduced end to end: three accounts in, two accounts out. mutateAccounts and saveAccounts now carry a dropped entry's raw config record through to the write verbatim, so the account survives on disk until something removes it deliberately. Refusing to write instead was the obvious shape and the wrong one: updateMainRefreshState persists the main account's refresh lease through mutateAccounts on this same file, so one broken fallback entry would have taken down main token refresh, and the remove and re-login paths that could repair the account run through it too - the operator would have been trapped by the fix. Deliberate removal of a dropped account is expressible through a new allowDrop option, which the two remove paths pass. They previously could not remove such an account at all: it is absent from the loaded roster, so the mutator reported it missing. normalizeStorage now warns with the dropped ids on every load, and a config write is logged with the resulting roster, because the absence of both is what made the original loss undiagnosable. Also: a refresh response that omits refresh_token no longer fails. The grant rotates the token single-use and an absent value means the current one stands, but it was treated as a malformed response, which would arm refresh backoff on every account at once the first time the server declined to rotate. Not observed in the wild - the contract is documented by an independent implementation of the same API.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Reviewed. The bug is real and I reproduced it against The design call is right, including the part you rejected. I checked the trap you describe rather than taking it on trust: I went after verbatim preservation and was wrong. My concern was that a raw entry carries inline credentials forward indefinitely, so I checked whether they could be stripped. They cannot: Verified independently rather than from the table:
The One note for the record rather than a change request: your known gap (re-adding under a different id strands the old entry) now also means a stranded entry keeps whatever it holds until someone runs Merging. |
|
Hold the merge briefly — there are five cubic findings fixed but not yet pushed. You reviewed
One self-inflicted follow-up: the first pass at the second item sourced the user-facing message from a post-condition check, which made On verbatim preservation — you checked the right thing and reached the right conclusion, and I'd rather have your reasoning in the file than mine. Adding the comment you asked for: Agreed on the stranded-entry note: a stranded entry keeping what it holds until someone runs Will comment again the moment the commit is pushed and green — should be minutes, not hours. |
Five follow-ups from review. A refresh response carrying a defined non-string refresh_token was treated as if the field were absent and reported as a successful refresh. Absent means the server declined to rotate and the current token stands; a non-string is malformed wire data, and now throws. The remove paths read the raw roster outside mutateAccounts' lock to decide whether to pass allowDrop, so a concurrent add could leave it off and preserve the entry instead of removing it. allowDrop is now passed unconditionally, which is a no-op for a healthy account because preservation checks the loaded roster before it consults allowDrop. The pre-read survives only to answer whether the id was on disk, OR'd with whether the mutator spliced it, so the reported outcome stays honest: removing an id that never existed reports Not Found rather than claiming a removal. That second arm covers a concurrent add racing the pre-read and is not pinned by a test - reaching it needs a race the suite cannot deterministically produce. The preserve pipeline was duplicated across mutateAccounts and saveAccounts and had already drifted apart in one place, so it is now one helper both call. Extracting it surfaced that each writer was also emitting the dropped-roster warning independently. The config-write log reported the mutator's account list, which excludes the preserved entries, so the record added to make this failure diagnosable described a roster that differed from the file. It now logs what was written. The dropped-roster warning fired on every load and every write. Since a preserved entry persists until something removes it deliberately, that condition never clears and the warning repeated indefinitely. It is now deduped on the set of dropped ids, so a newly dropped account still warns. Also documents why a preserved entry keeps its credentials: state is spread over config on load, so a config-inline refresh token is load-bearing, and stripping it would turn a recoverable account into a dead one.
|
Pushed — All five cubic findings, plus the comment you asked for. Each production hunk mutation-tested individually:
One arm is unpinned and I'd rather say so than have you find it. The remove message is The credential comment went on Two notes on your review, since they changed the change: You reproduced against And you went after verbatim preservation, found it was load-bearing, and published being wrong. I hadn't examined that at all. It's now in the file because your instinct is the one the next reader will have. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Re-reviewed at The WARN dedup is the right catch. Preserve keeps a broken entry on disk indefinitely, so the load-time warning would have fired on every I checked the hazard that comes with process-wide dedup state: a test asserting the warning can be silently defeated by an earlier test that already warned the same key, with no reset seam. You handled it with per-test Also verified on this head:
Merging as-is. |
|
Four cubic findings from the 14:46 run are still open. I checked the two P2s against running code rather than reading them, and one is worse than cubic rated it — it breaks the recovery path this PR's design argument rests on. accounts.ts:1040 — duplicate on re-login (cubic confidence 5, actually live)Cubic called this latent: "No current in-tree mutator re-adds an id." That is not right. Reproduced through the real add wiring ( The duplicate is not merely on disk — it survives into the loaded roster, so routing sees two candidates with one id, the sidebar lists it twice, and This matters more than its confidence score suggests because the PR's own argument for preserve-over-refuse is "unusable is recoverable, and deleted is not." Re-login is the documented recovery, and today it lands the operator in a duplicated roster. The fix cubic proposes (filter additions against the mutator's output rather than the pre-mutator set) is right; the pre-mutator snapshot is still correct for deciding what to preserve, so the two need to be separate sets rather than one reused for both jobs. accounts.ts:546 — comma in the dedup key (confidence 10)Real but narrow. The two P3sBoth worth doing: the doc block above Where this leaves the PRThe core fix is still right and I stand by the earlier review — the bug is real, I reproduced the data loss on Holding the merge until the duplicate is closed, since shipping it would trade a silent deletion for a silently duplicated account. A regression pinning re-login-after-drop to a single roster entry is the one I would want, since that is the path a user actually walks. |
|
I prototyped the fix I recommended before leaving you to implement it, since advice I have not run is just a guess. It works, and here is the shape that does. Keep the pre-mutator set for deciding what to preserve — that part is already right, and it is what stops a deliberate removal being resurrected. Add a second set for deciding whether to append, built from the mutator's output ( const preserved = buildPreservedAdditions(configJson.value, currentAccountIds, allowDrop)
const baseConfig = configFromStorage(next)
const writtenIds = new Set(
(Array.isArray(baseConfig.accounts) ? baseConfig.accounts : [])
.map((e) => (isRecord(e) && typeof e.id === 'string' ? e.id.trim() : ''))
.filter(Boolean),
)
const additions = preserved.filter(
(raw) => !(isRecord(raw) && typeof raw.id === 'string' && writtenIds.has(raw.id.trim())),
)Measured against the four behaviours that have to hold simultaneously, on your branch:
1086 pass / 0 fail, typecheck clean with it applied. I have discarded the prototype — the branch is yours, and a regression pinning row one is the piece I did not write. Note the filter is on |
|
Hold again — cubic found a regression in The extraction I did for the duplication finding dropped a filter. cubic called it latent — "no current in-tree mutator re-adds an id". That is the part that is wrong, and I checked rather than taking the rating: re-login is a re-add. Reproduced against the real So an operator recovering from the exact bug this PR fixes gets two entries for one account — the fresh one and the stale raw one carrying the old broken data. Every roster iteration afterwards sees both. It is on the primary recovery path, not a latent edge. The irony is worth stating plainly: the refactor existed to stop the two writers from diverging, and it introduced a semantic change to both at once. Two sets were collapsed into one because they happened to hold the same value at the call site, and they answer different questions — pre-mutator decides was this dropped at load or deliberately removed, post-mutator decides is the writer already emitting this id. The fix restores both as separate parameters with the distinction documented, since this is now the second time it has been lost. Also folding in: the dedup key joins ids with a comma, so a comma-bearing id can collide and suppress a real warning ( Will push and re-report when green. Apologies for the second hold — this one was mine, and I would rather you merge it after the round-trip than merge a roster duplicator into the recovery path. |
|
Got all three. Implementing your shape — it is better than what I had in flight, in two specific ways. Your repro is stronger than mine and proves a consequence mine did not. I reproduced the duplicate with a synthetic push mutator and stopped at the disk state. You ran it through the real add wiring — The The pre-mutator set stays where it is, deciding what to preserve. Your four-scenario matrix is the acceptance criteria, plus a padded-id case since Two things I will not do quietly, so they are on the record: The refactor that caused this was a cubic finding I accepted and specced — extract the duplicated preserve pipeline so the two writers cannot drift. It removed a filter whose purpose was not visible from the extraction site, because at that call site both sets held the same value. A DRY refactor across two writers is exactly where a semantic difference hides behind an incidental equality, and I collapsed it. And cubic rated this confidence 5 and called it latent while rating a comma-in-a-label collision confidence 10. Both were valid — nine for nine on this PR — but the severity ordering was inverted, and the one it rated lowest is the only finding that broke a user-facing path. I checked it because it touched recovery, not because of the score. Will push and report when green. |
Extracting the preserve pipeline collapsed two sets that happened to hold the same value at the call site but answer different questions. One decides WHAT to preserve - an id the load dropped, as opposed to one the mutator deliberately removed - and must be read before the mutator runs. The other decides whether the writer is already emitting that id, and can only be read after. Using the first for both meant an id the mutator re-added was preserved alongside itself. Re-login is exactly that case. A load-dropped account is by definition absent from the roster upsert is handed, so upsert finds no match and pushes a fresh entry; the preserve pass then appended the stale raw one next to it. The duplicate survived into the loaded roster, so routing saw two candidates under one id, the sidebar listed it twice, and remove spliced only the first - on the path an operator walks to recover from the failure this whole change exists to prevent. The two sets are separate again: the helper takes the pre-mutator set and decides preservation, and each writer filters the result against the ids it is about to serialize. Both sides compare trimmed ids, matching the roster collector, so a padded id cannot slip past the comparison and reintroduce the duplicate. Also: the warning dedup key joined ids with a comma, so one id containing a comma collided with two ids that did not and silently suppressed a real warning. It now encodes the sorted array.
|
Pushed — Your four scenarios, run against the real And the original repro that produced The regression you left to me is One honest gap in that pinning. Mutating the trim on the written side alone reddens nothing: The comma-collision key is Worth recording what the extraction actually did wrong, since it is the more general lesson: it collapsed two sets that were incidentally equal at one call site but answer different questions — what to preserve, read before the mutator, versus what is already being emitted, readable only after. DRY across two writers is exactly where that kind of difference hides, because the duplication looks like duplication right up until one of the copies means something else. The helper now takes the first and each writer computes the second, which is more code and harder to collapse again. Ready to merge. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The doc block on buildPreservedAdditions still described an emittedIds parameter that moved to the call sites, so the comment explaining the distinction that has now broken twice described a signature the function does not have. It documents the real one: the helper decides what to preserve from the load-side view, and each writer separately filters that against the ids it is about to serialize, because only the writer knows its own output. Two stranded duplicate doc blocks from the same extraction are gone, and a misspelled reference to collectConfigRosterIds is fixed. The dedup-collision test used fixed ids against a module-level set that is never reset and is shared across the whole process, while its siblings deliberately namespace theirs. It now namespaces too. The collision it exercises needs the two id sets to join to the same string, which constrains the prefix to sort before the unprefixed id - recorded in the test, since it otherwise reads as an arbitrary choice.
|
Pushed — The doc block still described an The dedup test used fixed ids against a module-level set that is never reset and is shared process-wide, while its own siblings namespace theirs with Mutation-verified myself rather than from the report, since the construction is subtle enough that a namespaced version could easily stop triggering the collision it exists to catch: reverting the dedup key to Nothing outstanding on my side. Every cubic finding across the three runs is addressed, and the two arms I know are unpinned are stated in-thread rather than buried — the mutator-splice OR arm, and the written-side For the record on this PR's review loop: eleven cubic findings, all eleven valid on inspection, zero false positives. Its severity ordering was the unreliable part — the re-login duplicate that broke the recovery path came in at confidence 5, below a comma-in-a-label collision at 10. Worth knowing for anyone calibrating how much to trust the rating versus reading the finding. |
|
Verified at The duplicate guard landed on both writers with the
Row two is the one I flagged as a trap rather than a request, and you pinned it. Neutralizing the guard at both writers reddens exactly those two tests and nothing else, so they gate the behaviour rather than describe it. 1089 pass / 0 fail, typecheck, lint and format clean on my machine. On your calibration note — agreed, and it generalizes. Eleven findings, eleven valid, and the ordering inverted: the confidence-5 finding broke the recovery path the whole design rests on, while the confidence-10 one cost a suppressed log line. The reason is worth naming, because it will recur: severity models rate the code shape in front of them. A duplicate append looks local and cheap. What made it expensive was context no reviewer of that hunk could see — that re-login is the documented recovery for the exact condition this PR creates, so the defect landed precisely where the design promised safety. Read the finding, decide severity against the system. Worth noting what this loop actually cost: three revisions, two of which came from findings I had already approved past. I signed off on the preserve twice before walking the recovery path, and the duplicate was sitting on it both times. Merging. |
2 participants
An OAuth account whose state entry lacks a usable
refreshtoken gets removed from the config file, with no deletion code running and nothing logged.normalizeAccountreturnsnullfor such an entry,normalizeStoragefilters it out silently, andmutateAccounts— re-reading fresh under its lock, behaving exactly as designed — writes the roster it just loaded. Reproduced end to end against real code: three accounts in, two accounts out, zero log lines.This is not hypothetical. It cost an operator two accounts, and because the roster write is unlogged and the filter is silent, the loss left nothing to diagnose from. Finding it took log forensics across 38 processes and four parallel investigations; the deletion itself was invisible.
The fix
mutateAccountsandsaveAccountsnow carry a dropped entry's raw config record through to the write verbatim. The account survives on disk until something removes it deliberately. It stays unusable until re-login — but unusable is recoverable, and deleted is not.Refusing to write was the obvious shape and the wrong one. I built that first.
updateMainRefreshStatepersists the main account's refresh lease throughmutateAccountson this same file, so one broken fallback entry would have taken down main token refresh — and the remove and re-login paths that could repair the account run through it too. The operator would have been trapped by the fix, holding an error message telling them to re-login through a path the fix had just blocked. Preserve has no such failure mode.Deliberate removal is expressible through a new
allowDropoption, which both remove paths pass. They previously could not remove such an account at all: it is absent from the loaded roster, so the mutator reported it missing.Observability
normalizeStoragewarns with the dropped ids on every load, and a config write logs the resulting roster. The absence of both is what made the original loss undiagnosable — worth fixing independently of the deletion itself.Also: omitted
refresh_tokenno longer failsThe refresh grant rotates the token single-use, and an absent
refresh_tokenon a successful exchange means the current one stands. It was treated as a malformed response, which would arm refresh backoff on every account simultaneously the first time the server declined to rotate.Not observed in the wild — flagging it as defensive. The contract is documented by an independent implementation of this same API, and the failure mode is bad enough to close ahead of observation.
Verification
1076 pass / 0 fail (baseline 1064), typecheck and biome clean.
Every production hunk was mutation-tested rather than assumed covered — this repository has shipped tests that looked like coverage and gated nothing, so each hunk was reverse-applied individually and confirmed to redden:
allowDropwiring (commands.ts)saveAccountsparallel preserverefresh_tokenreuseThe pinned regressions that matter: a load-dropped account survives an unrelated write; an
updateMainRefreshState-shaped mutation succeeds while a broken fallback exists (this is the trap the first design walked into);removeof a load-dropped account deletes it from disk; normal removal and first-run still work.Reviewed adversarially cross-family across two rounds. The first round found the fail-stuck design and the main-refresh breakage; the second confirmed preserve cannot resurrect a deliberately removed account and that no third config writer bypasses it.
Known gap
Re-adding the same ChatGPT account under a different id leaves the old broken raw entry in place, needing a manual remove. Out of scope here.
cli.ts's remove wiring is not directly pinned —main()is not exported, and a test re-invoking the same logic through mocked argv would be coverage that gates nothing. Its wiring is identical to the pinnedcommands.tspath.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Stops silent account loss from load-time drops. Before: accounts missing a usable refresh token were filtered at load and erased on the next write with no logs. Now: we preserve the raw entry on write, warn on load (deduped), and require an explicit allow-drop to remove it.
No migration actions.
Written for commit 2ad77e5. Summary will update on new commits.