From 69441aa097b08865fdb4d5f42a07a3a9528485e1 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Wed, 28 Oct 2026 14:27:47 -0700 Subject: [PATCH] Pick the first claude account that is under its limits docket takes the accounts in config.toml's order and runs a new review under the first one that no recorded usage limit applies to, so it returns to an earlier account as soon as that account's limit resets. Claude-Session: https://claude.ai/code/session_01AovXvZFRJZXuPP3RLtqHUA --- CLAUDE.md | 2 +- README.md | 2 +- internal/core/account/account.go | 43 +++----------- internal/core/account/account_test.go | 82 ++++++++++++--------------- internal/core/config/config.go | 8 +-- internal/core/session/limit_test.go | 18 +++--- 6 files changed, 61 insertions(+), 94 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b0a3397..23c4730 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -131,7 +131,7 @@ Work in flight is drawn with one spinner that the root owns, in `styles.Busy`, w M5 is in: the review requests screen (`i`). `session.Requests` runs one REST search for `user-review-requested:` and one per `teams` entry in `config.toml`, and returns them raw as `requests.Fetched`. Each team search adds `-author:`, because CODEOWNERS asks the author's own team to review the author's pull request. GitHub refuses to ask the author directly, so the search that names the user needs no exclusion. On PostHog/team-feature-flags the exclusion took the search from 31 results to 27, and the four it dropped were the user's own. A team's list also leaves out a pull request the user has reviewed at its current head (`caughtUpCheck`). One further search for `reviewed-by: -author:` serves every team, because GitHub allows 30 searches a minute and a search per team would pass that with 15 teams. It runs when the first team has rows, a failure of it stands for every later team without searching again, and only the pull requests in both it and a team's rows are checked. A check reads `Reviews` for the commits of the user's submitted reviews (`review.ReviewedCommits`), and reads `PR` for `headRefOid` only when there is one. A pull request the user's own search returned is never checked, and one that two teams' searches returned is checked once, because `requests.Group` counts a pull request only under the first search that returned it. The checks run one after another, like the searches. A check that fails ends the checks of that team's rows, keeps every row no check found caught up, and adds the reason to `Team.Errs`, and the requests screen draws the reason above the rows. The failed pull request still counts as checked, so a later team does not try it again. On PostHog/team-feature-flags this took the list from 27 to 26, dropping #95457 and keeping #100734, where the user had only replied in threads. Only a new head brings a row back. A team request made after the user's review does not, because it asks nothing new of the user: on #95457 the team was requested 26 seconds after the user approved that same commit. A team whose search fails carries the error in `requests.Team.Errs`, and the other searches still run, because GitHub answers 422 to a team slug it cannot resolve. It reads the cached login or asks `gh` for one, and never calls `Login`, because `Login` writes the login to `config.toml` and a dry run may open the screen. The root keeps that and regroups it through `requests.Group` whenever the records reload, so a record's state shows on its row without searching GitHub again. `Group` puts each pull request in one of three sections (`requests.Kind`), and a pull request appears once, because a mark and the cursor follow the pull request. `Yours` holds a request that names the user, a team request the user is assigned to, and any pull request with an open record. Otherwise an assignee other than the owner (`PR.Owner`) is a reviewer, and `Covered` holds a team request with a reviewer among `Team.Members`. Everything else is `NeedsReviewer`, which lists the pull requests a member owns first. The owner is the author of a person's pull request, because PostHog's authors assign themselves, so an author's assignment says nothing about review. A bot's pull request has no owner, so its assignee counts as its reviewer, except when the body carries `Task started by: a person` (`StartedByAPerson`), where the bot's own first assignment names the owner (`StartedBy`). `Requests` reads the team's members (`TeamMembers`) for each team with rows and the issue events (`FirstAssignee`) for each such bot pull request that does not also name the user, after the caught-up check has dropped what it can. It leaves out a pull request whose `Owner` is the user (`ownPR`), as `-author:` leaves out the user's own. A read that fails adds its reason to `Team.Errs` and keeps the rows. `Group` gives each reason a line of its own, because the screen cuts each line at its width. A failed read of the members also sets `Team.MembersUnknown`, which `Group` copies to each row, and the screen then neither calls an assignee "not on the team" nor splits the rows that need a reviewer by teammate. A bot pull request a person started whose starter is unknown lands in `NeedsReviewer`, because its member assignee may be the starter. `Fetched.Login` is the login the searches named, so a regroup can find the user's rows. The requests screen hides draft pull requests until `d`, as the dashboard hides archived records until `a`, and hides the covered rows until `c`. `SetList` keeps the list it was given and draws it through `withoutDrafts`, so `d` and `c` search nothing and a reload keeps both choices. `Needs a reviewer` and `Covered` are drawn only when `List.Teams` names a team, and a row names its team only when it names more than one. When some marked pull requests are not assigned to the user, `enter` asks whether to assign the user (`Unassigned` counts them), and `msg.StartBatch.Assign` carries the answer. `startBatch` then calls `Service.Assign` for every pull request whose launch started or waits on the trust prompt, because both leave a record. GitHub keeps an assignee it is asked to add again, so the pull requests the user is already assigned to need no exception. A dry run says it would assign instead. Each section's heading counts the drafts it hides, and `helpFor` builds the footer's `d` label from `ShowDrafts` as it builds `a` from `ShowArchived`. The screen's header line says neither, because the root cuts that line at the terminal's width and it already runs past 76 columns. Hiding the drafts drops their marks through the same rule that drops the mark of a row that left the list, because a batch would otherwise start a pull request the user cannot see. It uses `gh api search/issues` rather than `gh search prs`, because the latter goes through GraphQL and refused with a rate-limit error while the REST search still answered. `requests.PR` lives in the pure package rather than in `gh`, because a screen may not import `gh`. A batch runs `Prepare` and `StartBackground` for each marked pull request in turn inside one command and reports once, because each single start answers with a `detectedMsg` that would overwrite the status line. A batch calls `session.Existing` for every marked pull request before it starts any (`checkBatch`), because review-code would otherwise ask whether to append or overwrite in a background session nobody can answer. When any has a review, the root holds the checked batch in `App.batch` and the requests screen asks once for all of them: append, overwrite, or skip them and start the rest. The answer arrives as `msg.AnswerBatch`. `esc` puts the list back with the marks kept and sends nothing, so `App.batch` stays until the next check replaces it. A pull request whose check fails is left out. Its failure shows on the status line under the question and again in the batch's report. When every check fails, nothing starts and the list keeps its marks. `resetBusy` leaves the requests screen's `Busy` alone, because the check always answers with a `batchCheckedMsg` that clears it, and an unrelated error that cleared it would let a second `enter` check and start the same batch. A dry run asks the same question and then explains the commands. `o` on the requests screen opens the selected pull request in the browser: a row with an open record sends `msg.OpenOnGitHub`, as the dashboard does, and any other row sends `msg.OpenPullRequest` with its URL. Both go through the root's `browse`. `t` on the requests screen opens the teams screen, which lists the teams `gh api user/teams` returns alongside the configured ones, matched without regard to case because GitHub slugs are case-insensitive, and keeps a configured team GitHub does not list so the user can uncheck it. A failed read of the teams, such as one for a token without `read:org`, answers `teamsLoadedMsg` with the error rather than `errMsg`, so the configured teams stay on show. `enter` saves through `SaveTeams`, which writes the `teams` key alone through `config.SaveKey`, and the requests screen searches again. A failed save answers `teamsSavedMsg` with the error for the same reason, so `resetBusy` leaves both of the screen's markers alone. `Login` caches the login through `SaveKey` too, so docket writes only the keys it sets. `SaveKey` reads the file as written, so a save neither writes the defaults and expanded paths the service's `Config` holds nor undoes a hand edit to another key made while docket runs. The screen starts from the teams the service loaded at startup, so a hand edit to `teams` made while docket runs is lost on the next save from the screen, the same way the requests screen does not search for it until a restart. The root refuses `OpenTeams` while a search, a batch check, or a save of the teams runs, and says why: a save searches again, so a running search could answer after the save's search and show the old teams, and a batch check's answer and a finished save each switch to the requests screen. A read still running from an earlier visit answers the reopened screen instead of a second read, because `SetMemberships` merges idempotently and keeps the rows `Load` checked. A save that finishes after `esc` leaves the screen alone. It searches again only when the requests screen is showing and no search is running, because the next `i` searches afresh and GitHub's searches for one user run one at a time. `Service.cfgMu` guards config.toml and the two `Cfg` fields that change after startup, the login and the teams. Writers hold it, and every reader of those two fields goes through `Service.Config()`, which copies `Cfg` under it. `Login` checks the cache again under the lock, so two first detections that both ask `gh` write config.toml once. The lock covers one process: two docket instances that write config.toml at the same moment can still drop each other's key. Without the lock, the rename that lands last drops the other write's key, and a search can read the teams mid-save. Other reads of `Cfg` through its value-receiver methods, such as `NotesPath`, still copy the struct without the lock. They read only fields fixed at startup, but the copy includes the two that change. A dry run reports the teams it would write and writes nothing. -Claude account rotation is in. `claude_fallback_config_dirs` in `config.toml` lists the accounts that follow `claude_config_dir`, and `Config.ClaudeAccounts` is the rotation order, each account once. `Load` turns any spelling of `~/.claude` in either key into "" (`accountDir`), because claude's default account runs with the variable unset. It keeps every other path as written, because claude 2.1.308 hashes the exact string into the name of the keychain entry it looks up, so `/x/` and `/x` are two logins. A `CLAUDE_CONFIG_DIR` that docket inherited stays as given. `account.Pick` starts after the first account that the most recent limit applies to, passes every account that limit applies to, and takes the first one whose limits have reset, so docket stays on an account until it hits a limit and returns to the first only after a later one does. claude counts usage per login, which is one account in one organization, and the same person has one account UUID in every organization. A limit therefore applies to every dir signed in to the login it was recorded under. `recordLimit` stores that login in `Limit.Login` when it records the limit, because the user can `/login` a dir to another login later, and the old limit belongs to the old login. `account.ReadLogin` reads it from `oauthAccount.accountUuid` and `oauthAccount.organizationUuid` in `.claude.json`, which claude keeps inside a named dir and at `~/.claude.json` for the default account (`ClaudeLoginFile`). A limit applies to a dir when both logins are known and equal. A login is known only with both ids. When either is unknown, it applies to the dir it was recorded for, which covers a line written before the field and a dir whose `.claude.json` docket cannot read. A test `Config` has no `ClaudeDefaultDir`, so no test reads the real `~/.claude.json`. Every docket instance appends to and reads the one log at `Paths.Limits`, so they all pick the same account without a lock or a stored pointer. `resolve` stamps a new claude record with Pick's account, and a codex record with `claude_config_dir` without reading the log (`rotatesAccounts`). When every account is over its limit it still starts under the one that resets first, so a limit raised since claude reported it costs nothing. `Rereview` (through `rearm`) and `Restart` (through `readyAgain`) keep the record's own account unless that account is over its limit now (`freshAccount`), which keeps the rule that a change to `claude_config_dir` applies only to new records. A codex record never moves. A record that moves drops its `AskSessionID`, because the new account cannot resume that conversation. A resume or an attach always runs under the record's account. The poll's `settleIdle` reads the transcript only for a waiting session that `settle` found nothing of its own for. A refusal there goes through `startElsewhere`: it appends the limit, leaves alone a record another instance already moved on, and writes the record as `reviewing` with no id and a new `StartedAt`. Every other path refuses that state, and `launchGrace` keeps the poll and `recoverLost` off it. It then stops the refused session under the old account and launches through `readyAgain` and `StartBackground`. The record is never `not_started` while that runs, because `start_review` and `enter` start a `not_started` row and either would launch a second session that no record names. A stop that fails writes the refused session's id back, so the next poll tries again, and no `not_started` row ever names a session. `StartBackground` refuses a record that still names a background session, so no launch writes over the id of a session claude still holds. `recordLimit` skips a refusal the log already holds. A review started fresh whose notes now exist starts with `--overwrite`, because the refused session wrote them and review-code would otherwise ask in a background session. When no account is free, the row becomes `not_started` with the time the first limit resets. `AfterExit` and `AfterAsk` record the limit of an interactive session through `noteLimit` and start nothing. `noteLimit` skips a limit that has reset, because the user can resume a refused session after the reset and leave it without sending anything. A limit claude gave no reset for counts for `unknownReset` (one hour). +Claude account rotation is in. `claude_fallback_config_dirs` in `config.toml` lists the accounts that follow `claude_config_dir`, and `Config.ClaudeAccounts` is the rotation order, each account once. `Load` turns any spelling of `~/.claude` in either key into "" (`accountDir`), because claude's default account runs with the variable unset. It keeps every other path as written, because claude 2.1.308 hashes the exact string into the name of the keychain entry it looks up, so `/x/` and `/x` are two logins. A `CLAUDE_CONFIG_DIR` that docket inherited stays as given. `account.Pick` takes the first account in that order that no limit in force applies to, so docket returns to an earlier account as soon as its limit resets. claude counts usage per login, which is one account in one organization, and the same person has one account UUID in every organization. A limit therefore applies to every dir signed in to the login it was recorded under. `recordLimit` stores that login in `Limit.Login` when it records the limit, because the user can `/login` a dir to another login later, and the old limit belongs to the old login. `account.ReadLogin` reads it from `oauthAccount.accountUuid` and `oauthAccount.organizationUuid` in `.claude.json`, which claude keeps inside a named dir and at `~/.claude.json` for the default account (`ClaudeLoginFile`). A limit applies to a dir when both logins are known and equal. A login is known only with both ids. When either is unknown, it applies to the dir it was recorded for, which covers a line written before the field and a dir whose `.claude.json` docket cannot read. A test `Config` has no `ClaudeDefaultDir`, so no test reads the real `~/.claude.json`. Every docket instance appends to and reads the one log at `Paths.Limits`, so they all pick the same account without a lock. `resolve` stamps a new claude record with Pick's account, and a codex record with `claude_config_dir` without reading the log (`rotatesAccounts`). When every account is over its limit it still starts under the one that resets first, so a limit raised since claude reported it costs nothing. `Rereview` (through `rearm`) and `Restart` (through `readyAgain`) keep the record's own account unless that account is over its limit now (`freshAccount`), which keeps the rule that a change to `claude_config_dir` applies only to new records. A codex record never moves. A record that moves drops its `AskSessionID`, because the new account cannot resume that conversation. A resume or an attach always runs under the record's account. The poll's `settleIdle` reads the transcript only for a waiting session that `settle` found nothing of its own for. A refusal there goes through `startElsewhere`: it appends the limit, leaves alone a record another instance already moved on, and writes the record as `reviewing` with no id and a new `StartedAt`. Every other path refuses that state, and `launchGrace` keeps the poll and `recoverLost` off it. It then stops the refused session under the old account and launches through `readyAgain` and `StartBackground`. The record is never `not_started` while that runs, because `start_review` and `enter` start a `not_started` row and either would launch a second session that no record names. A stop that fails writes the refused session's id back, so the next poll tries again, and no `not_started` row ever names a session. `StartBackground` refuses a record that still names a background session, so no launch writes over the id of a session claude still holds. `recordLimit` skips a refusal the log already holds. A review started fresh whose notes now exist starts with `--overwrite`, because the refused session wrote them and review-code would otherwise ask in a background session. When no account is free, the row becomes `not_started` with the time the first limit resets. `AfterExit` and `AfterAsk` record the limit of an interactive session through `noteLimit` and start nothing. `noteLimit` skips a limit that has reset, because the user can resume a refused session after the reset and leave it without sending anything. A limit claude gave no reset for counts for `unknownReset` (one hour). Merged and closed pull requests are handled. `session.decide` reads the pull request's state through `gh pr view` alongside its reviews, with the two reads running at the same time, and `record` archives a row that `Record.Finished` reports: an `unreviewed` or `reviewed` row whose pull request merged or closed. A `drafted` row stays open with a `merged` or `closed` tag, because GitHub accepts a review on a merged pull request and only the user can choose to submit it or drop it. `resolve` and `Rereview` refuse a merged or closed pull request through `openPR`, so `n`, a batch from the requests screen, and `u` cannot start a review there; `Rereview` reads the state again because the stored `PRState` is only as fresh as the last detection. `Existing` makes the same check only when it finds a review, which spares the user the append-or-overwrite choice without costing the common path a call. The refusal covers view and ask too, so `n` cannot adopt an existing review of a merged pull request. `State.Label` names `unreviewed` "no review posted" on the requests screen, and the dashboard titles that group "No review posted", because the state means the last session posted nothing new, not that the user never reviewed the pull request. Nothing reads GitHub at startup except for records left `reviewing`, so a row whose pull request merged while docket was closed archives on the next `r` or `R`. `r` and `R` skip a background review that is still running (`refuseRefresh`), because the poll owns it and an archive mid-session would stop the agent and delete the clone under it. diff --git a/README.md b/README.md index c792bdd..25515d0 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ claude_config_dir = "~/.claude-work" claude_fallback_config_dirs = ["~/.claude"] ``` -When claude refuses a background review because its account is over a usage limit, or cuts one short because the account reached its limit partway through, and the session has posted nothing, docket stops that session and starts the review again under the next account in the list. New reviews then run under that account until it hits a limit too, and docket then moves on to the next account whose limit has reset, coming back round to the first. When every account is over its limit, the review waits under "Did not start" with the time the first limit resets, and `enter` starts it again. A review you run in this terminal shows you claude's refusal itself. docket records the limit when the session ends, so `u` and the next review move to another account. docket keeps the limits claude reported in `~/.docket/limits.jsonl`. claude counts usage per login, so a limit on one directory also holds every other directory signed in to the same login, and docket skips them all. Listing two directories signed in to one login therefore adds nothing to fall back to. +When claude refuses a background review because its account is over a usage limit, or cuts one short because the account reached its limit partway through, and the session has posted nothing, docket stops that session and starts the review again under the first account in the list that is not over a limit. Each new review runs under the first account in the list that is not over a limit, so docket goes back to an earlier account as soon as its limit resets. When every account is over its limit, the review waits under "Did not start" with the time the first limit resets, and `enter` starts it again. A review you run in this terminal shows you claude's refusal itself. docket records the limit when the session ends, so `u` and the next review move to another account. docket keeps the limits claude reported in `~/.docket/limits.jsonl`. claude counts usage per login, so a limit on one directory also holds every other directory signed in to the same login, and docket skips them all. Listing two directories signed in to one login therefore adds nothing to fall back to. ## Review requests diff --git a/internal/core/account/account.go b/internal/core/account/account.go index cb801eb..a8f1afa 100644 --- a/internal/core/account/account.go +++ b/internal/core/account/account.go @@ -1,6 +1,6 @@ // Package account picks the claude account a new session runs under. docket -// rotates through the accounts config.toml names, and moves past an account once -// claude refuses one of its sessions for a usage limit. claude counts usage per +// takes the first account config.toml names, and passes over an account while +// a usage limit that claude reported for it is in force. claude counts usage per // login. A limit therefore applies to every account dir signed in to that login. package account @@ -9,7 +9,6 @@ import ( "encoding/json" "fmt" "os" - "slices" "time" ) @@ -66,44 +65,16 @@ func (c Choice) Limited() bool { return !c.Until.IsZero() } -// Pick chooses the account for a new session. accounts is the rotation order. +// Pick chooses the account for a new session. accounts is the priority order. // logins maps each account to the login it is signed in to. An account that // logins leaves out has an unknown login. limits is every limit docket has -// recorded, in any order. A limit that applies to no account in the rotation -// counts for nothing. +// recorded, in any order. // -// The rotation starts after the first account that the most recent limit -// applies to, passes every account that limit applies to, and takes the first -// account that is not over a limit. docket therefore stays on an account until -// that account hits a limit, even after an earlier account's limit resets. -// Before any account has hit a limit, the pick is the first. +// The pick is the first account that is not over a limit. docket therefore +// returns to an earlier account as soon as its limit resets. func Pick(accounts []string, logins map[string]Login, limits []Limit, now time.Time) Choice { - if len(accounts) == 0 { - return Choice{} - } - start := 0 - var latest Limit - for _, limit := range limits { - i := slices.IndexFunc(accounts, func(account string) bool { - return limit.appliesTo(account, logins[account]) - }) - if i >= 0 && limit.At.After(latest.At) { - latest, start = limit, i+1 - } - } - // The latest limit also covers every later dir on its login. A start on one of - // those would move docket back to that login once the limit resets. - for range accounts { - candidate := accounts[start%len(accounts)] - if latest.At.IsZero() || !latest.appliesTo(candidate, logins[candidate]) { - break - } - start++ - } - var soonest Choice - for step := range len(accounts) { - candidate := accounts[(start+step)%len(accounts)] + for _, candidate := range accounts { until := reset(candidate, logins[candidate], limits) if !until.After(now) { return Choice{Account: candidate} diff --git a/internal/core/account/account_test.go b/internal/core/account/account_test.go index a913675..2f30706 100644 --- a/internal/core/account/account_test.go +++ b/internal/core/account/account_test.go @@ -24,28 +24,22 @@ func at(minutes int) time.Time { return t0.Add(time.Duration(minutes) * time.Min // The user's own setup: an automation account first, and the default account // as the fallback. -func TestPickStaysOnAnAccountUntilItHitsALimit(t *testing.T) { +func TestPickReturnsToAnEarlierAccountOnceItsLimitResets(t *testing.T) { accounts := []string{automation, defaultAccount} t1 := at(0) - t2 := t1.Add(8 * 24 * time.Hour) var limits []Limit if got := Pick(accounts, nil, limits, t1.Add(-time.Hour)); got != (Choice{Account: automation}) { t.Fatalf("with no limits, pick = %+v, want the automation account", got) } - limits = append(limits, Limit{Account: automation, At: t1, Until: t1.Add(7 * 24 * time.Hour)}) + limits = append(limits, Limit{Account: automation, At: t1, Until: t1.Add(5 * time.Hour)}) if got := Pick(accounts, nil, limits, t1.Add(time.Minute)); got != (Choice{Account: defaultAccount}) { t.Fatalf("after automation hit its limit, pick = %+v, want the default account", got) } - if got := Pick(accounts, nil, limits, t1.Add(7*24*time.Hour+time.Hour)); got != (Choice{Account: defaultAccount}) { - t.Fatalf("after automation's limit reset, pick = %+v, want docket to stay on the default account", got) - } - - limits = append(limits, Limit{Account: defaultAccount, At: t2, Until: t2.Add(5 * time.Hour)}) - if got := Pick(accounts, nil, limits, t2.Add(time.Minute)); got != (Choice{Account: automation}) { - t.Fatalf("after the default account hit its limit, pick = %+v, want the automation account again", got) + if got := Pick(accounts, nil, limits, t1.Add(5*time.Hour)); got != (Choice{Account: automation}) { + t.Fatalf("after automation's limit reset, pick = %+v, want the automation account again", got) } } @@ -121,10 +115,9 @@ func TestPickWithASingleAccount(t *testing.T) { } } -// Each case lists the accounts in rotation order. The rotation starts after the -// account that hit a limit most recently, wraps past the end, and skips every -// account still over a limit. -func TestPickRotatesFromTheMostRecentlyLimitedAccount(t *testing.T) { +// Each case lists the accounts in priority order. The pick is the first account +// that is under its limits, whichever account hit a limit most recently. +func TestPickTakesTheFirstAccountUnderItsLimits(t *testing.T) { accounts := []string{automation, defaultAccount, work} for _, tc := range []struct { name string @@ -144,51 +137,50 @@ func TestPickRotatesFromTheMostRecentlyLimitedAccount(t *testing.T) { {Account: defaultAccount, At: at(20), Until: at(30)}, }, now: at(60), - want: work, + want: automation, }, { - name: "the next account is free", + name: "the first account is limited", limits: []Limit{ - {Account: defaultAccount, At: at(0), Until: at(600)}, + {Account: automation, At: at(0), Until: at(600)}, }, now: at(1), - want: work, + want: defaultAccount, }, { - name: "the rotation wraps past the last account", + name: "a later account is limited", limits: []Limit{ - {Account: work, At: at(0), Until: at(600)}, + {Account: defaultAccount, At: at(0), Until: at(600)}, }, now: at(1), want: automation, }, { - name: "the next account is still limited", + name: "only the last account is free", limits: []Limit{ - {Account: work, At: at(0), Until: at(600)}, + {Account: automation, At: at(0), Until: at(600)}, {Account: defaultAccount, At: at(5), Until: at(600)}, }, now: at(10), - want: automation, + want: work, }, { - name: "the most recently limited account is checked last", + name: "the most recently limited account has reset", limits: []Limit{ - {Account: automation, At: at(0), Until: at(600)}, - {Account: defaultAccount, At: at(5), Until: at(600)}, - {Account: work, At: at(10), Until: at(20)}, + {Account: work, At: at(0), Until: at(600)}, + {Account: automation, At: at(10), Until: at(20)}, }, now: at(30), - want: work, + want: automation, }, { - name: "limits recorded out of order", + name: "an earlier account reset while a later one is still limited", limits: []Limit{ - {Account: automation, At: at(10), Until: at(20)}, - {Account: work, At: at(0), Until: at(600)}, + {Account: automation, At: at(0), Until: at(20)}, + {Account: defaultAccount, At: at(10), Until: at(600)}, }, now: at(30), - want: defaultAccount, + want: automation, }, } { t.Run(tc.name, func(t *testing.T) { @@ -202,19 +194,19 @@ func TestPickRotatesFromTheMostRecentlyLimitedAccount(t *testing.T) { } // config.toml can drop an account that docket recorded a limit for. That limit -// must neither move the rotation nor offer a reset for an account docket no -// longer runs. +// must neither block an account docket runs nor offer a reset for an account +// docket no longer runs. func TestPickIgnoresLimitsOnAccountsOutsideTheRotation(t *testing.T) { accounts := []string{automation, defaultAccount, work} t.Run("for the order", func(t *testing.T) { limits := []Limit{ - {Account: defaultAccount, At: at(0), Until: at(10)}, + {Account: automation, At: at(0), Until: at(600)}, {Account: personal, At: at(20), Until: at(600)}, } - if got := Pick(accounts, nil, limits, at(30)); got != (Choice{Account: work}) { - t.Errorf("pick = %+v, want work, after the default account that was limited last", got) + if got := Pick(accounts, nil, limits, at(30)); got != (Choice{Account: defaultAccount}) { + t.Errorf("pick = %+v, want the default account, the first one under its limits", got) } }) @@ -483,9 +475,9 @@ func TestALimitMatchesByDirWhenEitherLoginIsUnknown(t *testing.T) { } } -// The rotation starts past every dir that the most recent limit applies to. docket -// therefore stays on the dir it moved to after that limit resets. -func TestPickStaysOffALoginAfterItsLimitResets(t *testing.T) { +// Once a login's limit resets, the first dir signed in to it is free again, +// wherever the other login sits in the order. +func TestPickReturnsToALoginAfterItsLimitResets(t *testing.T) { for _, tc := range []struct { name string accounts []string @@ -498,26 +490,26 @@ func TestPickStaysOffALoginAfterItsLimitResets(t *testing.T) { logins := map[string]Login{automation: team, defaultAccount: team, personal: ownOrg} limits := []Limit{{Account: defaultAccount, Login: team, At: at(0), Until: at(10)}} - if got := Pick(tc.accounts, logins, limits, at(30)); got != (Choice{Account: personal}) { - t.Errorf("pick = %+v, want personal, which never hit a limit", got) + if got := Pick(tc.accounts, logins, limits, at(30)); got != (Choice{Account: tc.accounts[0]}) { + t.Errorf("pick = %+v, want %q, the first account", got, tc.accounts[0]) } }) } } // A limit on a dir that the user has since signed in to another login applies -// to no dir in the rotation. It must not move the rotation. +// to no dir in the rotation. It must not block that dir. func TestPickIgnoresALimitThatAppliesToNoDirInTheRotation(t *testing.T) { accounts := []string{automation, personal} logins := map[string]Login{automation: team, personal: ownOrg} other := Login{AccountUUID: "ef2621bd-2fb9-4866-8453-6b8e4875b115", OrganizationUUID: "2bb69dd2-45b3-40ef-80fe-d40827cc4cd5"} limits := []Limit{ - {Account: automation, Login: team, At: at(0), Until: at(10)}, + {Account: automation, Login: team, At: at(0), Until: at(600)}, {Account: personal, Login: other, At: at(20), Until: at(600)}, } if got := Pick(accounts, logins, limits, at(30)); got != (Choice{Account: personal}) { - t.Errorf("pick = %+v, want personal, after automation, which was limited last", got) + t.Errorf("pick = %+v, want personal, whose old login's limit is not its own", got) } } diff --git a/internal/core/config/config.go b/internal/core/config/config.go index 0ee11a9..8549095 100644 --- a/internal/core/config/config.go +++ b/internal/core/config/config.go @@ -46,8 +46,8 @@ type Config struct { // therefore turns ~/.claude in the file into "". It keeps a CLAUDE_CONFIG_DIR // that docket inherited as given, because claude reads that value as given. ClaudeConfigDir string `toml:"claude_config_dir"` - // ClaudeFallbackConfigDirs are the accounts a new review moves to once - // claude refuses a session for a usage limit, in the order docket tries + // ClaudeFallbackConfigDirs are the accounts a new review runs under while + // every account before it is over a usage limit, in the order docket tries // them. Load turns ~/.claude into "" here too. ClaudeFallbackConfigDirs []string `toml:"claude_fallback_config_dirs"` // ClaudeDefaultDir is DefaultClaudeConfigDir with the tilde expanded. Load @@ -157,8 +157,8 @@ func (c Config) claudeDir(configDir, name string) string { return filepath.Join(dir, name) } -// ClaudeAccounts are the accounts docket rotates new claude sessions through: -// ClaudeConfigDir first, then each fallback, each account once. +// ClaudeAccounts are the accounts new claude sessions run under, in priority +// order: ClaudeConfigDir first, then each fallback, each account once. func (c Config) ClaudeAccounts() []string { accounts := []string{c.ClaudeConfigDir} for _, dir := range c.ClaudeFallbackConfigDirs { diff --git a/internal/core/session/limit_test.go b/internal/core/session/limit_test.go index 860dab4..34eee86 100644 --- a/internal/core/session/limit_test.go +++ b/internal/core/session/limit_test.go @@ -119,17 +119,21 @@ func TestAPollStartsAReviewRefusedForUsageAgainUnderTheFallbackAccount(t *testin } } -// docket stays on the fallback after the poll moves a review there, until the -// fallback hits a limit of its own. -func TestANewReviewRunsUnderTheAccountTheRotationMovedTo(t *testing.T) { +// A new review runs under the fallback while the primary is over its limit, +// and under the primary again once that limit resets. +func TestANewReviewReturnsToThePrimaryOnceItsLimitResets(t *testing.T) { r := refusedForUsage(t) r.poll(t) - r.svc.Now = func() time.Time { return r.resets.Add(time.Hour) } - rec := prepareWith(t, r.svc, pr.Ref{Org: "haacked", Repo: "docket", Number: 8}, "claude", review.IntentReview) + during := prepareWith(t, r.svc, pr.Ref{Org: "haacked", Repo: "docket", Number: 8}, "claude", review.IntentReview) + if during.ClaudeConfigDir != r.fallback { + t.Errorf("account = %q, want the fallback %q while the primary is limited", during.ClaudeConfigDir, r.fallback) + } - if rec.ClaudeConfigDir != r.fallback { - t.Errorf("account = %q, want the fallback %q, which has not hit a limit", rec.ClaudeConfigDir, r.fallback) + r.svc.Now = func() time.Time { return r.resets } + after := prepareWith(t, r.svc, pr.Ref{Org: "haacked", Repo: "docket", Number: 9}, "claude", review.IntentReview) + if after.ClaudeConfigDir != r.primary { + t.Errorf("account = %q, want the primary %q once its limit reset", after.ClaudeConfigDir, r.primary) } }