Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This security review couldn’t start because the paying account has exhausted its Codex allowance and has insufficient available credits. The payer can check their balance and add credits in the Codex usage dashboard, then try again. If you do not manage the paying account, contact this repository's admins. |
Mearman
force-pushed
the
feat/identity-name-decoupled
branch
10 times, most recently
from
November 4, 2026 11:57
0f402fe to
d91c853
Compare
Member
Author
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…its name An identity's name is the name in its identity.json, and its directory is a storage id fixed when it is created: the name itself, or the first free <name>-<n> when something is already filed under that name. Every lookup by name goes through one resolver over the identity records, which refuses a name two identities claim and finds an identity whose directory a resync left displaced in a superseded farm. Everything on disk or held by a running session is keyed by the id: CLAUDE_CONFIG_DIR, the identity header, the farm and its lock, launch records, the token file, the cached credential, the usage snapshot and the usage log records. Pools, rules, jobs, model refs, the active selection, picks and anchor stamps keep naming the identity, and reports show the name read from the record of the id. doctor fails for a name two identities claim and warns about a displaced identity, and identity show and list report each identity's id. BREAKING CHANGE: collectSessionList, locateSession and showSession take a function from an identity's name to its configuration directory in place of the identities directory, since an identity's directory is named by its id, not its name. mintRcSelfHostCredential resolves deps.identity as a name to its id, so its deps.fs needs readdir as well.
…hind --move identity rename <old> <new> now rewrites the name in identity.json and every reference to it (pools with nested and conditional members, jobs, account model refs, profiles, directory rules, the active selection, picks and anchor stamps) and nothing filed under the id. It takes no Keychain action and goes ahead while sessions run on the identity, since they keep their directory, their login and their usage key. It holds the identity's resync lock, refuses beside an unfinished --move of either name, rolls back on a failed step or a signal, and rewrites the name last, so a run killed part way is finished by running it again. The physical rename stays as identity rename --move, with its refusals, journal and Keychain copy unchanged; it reasons about directories, and refuses an identity whose directory does not carry its name. --discard-journal is given with --move, and the commands the journal and its refusals print name --move. The reference-rewriting steps both renames share live in identityRenameSteps.ts, and the table of state locations says which hold a name, which an id, and which neither. BREAKING CHANGE: identity rename <old> <new> no longer moves the identity's directory or copies its macOS Keychain login; it changes the name and its references only, and the directory move is identity rename <old> <new> --move. identity rename <old> <new> --discard-journal is now a usage error and is given as --move --discard-journal.
…name alone The configuration model explains which state is keyed by an identity's name and which by its id, how a new identity's id is chosen and how a name is resolved, and documents identity rename without --move beside the physical rename. The README, CLI reference, architecture, library and testing pages follow.
The supervisor took the member's name from the launch decision and watched, checked, compacted and registered under it, while snapshots and usage records are filed by id: a member whose id differs from its name never woke on its own quota, and a rename left a running session pointing at a name nothing had. The launch decision now carries the identity id, and the supervisor, its registry, moves and the door's model-move requests key by it; the name is read fresh only to relaunch the member and to say which it is.
A record whose name could not be read took its directory's name, so a damaged file in a directory whose name another identity now holds made that healthy identity a duplicate. Such a record now claims no name and is reached by its id only when no record holds that name, so addressing it still reports the file.
…s id An identity whose name cannot be read claims none, so the supervisor and the identity mover name it by its id, which still reaches it, and the launch decision test expects the identity id the decision now carries.
A rename locked only its source id and identity add took no lock, so two operations could both find a name free and leave two records claiming it, and an identity could be removed while a rename of it applied, which then reported success. identity add, identity remove and both renames now take one identity names lock before any resync lock and check the names again under it; the front door takes it without waiting and answers SERVICE_UNAVAILABLE while it is held. A rename whose identity.json has gone by the time the name is written fails and rolls back.
… one identity The remedy for two identities claiming one name was to rename one, which the rename refused because the name was ambiguous, and a symlink beside an identity's directory read the same identity.json and so made its own name a duplicate. identity rename --id takes the old identity by its id, and leaves references to a name another identity also claims for the one that keeps it. Entries that are one directory on disk are one record, the entry whose id is its name, listed once.
…n write The keeper read the stamps, awaited the window opener and wrote back the whole map under the names it listed first, so a rename during the await was overwritten and the renamed identity armed afresh, skipping an anchor that fell meanwhile. Stamps and the snapshot the keeper reads are now keyed by id, which a rename of the name keeps, the window still opens under the name, and the keeper writes only the stamps it changed over what is on disk when its run ends. Only --move rekeys a stamp.
… on the remote host Identity ids are local to each host, and push wrote the remote cache entry under this host's id, so it could land on another identity there. The remote script now finds the id by reading the identity names under the remote root, refuses when the name is absent or claimed twice there, and standard input is read through the command's ports.
…ng-quota events The events carried the stored id where the usage routes label identities by name, and account show named the view by name and its nested snapshot by id. Both events now carry id and, while an identity is stored under it, name, with UsageChangeEventSchema describing the usage payload, and the nested snapshot is named by the identity's name. BREAKING CHANGE: the quota-expiring event names its identity by id and name in place of the identity field, and createQuotaExpiringPublisher takes a nameOf dependency. The exported UsageChangeEvent is now the type of UsageChangeEventSchema, a usage snapshot with a required id and an optional name, in place of UsageSnapshot itself.
… id that has some identity remove left the token, cached credential and usage snapshot filed under the removed identity's id, and a new identity could be given that id and inherit them. Remove now deletes them, the CLI and the door also forget a credential the Keychain store caches under the id, and an id with any leftover such file or usage log record is not given to a new identity.
… killed part way A killed rename left references naming the new name with the identity still under the old one, and identity add of the new name adopted them. The rename now keeps a record under identity-name-renames/ while it applies; identity add of the new name (the CLI and the door, as CONFLICT), identity remove of the identity and another rename of either name refuse while it is there, naming the references already rewritten and the command that finishes the rename.
… case-only refusal on disk The --move refusal for an identity renamed without it, and the door's account-move request, which the door's wiring now makes through requestAccountMove, had no test. The case-only Work refusal now runs through the real ports where the filesystem ignores case, beside the faked symlinked alias.
… by identity id Comments, the --move success line and note, and the configuration model still said the usage log's past records and the snapshot file are kept under the identity's name, and that a name counts as an identity when it has an identity.json.
The remote push script refused an id only for a character outside ASCII letters, digits and . _ @ -, so an id renamed by hand to start with a hyphen took the entry while every other command on that host refused the identity. Its check is now built from the same character classes and length as isFileName, and a test runs it on a table of ids against isFileName. The refusal and the docs say the id must start with an ASCII letter or digit. BREAKING CHANGE: credential push of an identity refuses, writing nothing, when no identity on the remote host has the name (create it there with identity add), when more than one claims it (give all but one a new name there with identity rename <id> <new name> --id), or when its id there is not an ASCII letter or digit followed by ASCII letters, digits and . _ @ - (rename its directory there by hand, as the message says), where it wrote the entry under the name regardless. This corrects the earlier footers, which did not limit the refusals to an identity push, said letters for ASCII letters, gave one remedy for all three and allowed an id that starts with a dot or a hyphen.
The configuration model gave a remedy only for an unusable id; it now names identity add for a missing name and identity rename --id for a claimed one, as the messages and the command reference do.
The comment said the probe reads identitiesDir/<name> and that the path is the Keychain account; the directory is named by the id, and its path is what the Keychain item's service name is derived from.
The credential cache test description named a write, an asynchronous write and a removal twice.
Two remote ids such as a@b and a_b name one cache file, and the remote push script wrote the entry without reading whose entry the file held, so a push for one replaced the other identity's cached credential. The script now refuses, writing nothing, while the file records any owner but the id's own, and prints the id and that owner, which the message uses to name the file. An entry with no owner, the id's own entry or a file that is not an entry is overwritten.
…the host The advice for an id a push cannot use named a free id by the identities directory alone, which missed an id taken in another case (mv then moves the directory inside that one) and an id with a leftover token file (identity add does not give it out either). Every such refusal now names the id the remote host reported, says to rename the directory with mv -n to an id no entry under identities/ or credentials/ uses ignoring case, and to check identity list there before pushing again; the refusal for no identity of the name says not to add one while identity list shows it. BREAKING CHANGE: credential push of an identity also refuses, writing nothing, when the cache file of its id on the remote host already holds a credential recorded for another identity, whose id names the same file. For an id the push cannot write for, the remedy is now to rename the directory by hand to an unused plain id with mv -n, check identity list there, and push again. This corrects the earlier footers, which named no refusal for another identity's cache entry and said to rename the directory to a free id without saying how.
…he host Renaming an identity's directory to an id a push can use leaves the credential cached and the token stored under its old id, which no command there removes. The refusal now says so and names both files exactly: the cache file as the store names it for the old id, and credentials/<old id>.token.
…ential The credential resolution put "agent-shim: " into its message, and every command that throws it as a CliError printed the prefix again, as did a catalogue refresh, while the front door and the models-block check embedded or stripped it. The resolution now has no prefix: the launch's identity and provider refusals add it, the models-block check uses the message as is, the front door's 502 gives it after its own text, and credential warm, credential push and a catalogue refresh print it once.
The Keychain delete command and the supervise hooks each quoted shell words with their own copy; both now use shellQuoted from src/shellQuote.ts.
… after a marker OpenSSH runs a remote command in the account's login shell, and zsh, the macOS default, refuses the unmatched glob of a host with no identities directory, so such a push failed as an ssh error instead of the refusal for no identity of the name; a login shell's start-up output also ran into the fields a refusal reads. The command now hands the script to /bin/sh, and the script prints a refusal's fields after a marker, separated by NUL, which the push reads from the last marker on.
… on the host The refusals for an id a push cannot write for, and for a cache file holding another owner's entry, told the user to rename directories and delete files by hand, which could lose another identity's credential or a token file an identity still reads. Both now give one route that runs only agent-shim there: rename that identity aside by name, identity add the name, which picks an id a push writes for, and push again; the identity renamed aside keeps everything it had. The file a refusal names is given under the host's state root, which the script now replies with. BREAKING CHANGE: credential push of an identity refuses, writing nothing, when its id on the remote host is not an ASCII letter or digit followed by ASCII letters, digits and . _ @ - within the longest a name may be, or when the cache file of that id already holds a credential recorded for any other owner (another identity whose id names the same file, or an owner that is not a string). For both the remedy is now: identity rename <name> <other name> there, identity add <name> there, and push again. This corrects the earlier footers, which gave a remedy of renaming the directory by hand with mv -n and deleting the old id's files, and said the cache-file refusal was only for another identity whose id names the same file.
Two provider names such as a@b and a_b name one cache file, and a provider push wrote it without reading whose entry it held, so it replaced the other provider's credential. The provider push now runs the same owner check as an identity's: it refuses, writing nothing, while the file records any owner but the pushed provider, and says to rename the other provider there with provider rename, which moves its credential to its new name's file, or, for an owner that is not a string, to remove that entry with credential forget --provider. BREAKING CHANGE: credential push --provider refuses, writing nothing, when the provider's cache file on the remote host already holds a credential recorded for another owner, where it wrote the entry regardless.
…ivate Only the push in the same module builds the remote command, so nothing exports it.
…d-back resume Keep an existing unfinished `NameRenameRecord` on disk when a resuming `identity rename` is interrupted or rolls back a failed step, match the record's `id` in `requireNoInterruptedNameRename`, refresh locks only between steps in `renameIdentity` so a completed final name rewrite is never rolled back, and require unshared references to the old name to be gone before treating a record as settled.
…atusline and rename --id Validate identity directory ids in `readAccountMetadata`, `createAccountReader`, `snapshotPath`, `listUsageSnapshots`, `createUsageStore` and `statuslineSettingsForRender` with `isIdentityId` (`!id.startsWith(\".\") && isStoredName(id)`), matching `readIdentityRecords`, and allow `<old>` to start with `-` when `identity rename` is called with `--id`.
… and missing providers Move file-cached provider credentials in `renameProvider` even when the provider has no `credential.cache` block or caches in the Keychain on darwin, delete a removed provider's own file-cached credential in `removeProvider`, remove entries no owner of that file can read in `createRealCredentialCache`, and allow `credential forget --provider` to clear an orphaned entry when the provider is not defined on the host.
…ured file cache Exempt a credential-cache file in `stateFiledUnder` during `identity rename --move` only when `createRealCredentialCache` reads it as the moving identity's own entry, compare cache file names case- insensitively in `isReadableByAnotherOwner`, and move an existing file- store entry on disk even when `credential.cache` is unset or uses the Keychain store.
…d frame its reply The script held a literal newline and three `!` characters, which csh and tcsh refuse or history-expand inside the quoted word before /bin/sh runs, so a push to such a host never ran. The newline now comes from printf, and the id and symlink checks use grep and an if branch. The reply marker is framed by a NUL on each side and read from its first frame, so an owner that ends with or equals the marker text is no longer taken for it, and a status 5 or 6 with no reply is reported as a failed ssh. completion.ts quotes with shellQuoted, and a doubled full stop in the configuration model is removed.
…and name its model refs Describing an interrupted rename read the pools, jobs and directory rules without guarding against a config.json, directory-rules.json or profile that is malformed, so doctor and the refusals that name the rename threw instead of reporting that file as its own finding. The collectDoctorReport read of the models block is guarded the same way. The references it lists now include the account model refs a rename rewrites in config.json, in a profile and in a directory rule.
credential store wrote credentials/<id>.token before setIdentityCredential found that the identity is displaced in a superseded farm or that its identity.json cannot be read, so a refused store had already replaced the token the identity points at. The identity is now resolved to a stored id and its identity.json read before the token is written.
…shed move holds identity add and identity remove refused a name that reaches the same directory as one an unfinished --move journal holds (a symlink, or another case on a filesystem that ignores case), but identity rename compared the journal with exact equality, so a rename to such a spelling went ahead beside the half-moved identity. unfinishedRenameFor now takes the same comparison, and both rename paths pass it.
…a case-sensitive disk Passing a spelling comparison to unfinishedRenameFor replaced exact equality, so on a filesystem that keeps case a rename of another identity to a name the journal holds, and the reverse of a half-moved pair, no longer found the journal and went ahead. An exact name now always counts as held, and the comparison only adds other spellings.
… for another identity The remote push script cut the directory and id from the path with dirname and basename in command substitution, which strips trailing newlines, so an id of work followed by a newline was read as work, another identity's directory, and its cache entry was overwritten. It also checked the id with grep, which reads line by line, so an id with a newline inside passed on each line and was written to a file with a raw newline in the owner. The path pieces now come from parameter expansion, the real paths seen are kept in the positional parameters, and the id is checked by a case pattern for its first character and by deleting every allowed character from the rest. Neither needs a negated bracket, which dash reads literally as ^ and csh and tcsh history-expand as !. A test runs the script under every installed POSIX shell.
…t remedies a shell can run A provider stored under a name provider add does not give (a quote, a "!", a space, a newline) was pushed with its name embedded in the remote script and in the cache file's owner. A quote made the owner check read the provider's own entry as another's, so the second push refused and its remedy did not help, and a "!" is history-expanded by a csh or tcsh login shell before the script runs. Such a push is now refused before anything is read or sent, naming the provider as JSON and saying to rename it here with provider rename. The owner check reads a value as a whole JSON string, escapes included, and replies with it escaped for the client to decode. The remedies print the other owner's provider name through shell quoting, and compare the two names' cache files with case folded, as the store does, so a provider Z that reads the entry of a pushed z is told to be renamed rather than that no provider reads it. When the escaped name cannot be turned into a file name there, the refusal says to check provider list there first. BREAKING CHANGE: credential push --provider refuses, writing nothing and without contacting the host, a provider whose name is not an ASCII letter or digit followed by ASCII letters, digits and . _ @ - within the longest a name may be, where it ran the remote script with the name in it; the remedy is provider rename <name> <new name> here.
…rt on rename identity rename --move read an entry left in the file store, and wrote it back, through createRealCredentialCache beside the injected port, so a test could not substitute it and the ports comment saying the library never imports the real cache was false. The entry is now read through the injected port, which already takes the store, and credentialCacheStep moves one entry per store it is given and undoes each the same way. Its doc comment says when the step applies, including when the credential block has no cache.
The usage and statusline readers validate a directory id, not an identity name, but their messages still called it a name and the statusline stated a rule that a leading-dot id satisfies. The shared id validation now names the stored-name or leading-dot rule it rejects, and the usage log calls the header value an id.
…provider credential removal The configuration model and test map are brought into line with name-rename records kept by a resumed run, model references in their refusal description, either form of rename refusing a spelling an unfinished move holds, directory ids wider than names, file-cache cleanup on provider remove, and the credential push shell checks.
Mearman
force-pushed
the
feat/identity-name-decoupled
branch
from
November 4, 2026 12:29
d91c853 to
987bbc3
Compare
Member
Author
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #679.
An identity's name is now the
namein itsidentity.json, and its directory is a storage id fixed when it is created: the name itself, or the first free<name>-<n>when something is already filed under that name (a renamed identity's old directory, a lock, a stray file). Every lookup by name goes through one resolver inidentityRecords.ts, which reads the records, refuses a name two identities claim, and still finds an identity whose directory a resync left in a superseded farm so recovery can put it back.Anything on disk or held by a running session is keyed by the id:
CLAUDE_CONFIG_DIR, the identity header a session sends the door, the farm, its lock and launch records,credentials/<id>.token, the cached credential (identity <id>, with the subject in messages still naming the identity), the usage snapshot and so the usage log records, since the door takes those from the header. That made keying usage by id the cheap option, and the "records keep the old name" warning only applies to--movenow. Pools, rules, jobs,acct:refs, the active selection, picks and anchor stamps stay keyed by name, and reports (usage,account show, the statusline, the door's usage API) show the name read for the id.identity rename <old> <new>now rewrites only the name and those references, reusing the rewrite steps from #678 (moved toidentityRenameSteps.ts). No Keychain call, no session check, no journal: it takes the identity names lock and then the identity's resync lock, keeps a small record underidentity-name-renames/while it applies, rolls back on failure or a signal, and writes the name last so a killed run finishes when run again. The physical rename is--move, unchanged apart from reasoning about directories rather than names and refusing an identity whose directory does not carry its name;--discard-journalgoes with--move.Existing identities need no migration:
identity addhas always writtennameequal to the directory, and the schema has always required it.doctorfails for a name two identities claim and warns about a displaced identity, and does not care that a name differs from its directory.identity show --jsongivesidbesidedirectory.After review, a few more pieces landed on top. The supervisor, its registry and the door's model-move requests key by id too, so a supervised session keeps working across a rename. One identity-names lock serialises
identity add,identity removeand both renames, and a rename of the name alone keeps a small record while it applies, soidentity addof the target refuses after a killed run instead of adopting its references. An unreadableidentity.jsonclaims no name, a symlink beside an identity is the same identity, andidentity rename <id> <new> --idresolves a name two identities claim.identity removedeletes the token, cached credential and snapshot filed under the id, and an id with any of them left, or usage log records, is not reused. Window keeper stamps are filed by id,credential pushfinds the id on the remote host by name, and the usage and quota-expiring events carryidandname.A second round of review tightened the edges. The door reads configuration uncached, so a long-lived door or supervisor sees a rename at once (a provider request costs two small file reads, which a test counts), and it names the identity in each usage event through a per-id read of
identity.jsonvalidated by size, mtime and inode. The record of a name rename blocks--moveof either name, is named by a hash of the pair soa--b/canda/b--cdo not collide and long names fit, and survives a rollback that could not undo a step. Choosing a new identity's id also asks the Keychain store, by attributes only, whether it still caches a credential for that id, reads the usage log once per pass, and the identity is created under the id its checks were made for, refusing if that id was taken in between. Removing a cached credential from the Keychain now reports asecurityfailure with the command that deletes the entry instead of ignoring it. Usage left under an id no identity holds any more is labelled<id> (gone)so it never merges into a later identity of the same name, and a busy names lock says an add, remove or rename holds it rather than a resync.A third round made the name-rename record exact about its file: a record is read only from the file its rename keeps it in and removed from the file it was read from, a settled record never blocks a name, and
doctorwarns about a record under any other file name. Removing an identity whose cached credential the Keychain will not delete now says the identity was removed and names the command that deletes the entry. The door reads the usage log once for an add however often the Keychain's answers restart the check, a pool pick resolves its members from one read of the identity records, a credential push picks the same twin on the remote host as locally whatever the locale, and the usage and expiring-quota events name an identity through one reader, a resync left displaced included.addIdentitygains an optional options object (namesLock,checkedId), which every released caller keeps working with; called alone it does not ask the Keychain, which its docs now say.A fourth round made the dry runs and the pre-add check leave settled name-rename records for the real add, remove or rename to clear, made the twin push test run under bash in a collating locale so it can fail on a byte-order sh (and checks the script under dash too), keeps the door's answer for an id with no
identity.jsonuntil the identities directory changes, and adds read counts for a pool launch and the picker.Breaking:
identity renamewithout--moveno longer moves the directory or copies the login,--discard-journalneeds--move, the quota-expiring event'sidentityfield is nowidandnameandUsageChangeEventrequiresid,mintRcSelfHostCredentialresolves its identity by name and needsreaddir, and the session tools take a name-to-directory function. The commits are marked accordingly.