fix(models): key team values by repo identity, migrating legacy slug names (#894) - #895
Merged
Merged
Conversation
scs0209
force-pushed
the
fix/team-values-hash-894
branch
from
September 29, 2026 01:32
eb4b302 to
f60428f
Compare
|
…nt#894) - never migrate the legacy values file in a dry run: `pull --dry-run` and `models switch --dry-run` thread { dryRun } into migrateTeamValuesPath - match the legacy digest the old implementation actually keyed on — the repo: claim in teamai.yaml overrode the identity there, so an SSH remote beside an HTTPS repo: no longer orphans keys and switches - when several legacy files share a digest, migrate the newest one, so keys re-entered after a team rename win over stale copies - sync docs/designs/model-profiles.zh-CN.md with the hash-only file name 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Earlier findings concerning mutation during dry-run, |
…ce-safe link - dry run returns the newest legacy file's path so it reads exactly the keys the real run would migrate and read, instead of reporting a key missing from a not-yet-existing hash-only file - a bare remote alias (`fork`) no longer names the values file: two checkouts sharing the alias hashed to one file and could read each other's keys, so hashing falls back to the URL, then the local path - migration links the legacy file to the target instead of renaming: a concurrent migration that links first makes ours fail with EEXIST, so stale keys can never overwrite fresh ones on POSIX 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Findings
Resolved
|
…inks fail - the legacy local-path digest is a candidate only when the old implementation would have keyed on the path itself (no repo: claim, no configured remote, no URL): a default-path checkout re-initialized for another team with a URL no longer adopts the previous team keys - when a filesystem rejects hard links (EPERM, ENOTSUP, ...), migration falls back to an exclusive, never-overwriting copy (open wx mode, partial writes removed) instead of skipping every candidate and returning a path with no file behind it 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Finding
Resolved
|
The copy fallback wrote the target in place after an exclusive open, so a concurrent reader could observe the empty or partially written values file and fail to parse it. The content is now written to a unique temp file and linked into place: until the publish link succeeds the target does not exist, so a concurrent reader sees either the complete previous file or none, and two migrations still cannot overwrite each other (the loser publish link fails and its temp file is removed). 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Finding
Resolved
|
Rename-and-migrate had to be atomic, no-clobber, link-optional, and partial-write-free all at once, and each fix round surfaced another way the migration could strand or clobber a secrets file. Drop the machine: while the hash-only file does not exist, findTeamValuesPath returns the newest legacy file and it is read where it lies; the next save writes the hash-only name, which then shadows the legacy file. Reads no longer mutate anything — a dry run needs no special casing — and no link, rename, or copy stands between the keys and their reader. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Finding
Resolved
|
Two checkouts that shared a bare non-origin alias (`fork`) hashed the same legacy digest, so findTeamValuesPath could read one team's values for the other. A repository-bound digest (URL, repo: claim, local path) still matches by digest alone, since the slug drifts on team renames; a file keyed by an alias digest is adopted only when its slug is this checkout's too, the discriminator the old scheme kept those files apart by. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Findings
Resolved
|
…old precedence - sameTeamIdentity applied the alias slug check only when reading a values file, not when matching a switch record: two teams sharing remote fork could treat each other managed switches as their own. The slug requirement now covers every non-repository-bound digest. - legacyTeamValueHashes mirrored the old precedence too loosely: with a repo claim present, the old implementation hashed only the claim, so the remote, URL, and path candidates never applied. A replaced checkout at the same path can no longer adopt a path-digest record the old code would never have produced for it. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Finding
Resolved
|
…s file name A config with remote origin and no repo.url hashed the checkout path, so reusing that path for another team shared the first team's hash-only values file and its identity: team B read team A's API keys and claimed its managed switches. The claim is the remaining repository identity before the path, so it now wins whenever the remote and the URL name no repository. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Finding
Resolved
|
A path-shaped identity like owner/repo names a different repository on each provider, so two teams with the same repo value on GitHub and GitCode hashed to one values file and one identity: opening team B read team A keys and accepted its managed-switch identity. The provider now qualifies path-shaped identities; host-bearing ones (URLs) already carry the host and stay as they are. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Findings
Resolved
|
…igests - the values file identity qualified a path-shaped source with the local provider override, normally absent, so two teams declaring the same repo: value on GitHub and GitCode still hashed to one file; the teamai.yaml provider now wins for its own claim, and the local override applies only when the claim carries none - legacy candidates marked a path-shaped claim digest repository-bound, so sameTeamIdentity adopted another team's switches and findTeamValuesPath read its file under the matching digest; a URL-shaped claim stays repository-bound while a path-shaped one is matched under this team's slug, like an alias - also fixes the candidate guard inverted while landing the above: with a claim present the remote, URL, and path candidates were dropped, and without one they were duplicated 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Findings
Review Status
|
…ly records Path-shaped repo claims now adopt their legacy values files by digest alone, so a team rename no longer orphans its keys; the slug check moves to switch-record matching, where a claim digest names no single repository. Path-only configs mix the team slug into the hash-only identity, so a checkout path reused by a differently named team can no longer read the previous teams keys. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…ages 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Findings
Review Status
|
…es by gateway origin A legacy file's digest hashes the bare path-shaped claim name with no provider, so GitHub team Alpha and GitCode team Beta can produce the same old digest. The previous fix read such files by digest across renames, letting the second provider adopt the first's file and API key; slug-guarding records instead orphaned a renamed team's own switches. Gate both on the one evidence that survives both a rename and a provider change: the gateway origin each stored key is bound to under `<team:<id>@<origin>`. Path-shaped claim digests now require this team's slug for files and switch records, and are re-admitted under a different slug only when the legacy file provably stores keys bound to this team's gateways — which a rename keeps and another provider cannot claim. Keys still in a beta-era `team:<id>` form (no origin) are not evidence, so their files are left unread rather than guessed at. Callers thread the resolved team profiles through `findTeamValuesPath`, `sameTeamIdentity`, `switchedGatewayOrigins`, `activeAgentsFor`, and the pull re-apply.
|
Findings
Review Status
|
…wn switch history Gateway-origin key binding is not a team identity: two teams sharing a profile id and gateway URL behind the same path-shaped claim would pass the provenance check, letting the second read the first's file and claim its switches. But refusing differ-slug files entirely orphans a renamed team, whose legacy `<slug>-<hash>.json` carries its own old slug. The trustworthy, machine-local evidence is managed.json: this checkout records the exact identity each past `team:` switch used. A differ-slug provider-ambiguous file or switch record is re-admitted only when that identity appears in this checkout's own switch history — a renamed team's file, recognized without trusting a gateway a foreign team could share, a key binding another team's file may also carry, or a slug a rename just moved. Foreign teams hold only their own slug in managed.json and are refused; beta-era unbound keys need no binding to be recognized this way. Provider-relative remotes (`owner/repo`) also get the provider qualification already given to path-shaped claims, so two providers using the same checkout path and slug no longer share one values file. Design docs synced in both languages.
|
Finding
Review Status
|
…exists A past adoption and migration is the permanent record: the provider-qualified hash-only target shadows every legacy name forever, so unadoptedLegacyFiles returns no candidates when getTeamValuesPath exists. Without this, the in-memory adoption set (which resets per invocation) combined with the intentionally-kept legacy file re-prompted on every later interactive command and misled non-interactive/--dry-run runs with a stale warning. Test: after migration the file is shadowed and never offered again.
|
Findings
Review Status
|
… config
Two review findings on the adoption layer:
1. adoptedLegacyValues was global, keyed only by the legacy basename. In a
pull with inheritUserScope, user scope runs first; a same-slug, same-claim
team on a DIFFERENT provider resolves to the same adoption key, so
confirming the user-scope file also silently authorized the project scope
to read and migrate that key in its own provider space. Adoption is now
keyed `${target}::<slug>-<digest>` — the adopting scope's own
provider-qualified file path — so a confirmation scopes to exactly that
team identity, and any other scope/provider must opt in separately.
2. adoptMigrated was a process-global boolean ("any adoption happened"). After
adopting a user-scope file, declining a project-scope candidate still set
it true, so the project loaded {} from its nonexistent target and wrote an
empty hash-only file, permanently shadowing its legacy keys and killing
future prompts. The write is now decided per config: only when readFrom
differs from the target (a real legacy read) is the migration saved; a
declined candidate is read nothing, written nothing, and re-offered next
run.
Tests: cross-provider scope regression (GitHub adoption under its target never
admits the file for the GitCode scope; same-scope adoption of the same
basename does), namespaced adoption keys everywhere, migration-shadow check.
|
Findings
Review Status
|
…aim ambiguous records by slug
Two review findings:
1. A non-interactive `models configure team:<id> --from-env X` reads {} past an
unadopted legacy file, then writes ONLY the configured key to the
hash-only target — permanently shadowing the legacy file (the target's
existence silences every future adoption prompt) and orphaning its other
API keys without a warning anyone can act on. configure and switch now
refuse the write while unadopted legacy files remain, naming the
interactive adoption path. A pull is unaffected: its read-time save is the
migration itself and already requires a real legacy read.
2. A same-slug provider-ambiguous switch record matched Team B's checkout. The
old name never encoded the provider, so a GitHub and a GitCode team both
named `Alpha` on the bare claim `acme/widgets` share the identical
`alpha-<digest>` form: with Team B holding its own hash-only key, pulling
B claimed an agent switched to Team A and overwrote it. A legacy record
under a provider-ambiguous digest never matches now — slug equality is not
ownership across providers, consistent with the differ-slug refusal.
Repository-bound records still match by digest (the host is in the
identity). Adoption of the values file re-establishes the team; its
switched agents are re-recorded by the next `models switch`.
Docs synced (en + zh-CN). Tests: path-only/alias/claim records all refused
even under the exact slug, cross-provider same-slug record regression, and
the 4 prior adoption-scoping expectations.
|
Findings
Review Status
|
…Windows drive as a URL
Two review findings on identity derivation:
1. A provider-relative remote never got its documented precedence.
isRepoReference('owner/repo') is false, so the selector picked the URL or
teamai.yaml claim instead; two checkouts with DIFFERENT remotes
(acme/team-a vs acme/team-b) but the same claim/provider therefore shared
one hash-only secrets file and could consume each other's keys. A
configured non-origin remote now names the repository with the highest
precedence whether URL-shaped or provider-relative (which names one repo
together with its provider); only a bare alias (no slash, no scheme) falls
through. Different remotes -> different files, always.
2. new URL('C:\\teams\\repo') accepts c: as a scheme, so Windows drive paths
were hashed as phantom URLs and a path-only config skipped the
provider-and-team-slug fallback; replacing that checkout with a
differently named team reused the file. URL parsing now rejects
single-letter schemes, keeping drive paths in the path form whose slug
separates teams at one checkout.
Tests: two different provider-relative remotes with a shared claim collide no
more; same remote+provider names one repo across paths; bare aliases and
drive paths never hash as URLs. Full suite 5285 passed.
|
Findings
Review Status
|
…es; bare aliases never key the file Two more identity findings: 1. A provider-relative remote ignored the provider declared in teamai.yaml when no local override existed, so GitHub and GitCode teams sharing `remote: owner/repo` both hashed `tgit:owner/repo`. The effective provider now consults the teamai.yaml `provider:` (via the claim or, when the claim is absent, directly) before the local override and the tgit default — the same qualification the path-shaped claim already applied. Provider survives a team rename, so the file stays bound to the repository. 2. A bare remote alias (`remote: fork`, no URL, no claim) was still hashed directly as `tgit:fork`, so two checkouts sharing the alias shared one hash-only secrets file. A bare alias names no repository, so it now falls through to the provider/slug/path identity, whose slug keeps teams sharing a checkout path apart. Legacy `<slug>-<digest>` files for alias-keyed checkouts remain migration candidates. Tests: each checkout declares its own teamai.yaml provider and stays distinct; a bare alias no longer changes the file over the path-only form and two teams sharing it stay separated by the slug. Full suite 5285 passed.
|
Finding
Review Status
|
…doption merges all adopted keys Two defects in the read-time migration: 1. Declining one of several matching provider-ambiguous legacy files then saving the hash-only target stranded the declined file forever: the very existence of the target silences every future adoption candidate, so keys unique to the declined file became unreachable with no further prompt possible. The migration save now happens only when NO candidate remains for this team (adopted or gone); a declined file stays a candidate, keeps its keys reachable, and is re-offered on the next run. 2. A single-file read kept only the newest adopted identity's keys. When several files shared this checkout's digest, adopting all of them still dropped every non-newest identity's unique keys before the target shadowed them. Adoption now merges the keys of every adopted identity into the migrated target (new mergeModelInputs; the later map wins). Tests: with two matching files, adopting one still surfaces the other to the migration guard; adopting both leaves no candidates and the merge keeps both identities' keys. Full suite 5286 passed.
|
Findings
Review Status
|
…le and no longer block migration
Two review findings, one shared root: the migration's consent model could not
distinguish "not yet decided" from "deliberately declined".
1. The effective provider put teamai.yaml's provider before the member's own,
so a team declaring GitHub with the same bare claim made a member
initialized with --provider gitlab share the GitHub team's hash-only file.
The member's own provider now wins (localConfig.provider ?? team provider ??
tgit), as everywhere else in the CLI.
2. Requiring every same-digest file to be adopted blocked migration behind a
foreign team's file: declining GitHub Alpha to adopt GitCode Beta left an
unadopted candidate forever, so no target could ever be created. Declining
now records a durable, target-scoped ${target}::<slug>-<digest> refusal in
the models manifest. A declined identity is neither re-offered on every run
(no re-prompt loop) nor silently shadowed when migration proceeds (the
choice is explicit, visible state, reversible by removing the key or the
file) and does not count toward the migration guard — adopting only your
own team's file no longer forces you to merge a foreign team's keys.
unadoptedLegacyFiles and findTeamValuesPath now take { adopted, declined };
findTeamValuesPath reads an adopted identity and skips a declined one.
Tests: found the regression itself (adopt one of two, decline the other leaves
no candidate and reads the adopted file only), the provider-override case, and
a refuseLegacyValue/refusedLegacyValueKeys manifest round-trip with a
fail-closed schema check. Full suite 5288 passed.
|
Finding
Review Status
|
…er by merging before save The migration write into the new hash-only target is the one place a values file is created from a different file, so it must not clobber a target a concurrent process wrote between the legacy read and the save: writeJsonAtomic prevents torn files, not lost updates. A pull reading the legacy file while a configure creates the target could otherwise overwrite the newly configured key with its stale snapshot. The migration now re-reads the target and merges before saving (the concurrent content wins collisions). The merge runs only in the migration branch (readFrom !== target). When the run merely re-saves the file it already read, the target holds the same content this bind just consumed — merging would re-inject the raw legacy entries the bind renamed, so there is nothing to guard against there. Full suite 5288 passed.
|
Finding
Review Status
|
…rite Re-reading the migration target and saving it were still two separate operations, so the race the review flagged only shrank: a pull could re-read the target, a concurrent configure could save a new key, and the pull could then overwrite it with its stale merged snapshot. Team values writers now share an advisory lock on the target file, and each of them runs its entire read-modify-write cycle inside it: - the migration's was already read-merge-save; it now runs that whole cycle under the lock instead of only the merge step; - models configure and the models switch first-use prompt re-read the target inside the lock, so their write cannot clobber a migration or a sibling configure that landed in the same window. withTeamValuesLock reuses the existing owner-verified acquireLock/releaseLock primitive (dead-owner stale locks are reclaimed), so a crashed holder does not block pulls or configures. Local values writes are untouched; no legacy migration can race them. Full suite 5288 passed.
|
Finding
Review Status
|
…ractive switch The lock round-trip saved the first-use key into the freshly re-read snapshot (current), but the rest of the command resolved the profile against the original in-memory values, which had no key yet — so the switch stored the key and then failed with "no API key", demanding a rerun. Apply the same key to the in-memory copy after the locked write, and add a regression test for the first interactive team switch (ask, save, and resolve in one run) so the behaviour cannot silently split again. Full suite 5289 passed.
|
Finding
Review Status
|
…itch Moving the getStoredApiKey update after the branch left the local path saving values.json without the key the prompt returned: the switch succeeded from the later in-memory copy, but the next invocation found values.json missing the key and reported it not configured. Put the key in the in-memory copy before the branch, so the local save already carries it; regression tests now cover the first interactive switch for both the personal and the team profile. Full suite 5290 passed.
|
Review Status
|
jeff-r2026
approved these changes
Sep 29, 2026
This was referenced Sep 29, 2026
Author
|
Thanks for the merge! |
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
2 participants
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.
Summary
Team-profile API keys live in
~/.teamai/models/teams/<name>.json. The old file name combined a slug of theteamai.yamlteam:value with a hash of the repository identity, so renaming the team orphaned the keys and the name was never a stable binding to a repository. This PR names the file after the repository identity alone and reads legacy files in place.getTeamValuesPathresolves the repository identity in strict order — a non-origin/upstream remote that names a repository, thenrepo.url, then therepo:claim, then the local path — and hashes it viarepoIdentity(URL-shaped),provider:identity(path-shaped claim, provider from the claim, else the local override, else the team default), orprovider:slug:path(path-only, where no repository identity exists and the team slug keeps differently named teams sharing one checkout path apart).<slug>-<digest>.jsonfiles are read where they lie — never renamed, linked, or copied. A legacy file matches when its digest is one the old implementation could have produced from this checkout, mirroring the old single-identity precedence. Digests whose identity names a repository (a URL, a URL-shaped remote or claim, a path-shaped claim) match by digest alone; a bare-alias digest or a path-only digest additionally requires this checkout's slug, the discriminator the old scheme kept those files apart by. The next save writes the hash-only name, which shadows the legacy file.sameTeamIdentityapplies the same rules to switch records: digests that name a repository match by digest alone, so existing switches survive team renames; bare-alias and path-only digests additionally require the stored slug, so cross-team records are rejected.docs/designs/model-profiles.mdandmodel-profiles.zh-CN.mdare in sync.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpasses (0 warnings / 0 errors)npm testpasses (5280 passed / 1 skipped)npm run buildpassesnpx vitest run --config vitest.e2e.config.ts model-cli-switchpassesTests cover: hash-only naming stable across teamai.yaml renames; identity precedence (claim over path, provider qualification, alias exclusion, path-only slug hashing);
sameTeamIdentityaccepting only digests this checkout could have produced; legacy read-through (newest file wins, foreign digests ignored, alias files read only under this team's slug, a path-shaped claim file read by digest across renames, hash-only shadowing after the first save).Real-CLI verification (head
efd4ae5): sandbox$HOME, built CLI (dist/index.js), a bare local remote, a member config whose remote is an HTTPS URL while teamai.yaml carries a different canonicalrepo:claim, and a legacyhai-platform-<repo:-claim digest>.jsonvalues file:teamai pull --dry-run— legacy secrets file untouched, no hash-only file created, profile offered.teamai models list team:corp— reads the legacy key in place,API key: configured locally;models switch --dry-runlikewise, no rename.models switch— the unbound beta-era key is bound to the gateway origin, the hash-only file is created holding it, and the legacy file remains in place shadowed;models restoreclean.Related Issues
Fixes #894
Notes for Reviewers
repoIdentity, notnormalizeRepoUrlForCompare: the latter drops the port on purpose, while values files must distinguishhttps://host:8443/teamfromhttps://host/team.bindLegacyTeamKeys), and a foreign team key is only rewritten onto a profile id whose origin matches this team's.provider:slug:pathinto the name itself: a rename there still orphans keys (no repository identity exists to key on), but a checkout path reused by a differently named team can no longer read the previous team's file.writeJsonAtomic.