fix: stop one team adopting another team's Context Tree repo - #2122
Conversation
The Context Tree repo name was derived from the team display name alone. Display names are deliberately not unique, and `slugifyRepoBase` drops every character outside `[a-z0-9]`, so any name without an ASCII alphanumeric — any all-CJK team name — fell back to the same `"team"` base. Two such teams under one GitHub account derived one repo name. Provisioning then made that collision silent. Both storage backends adopt an existing repo on the deterministic name: the Organization path falls back to adoption on `422 already exists`, and the User path probes for the repo before creating anything. Adoption is meant to recover *this* team's half-created repo when a previous run wrote the repo but not the binding — initialization is gated on an unbound team, so that is the only case it can legitimately serve. With a colliding name it instead handed the second team the first team's tree, and both teams then read and wrote one Context Tree. Give the name a per-organization discriminator so the collision cannot happen, and check the binding table before adopting so it cannot go unnoticed if it does. The discriminator hashes the organization id rather than slicing it: ids are opaque text, and a uuidv7's leading bytes are a timestamp, so same-day organizations would share any prefix taken from one. The name stays stable for a given organization, so a retry still adopts its own repo. The derivation moves to the provisioner service, next to the adoption it constrains and where it can be tested directly. Existing bindings are unaffected by the new rule, so `scripts/audit-shared-context-tree-repos.ts` reports repos more than one team is already bound to. It is read-only and needs an owner decision per row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The new ownership guard compares raw URL strings and is not atomic, so supported equivalent repo URLs or concurrent bindings can still leave two teams on one Context Tree.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Two teams under one GitHub account can derive the same Context Tree repository name and silently adopt the same repository, exposing each team's durable context to the other.
- Approach: Add an organization-derived discriminator to generated repo names, reject an initializer target already bound elsewhere, and provide a read-only audit for existing shared bindings.
- Impacted modules: Context Tree initializer route, repository provisioner, organization settings, server tests, and an operator audit script.
Review findings
❌ 1. Compare canonical repository identity, not the stored URL spelling. context_tree.repo accepts HTTPS, ssh://, and scp-like SSH, with or without .git, while findOrgBoundToContextTreeRepo only lowercases the full string. Thus an existing binding such as git@github.com:acme/foo.git is not found when initialization checks https://github.com/acme/foo.git, and the initializer can still adopt it. The audit has the same gap because it groups by lower(repo), so it can report a clean result for two bindings to the same repository. Reuse the shared canonical Git repository identity and cover equivalent transports/suffixes in both the guard and audit. [R1, R5 / packages/server/src/services/org-settings.ts:437, packages/server/scripts/audit-shared-context-tree-repos.ts:41]
❌ 2. Enforce exclusivity atomically at the binding write boundary. The conflict lookup runs before repository provisioning and file writes, while putInitializedOrgContextTreeBinding later locks only the current organization. Two organizations targeting the same canonical repo can both observe no conflict, one can create while the other adopts, and both can then commit their own rows. The same invariant is also bypassed by the regular Settings binding writer. A canonical repo reservation/uniqueness guard must cover every Context Tree binding write in the same transaction (or use an equivalent cross-org serialization mechanism), with a concurrency regression test. [R4, R5 / packages/server/src/api/orgs/context-tree.ts:300, packages/server/src/services/org-settings.ts:485, packages/server/src/services/org-settings.ts:583]
✅ 3. The organization-derived hash avoids the display-name collision class while preserving the GitHub length budget and retry determinism for unchanged team coordinates. [packages/server/src/services/context-tree-repo-provisioner.ts:244]
Action taken
- Submitted request changes.
yuezengwu
left a comment
There was a problem hiding this comment.
The goal and the two main defenses are clear: make the derived repository name organization-specific, then refuse adoption when the derived repository is already bound elsewhere. I found one tenant-isolation blocker.
Blocker — compare canonical repository identities, not raw URL spellings. In packages/server/src/services/org-settings.ts:437, findOrgBoundToContextTreeRepo lowercases the stored repo and compares it with one exact HTTPS + .git spelling. The context_tree.repo contract deliberately accepts HTTPS, ssh://, and scp-like SSH forms, and canonicalGitRepoUrl already treats those forms (and optional .git) as the same repository. If Team A is bound to git@github.com:acme/<derived-name>.git or to the HTTPS URL without .git, Team B's initializer sees no conflict, adopts that exact GitHub repository, and binds it—the cross-team context leak this PR is meant to close remains reachable. The new audit script has the same gap because it groups by lower(raw_repo), so aliases of one repository are reported as distinct.
Please make both conflict detection and audit grouping use the shared canonical repository identity, and cover at least the HTTPS-without-.git and SSH aliases in tests. No schema migration is required for that correction, but the ownership decision must be based on canonical repo identity rather than transport text.
Non-blocking: consider closing the audit script's DB client in finally; a query failure currently reaches the outer catch without db.end().
…ng write Two corrections from review. The conflict lookup compared URL text. `context_tree.repo` accepts HTTPS, `ssh://`, and scp-like SSH spellings with an optional `.git`, so a team bound through one transport was invisible to a lookup arriving through another and the cross-team adoption this guards against stayed reachable. Both the lookup and the audit script now decide on `canonicalGitRepoUrl`, which is the shared cross-package repository identity. The guard also only covered the Cloud initializer, while manual Settings binding and the `tree init` callback recorded whatever repository they were given. The repository is the thing being claimed regardless of which surface names it, so the check moves into `assertContextTreeBindingTargetAuthorized` — the seam both write paths already call inside their transaction. Checking and writing in one transaction is still two steps for two different organizations, which the caller's own row lock does not serialize, so the check takes a transaction-scoped advisory lock on the repository identity first. The initializer keeps its own pre-check: failing before the GitHub round trip avoids creating a repo only to reject the binding afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The concurrent-binding window was not in scope for this change; the check stays a plain read and the residual race is documented rather than closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yuezengwu
left a comment
There was a problem hiding this comment.
The follow-up fixes raw URL aliasing in the GitHub path and moves the check onto every binding writer, but the latest commit explicitly removes cross-organization serialization. I cannot approve while the tenant-isolation race remains.
Blocker — the cross-organization race is still the original vulnerability. packages/server/src/services/org-settings.ts now documents that two organizations can both pass the lookup and commit the same repository, then deliberately leaves that window open. The per-organization row locks do not serialize these writes. This PR is a tenant-isolation fix whose stated invariant is one Context Tree repository per team; a known path that still records two bindings cannot be deferred as an implementation detail. Make the ownership decision atomic at the binding write boundary (reservation, unique authority key, advisory lock, or an equivalent mechanism) and add a deterministic two-connection regression test proving that exactly one of two organizations can commit the same repository authority. If the team instead wants to accept this residual security risk, that requires an explicit human-owner decision and the PR's safety claims must be narrowed accordingly.
Blocker — do not use the provider-neutral key as repository authority for GitLab. canonicalGitRepoUrl intentionally drops transport details, including an HTTPS port. The shared contract and tests explicitly treat https://gitlab.internal:8443/group/tree and https://gitlab.internal:9443/group/tree as different Context Tree authorities, while sameContextTreeRepository preserves that exact-origin distinction. The new guard and audit grouping collapse both to gitlab.internal/group/tree, so teams on distinct GitLab instances are wrongly blocked and the audit reports a false shared-tree incident. Derive a provider/connection-aware authority identity—including each team's exact GitLab web origin—for comparison, serialization, and audit grouping; cover different HTTPS ports and SSH-to-web-origin mapping.
This changes a core PostgreSQL-backed tenant invariant through a cross-org scan, although it adds no migration. Please have a human check the final atomicity mechanism, exact GitLab-origin semantics, historical data behavior, and concurrency coverage.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The follow-up fixes GitHub URL aliases and covers every binding surface, but it explicitly leaves the cross-Team race open and its global key conflates distinct self-managed GitLab repositories when their origins differ only by port.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Prevent two Teams from silently binding the same Context Tree repository after a generated-name collision or an equivalent transport spelling.
- Approach: Generate organization-specific GitHub repo names, compare bindings canonically, check one-Team ownership from every binding writer, and audit existing bindings.
- Impacted modules: Context Tree initialization, repository provisioning, organization-settings writes, audit tooling, and server tests.
Review findings
❌ 1. The latest commit explicitly leaves the cross-Team check/write race open. Both writers lock different organization rows, both can observe no holder at line 794, and both can then commit the same repository. This is the same silent shared-tree state the PR defines as a tenant-isolation defect; documenting the window does not enforce the stated one-Team-per-repository invariant. Serialize on the repository identity (or enforce equivalent atomic uniqueness) at every binding write and keep a deterministic concurrent-write regression test. [R4, R5 / packages/server/src/services/org-settings.ts:782, packages/server/src/services/org-settings.ts:794]
❌ 2. Do not use the provider-neutral canonical key as the global repository ownership identity. canonicalGitRepoUrl intentionally drops the URL port; the shared module provides stronger Context Tree comparison semantics because self-managed GitLab HTTPS repositories are identified by the complete web origin, including a non-default port. With this change, Team A bound to https://gitlab.internal:8443/group/tree.git makes Team B's distinct https://gitlab.internal:9443/group/tree.git look occupied: both canonicalize to gitlab.internal/group/tree, and the second write is rejected. The audit likewise reports those independent repositories as a shared tree. Build the exclusivity identity at provider authority strength, resolving SSH/scp spellings through each Team's GitLab connection origin, and use the same identity for serialization, conflict lookup, and audit grouping. Cover two Teams using the same host/path on different ports. [R1, R5 / packages/server/src/services/org-settings.ts:441, packages/server/scripts/audit-shared-context-tree-repos.ts:42; contract helper: packages/shared/src/canonical-git-repo-url.ts:77]
✅ 3. Canonical alias matching now closes the original HTTPS/SSH spelling gap for GitHub, and moving the guard into the shared write seam covers manual Settings plus tree init. [packages/server/src/services/org-settings.ts:458, packages/server/src/services/org-settings.ts:805]
Action taken
- Submitted request changes on the updated head.
`canonicalGitRepoUrl` deliberately drops transport detail including the HTTPS port, but a self-managed forge's port is part of which forge it is. Deciding ownership on it conflated `git.internal:8443` with `git.internal:9443`, so two teams on separate instances were refused their own trees and the audit would report a shared tree that does not exist. Ownership now compares on a key that keeps a non-default port when the reference carries one. scp-like SSH references have no web port to keep and fall back to the canonical form, so an SSH binding and an HTTPS binding on a non-default port do not compare equal — that direction fails open, leaving a conflict unnoticed rather than refusing a legitimate binding. The concurrent cross-organization binding window stays open by owner decision; the PR states it rather than claiming the rule is race-safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The latest change fixes the different-HTTPS-port false positive, but it deliberately fails open for SSH aliases of non-default-port GitLab repositories, and the previously raised cross-Team race remains unresolved.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Prevent multiple Teams from silently claiming one Context Tree repository while allowing distinct forge repositories to remain independent.
- Approach: Add an organization discriminator to generated GitHub names, check repository ownership on every binding path, normalize accepted repository spellings, and audit historical shared bindings.
- Impacted modules: Context Tree initialization, organization-settings writes, repository identity, audit tooling, and server tests.
Review findings
❌ 1. Resolve SSH/scp references through the Team's GitLab connection instead of accepting a documented fail-open. The new helper treats any URL port as forge identity, but an ssh://...:2222 port is a transport port, not the GitLab web origin; scp-like SSH has no port at all. Consequently, ssh://git@git.internal:2222/group/tree.git or git@git.internal:group/tree.git does not match the same repository bound as https://git.internal:8443/group/tree.git, and even two SSH spellings can diverge. That permits two Teams to bind the same tree and makes the audit miss it, contradicting both the one-Team invariant and the existing rule that SSH spellings map through the current GitLab connection origin. Compare provider-strength authority using each row's provider/connection context, and cover HTTPS :8443 against both SSH forms (while retaining the :8443 versus :9443 distinction). [R1, R5 / packages/server/src/services/org-settings.ts:784, packages/server/src/services/org-settings.ts:793, packages/server/scripts/audit-shared-context-tree-repos.ts:44]
❌ 2. The prior atomicity blocker is unchanged: lines 813-816 still state that concurrent organizations can both pass the ownership check and commit the same repository. The latest commit records a choice to leave that window open, but documenting it does not enforce the PR's tenant-isolation invariant. The final write still needs repository-scoped serialization or equivalent atomic uniqueness plus a deterministic concurrency regression test; accepting the residual isolation risk instead requires an explicit maintainer decision and correspondingly narrower safety claims. [R4, R5 / packages/server/src/services/org-settings.ts:813]
✅ 3. Preserving non-default HTTPS ports correctly distinguishes separate self-managed forge instances that share a hostname and project path. [packages/server/src/services/org-settings.ts:789, packages/server/src/__tests__/org-settings.test.ts:258]
Action taken
- Submitted request changes on the updated head.
yuezengwu
left a comment
There was a problem hiding this comment.
The latest commit correctly distinguishes self-managed GitLab instances that share a host/path but use different HTTPS ports. The binding owner has also explicitly accepted and documented the residual concurrent-write window, so I am not reopening that scope decision in this review. One tenant-isolation blocker remains.
Blocker — resolve GitLab SSH references through each Team's connection origin. contextTreeRepoOwnershipKey treats every URL port as forge authority, even though ssh://...:2222 carries an SSH transport port, not the GitLab Web origin; scp-like SSH has no Web port and deliberately falls back to a portless key. As a result, ssh://git@git.internal:2222/group/tree.git and git@git.internal:group/tree.git both fail to match the same repository stored as https://git.internal:8443/group/tree.git. Two Teams can therefore bind one Context Tree through supported equivalent transports, and the audit reports no sharing. This conflicts with the existing repository contract and helper tests, which map SSH through the Team's exact GitLab connection origin and never reuse its transport port.
Please derive the ownership identity from provider plus each binding Team's GitLab connection origin—for both the target and every stored candidate—and use the same identity in the guard and audit. Add coverage proving that HTTPS :8443 conflicts with both ssh://...:2222 and scp-like SSH on a Team connected to Web origin :8443, while HTTPS :8443 and :9443 remain distinct.
This adds no schema migration, but it changes a core PostgreSQL-backed cross-Team ownership decision. A human should check the provider-aware lookup, historical rows, and audit grouping before merge.
The ownership key appended any URL port, so `ssh://git@host:2222/group/tree` keyed as `host:2222/group/tree`. An SSH port is a transport detail of one checkout, not a forge web origin, and the repository contract never treats it as one — so that key matched neither the scp-like spelling of the same reference nor the team actually bound to that forge. SSH references now key on the canonical host and path, both spellings alike. GitHub is unaffected either way: neither spelling carries a port there, so SSH and HTTPS already compare equal. An SSH reference still does not match a team bound through a non-default web port; resolving that means mapping each team's reference through its own GitLab connection origin. That direction fails open — the conflict goes unnoticed rather than a legitimate binding being refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yuezengwu
left a comment
There was a problem hiding this comment.
The latest commit correctly stops treating ssh://...:2222 as forge Web authority and makes the two SSH spellings agree. It does not resolve the remaining tenant-isolation blocker.
Blocker — SSH still has to map through the Team's exact GitLab connection origin. contextTreeRepoOwnershipKey now produces git.internal/group/tree for both SSH forms, while the same repository stored as https://git.internal:8443/group/tree.git produces git.internal:8443/group/tree. The explicit fail-open at packages/server/src/services/org-settings.ts:784 therefore remains: one Team can hold the tree through HTTPS and another can bind it through SSH, and audit-shared-context-tree-repos.ts will group them separately. This contradicts the existing Context Tree repository contract and shared helper behavior, where SSH/scp references map through the Team's exact GitLab connection Web origin.
Please make the guard and audit resolve both the target and each stored binding with its provider and owning Team's GitLab connection. Cover HTTPS :8443 against ssh://...:2222 and scp-like SSH on a Team connected to Web origin :8443, while retaining the :8443 versus :9443 distinction.
I am not reopening the owner-accepted concurrent-write limitation. There is still no schema migration, but the provider-aware cross-Team lookup and historical audit grouping need human review before merge.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The new commit correctly removes the SSH transport port from the key, but it still does not map SSH references through each Team's GitLab web origin, so the same repository remains claimable through HTTPS and SSH spellings.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Prevent two Teams from silently binding one Context Tree repository while keeping repositories on distinct self-managed forge origins independent.
- Approach: Add an organization-specific generated name, enforce ordered-write ownership checks on every binding path, normalize repository aliases, and audit historical sharing.
- Impacted modules: Context Tree initialization, organization-settings writes, repository identity, audit tooling, and server tests.
Review findings
❌ 1. The previously reported GitLab origin gap remains. contextTreeRepoOwnershipKey now maps both ssh://git@git.internal:2222/group/tree.git and git@git.internal:group/tree.git to git.internal/group/tree, but the same repository bound through the Team's supported HTTPS origin maps to git.internal:8443/group/tree. findOrgBoundToContextTreeRepo and the audit therefore still treat one repository as two identities and allow/report no conflict. Derive the ownership identity from provider plus each binding Team's gitlab_connections.instance_origin for both the target and every stored candidate; cover HTTPS :8443 against both SSH forms while preserving the :8443 versus :9443 distinction. The new unit test currently documents only the transport-port correction, not the required cross-transport equivalence. [R1, R5 / packages/server/src/services/org-settings.ts:784, packages/server/src/services/org-settings.ts:792, packages/server/scripts/audit-shared-context-tree-repos.ts:41]
✅ 2. Treating SSH port 2222 as transport detail is correct and now makes ssh:// and scp-like spellings agree. [packages/server/src/services/org-settings.ts:796, packages/server/src/__tests__/org-settings.test.ts:255]
Action taken
- Submitted request changes on the updated head.
|
Owner decision on the remaining blocker: Gandy2025 accepts the SSH-versus-non-default-web-port fail-open rather than requiring connection-aware mapping in this change. The PR's "Accepted limitations" section now records it as a decision alongside the concurrent-write window, and the paired tree node states the same constraint as current truth. Scope of what is accepted, for the record:
Resolving it properly means resolving the target and every stored candidate through its owning team's GitLab connection origin, in both the guard and the audit — a separate change, not folded in here. |
yuezengwu
left a comment
There was a problem hiding this comment.
The code has not closed the SSH/non-default-Web-port gap, but the binding owner has now explicitly accepted that residual risk and the concurrent-write window, and the PR description narrows the guarantee accordingly. That satisfies the escalation condition in my previous review, so I am clearing my change request.
Approved with the documented limitations: exclusivity is an ordered-write best effort, and self-managed GitLab repositories on a non-default Web port can still evade the guard/audit when one binding uses SSH and the other HTTPS. This is an owner-approved scope decision, not evidence that those paths are safe.
There is no schema migration. The cross-Team ownership lookup and historical audit remain core PostgreSQL-backed behavior and should retain explicit human ownership as the paired Context Tree update lands.
|
Maintainer decision: this remains blocking for this PR. Please implement connection-aware repository ownership before approval rather than retaining the GitLab SSH/non-default-Web-port fail-open as an accepted limitation. The guard and audit need to resolve both the target binding and every stored candidate through the owning Team's exact GitLab connection origin. Regression coverage should prove that HTTPS The existing |
yuezengwu
left a comment
There was a problem hiding this comment.
The maintainer decision supersedes the earlier scope acceptance, so I am restoring my change request on the unchanged head.
Blocker — implement connection-aware GitLab repository ownership. The current key still treats https://git.internal:8443/group/tree.git and the equivalent ssh://git@git.internal:2222/group/tree.git / git@git.internal:group/tree.git references as different ownership identities. The guard can therefore allow two Teams to bind one Context Tree, and the historical audit can miss the shared binding. Resolve the target and every stored candidate through its owning Team's exact gitlab_connections.instance_origin, and use the same provider-strength identity in both guard and audit. Regression coverage must show HTTPS :8443 conflicts with both SSH forms for Web origin :8443, while HTTPS :8443 and :9443 remain distinct.
The separately accepted concurrent-write limitation is not reopened here. There is no schema migration, but this is a core PostgreSQL-backed cross-Team ownership and audit path and requires human review of the final connection-aware lookup and historical-row behavior.
Maintainer decision on this PR: the SSH-versus-web-port fail-open is blocking rather than an acceptable limitation, so ownership is now decided at provider strength instead of on a transport-shaped key. A repository reference alone cannot say who owns it. The binding contract accepts HTTPS, `ssh://`, and scp-like SSH spellings, and only the owning team's GitLab connection says which web origin an SSH reference belongs to — its transport port is never the forge's web port. So a GitLab reference now resolves through that team's own `instance_origin`, for the target and for every stored candidate alike, and one repository reached over SSH by one team and HTTPS by another reduces to a single identity. Two instances on one host stay distinct, because a non-default web port is part of which forge it is. GitHub needs no connection: one fixed origin, no port, both spellings already agree under the shared canonical form. A reference whose forge cannot be classified — a self-managed host with no connection, or one whose origin does not match its team's connection — has no establishable authority and is skipped. That direction fails open by design: a conflict goes unnoticed rather than a legitimate binding being refused on a guess. The audit joins the same connection origin and groups on the same identity, so it no longer reports transport aliases of one repository as unrelated rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Implemented the maintainer decision: ownership now resolves at provider strength through each team's own Regression coverage, each backed by a real connection row:
A reference whose forge cannot be established is skipped rather than matched on a guess — a conflict goes unnoticed rather than a legitimate binding being refused. The concurrent cross-organization write window remains the one accepted limitation.
|
yuezengwu
left a comment
There was a problem hiding this comment.
The connection-aware mapping fixes the reported live-connection case: HTTPS :8443, ssh://...:2222, and scp-like SSH now converge for Teams on the same GitLab instance, while :8443 and :9443 remain distinct. One historical-binding blocker remains.
Blocker — do not forget an exact HTTPS binding when its Team has no current GitLab connection. contextTreeRepoOwnershipIdentity returns null for every GitLab reference when gitlabInstanceOrigin is absent or mismatched, and both the guard and audit skip that row. Deleting a GitLab connection is currently allowed without removing context_tree, so this is reachable: Team A remains bound to https://git.internal:8443/group/tree.git, its connection is deleted, and Team B on https://git.internal:8443 can then bind the same repository because Team A's candidate identity is discarded. The audit misses the shared binding too. This regresses the previous exact-HTTPS comparison and violates the stored binding's tenant ownership even though the URL itself already establishes the full Web origin and port.
Preserve exact HTTP(S) origin + path as ownership identity without requiring a live connection; require the owning Team's connection only to map SSH/scp references that do not carry a Web origin. Cover a holder with an HTTPS :8443 binding after its connection is deleted (or otherwise absent), against a claimant using HTTPS and SSH through :8443, and ensure audit grouping follows the same rule.
The accepted concurrent-write limitation is not reopened. There is no schema migration, but the historical-row behavior of this PostgreSQL-backed cross-Team guard and audit requires human review.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The connection-aware guard now closes the requested GitLab alias gap, but the historical audit silently discards unresolved legacy bindings and can still report a false clean result.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Prevent two Teams from silently reading and writing one Context Tree after a generated-name collision or equivalent repository spelling, and identify historical shared bindings for repair.
- Approach: Generate organization-specific GitHub repository names, enforce ordered-write ownership checks on every binding path using provider-strength identity, and audit existing bindings.
- Impacted modules: Context Tree initialization, organization-settings writes, repository identity, audit tooling, and server tests.
Review findings
❌ 1. Do not turn unresolved historical bindings into a clean audit result. groupBySharedRepo skips every row whose provider authority cannot be established, but main then prints “No Context Tree repo is bound to more than one team” and exits successfully when no resolved duplicate remains. Legacy and manually edited bindings are explicitly allowed to retain a repo without a provider or matching GitLab connection, so these are valid historical rows and are exactly what this remediation audit must account for. Two affected Teams can therefore be omitted while the operator receives false assurance that no sharing exists. Keep unresolved rows separate and make the result explicitly inconclusive/non-zero (or provide an equivalently fail-safe outcome); add script-level coverage for unresolved legacy rows as well as the resolved HTTPS/SSH grouping. [packages/server/scripts/audit-shared-context-tree-repos.ts:45, packages/server/scripts/audit-shared-context-tree-repos.ts:89]
✅ 2. The runtime ownership comparison now resolves the target and each stored GitLab binding through that Team's exact connection origin. The requested HTTPS :8443 versus both SSH spellings equivalence is covered, while :8443 and :9443 remain distinct. [packages/server/src/services/org-settings.ts:438, packages/server/src/services/org-settings.ts:815, packages/server/src/__tests__/org-settings.test.ts:258]
Action taken
- Submitted request changes on the updated head.
Requiring a GitLab connection to resolve any GitLab reference discarded the identity of a binding whose team later deletes its connection — deleting one does not clear `context_tree`, so a holder outlives it. The guard and audit then skipped that row, and another team on the same instance could bind the repository the holder still owns. That regressed the exact-HTTPS comparison this PR previously had. The origin now comes from wherever the reference states it. GitHub states it implicitly. An HTTP(S) reference states it outright, port included, and needs nothing else. Only SSH and scp-like references, which carry no origin and whose transport port is never the forge's, resolve through the owning team's connection. All three land on one string for one repository, so an HTTPS holder and an SSH claimant still collide. An SSH reference whose team has no usable connection remains unclaimed rather than guessed at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: Exact HTTPS ownership now survives connection deletion, but the historical audit still silently discards unresolved SSH bindings and can report a false clean result.
Risk level: B-low
- Path baseline:
packages/server/**without schema changes or the high-risk service set -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Prevent two Teams from silently reading and writing one Context Tree after a generated-name collision or equivalent repository spelling, and identify historical shared bindings for repair.
- Approach: Generate organization-specific GitHub repository names, enforce ordered-write ownership checks on every binding path using provider-strength identity, and audit existing bindings.
- Impacted modules: Context Tree initialization, organization-settings writes, repository identity, audit tooling, and server tests.
Review findings
❌ 1. The previous audit blocker is unchanged for the rows that genuinely cannot be resolved. groupBySharedRepo still drops an SSH/scp binding when its Team has no usable GitLab connection, then main prints “No Context Tree repo is bound to more than one team” and exits successfully when no resolved duplicate remains. Legacy or manually edited bindings are valid without a current provider/connection, so the remediation audit cannot silently omit them and claim completeness. Track unresolved rows separately and make the result explicitly inconclusive/non-zero (or provide an equivalently fail-safe outcome), with script-level coverage for an unresolved SSH binding. [packages/server/scripts/audit-shared-context-tree-repos.ts:45, packages/server/scripts/audit-shared-context-tree-repos.ts:88]
✅ 2. The new authority split is correct: an HTTP(S) reference keeps its exact origin and path without a connection, while only SSH/scp uses the owning Team's connection. The holder-with-deleted-connection runtime leak is therefore closed for both HTTPS and SSH claimants. [packages/server/src/services/org-settings.ts:821, packages/server/src/__tests__/org-settings.test.ts:336]
Action taken
- Submitted request changes on the updated head.
yuezengwu
left a comment
There was a problem hiding this comment.
The latest commit fixes my previous blocker: an exact HTTP(S) binding retains its origin/path identity after the Team loses its connection, while SSH/scp still maps through the owning Team's connection. The added deleted-connection regression covers the runtime guard. One audit blocker remains.
Blocker — unresolved historical rows must make the audit inconclusive, not clean. groupBySharedRepo still drops every SSH/scp binding whose Team has no usable GitLab connection, then main prints “No Context Tree repo is bound to more than one team” and exits successfully when no resolved duplicate remains. Legacy/manual bindings are allowed to retain a repo without executable provider authority, so these are valid historical rows in the exact remediation population this script is meant to assess. Skipping them can give an operator false assurance even when two unresolved bindings share one repository.
Return unresolved rows separately and report an explicit inconclusive/non-zero result (or an equivalently fail-safe outcome), while continuing to report resolved duplicates. Add script-level coverage for unresolved legacy rows and for resolved HTTPS/SSH grouping; the current tests exercise the shared identity and runtime guard but not the audit's grouping/result semantics.
The accepted concurrent-write limitation is not reopened. There is no schema migration, but this operator audit reads core PostgreSQL tenant bindings and its clean-result semantics require human review.
The audit dropped every row it could not resolve and then printed that no repository is shared, exiting zero. Legacy and hand-edited bindings are allowed to keep a repo with no provider and no matching connection, so those rows are the remediation population this script exists to assess — two of them can name one repository while the operator reads a clean result and stops looking. Unresolved rows are now collected separately, listed, and make the run non-zero: the outcome says the run could not tell, not that nothing is shared. A clean result now means both that no repository is shared and that every binding resolved. The partitioning moves into `services/context-tree-binding-audit.ts`, beside the ownership rule it depends on, so it is typechecked and tested with the rest of the service layer while the script stays the runner that supplies rows and prints them. Importing that module no longer opens a database connection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both audit reviews are addressed on Unresolved rows are no longer dropped. The partitioning moved to Script-level coverage in Also refreshed both PR descriptions — they still claimed every identity resolves through a team's connection, which stopped being true when exact HTTP(S) origins were made connection-independent.
|
yuezengwu
left a comment
There was a problem hiding this comment.
The remaining audit blocker is fixed. Unresolved legacy/SSH rows are now retained and printed, and any shared or unresolved result exits non-zero; the clean message is reachable only when no repository is shared and every binding resolved. The extracted partitioning tests cover resolved transport aliases, distinct self-managed ports, unresolved legacy rows, and mixed shared/unresolved results.
The runtime ownership path also retains exact HTTP(S) identity after connection deletion and maps SSH/scp through the owning Team's connection, so the previously reviewed tenant-isolation cases are covered. I found no remaining blocking issue in the updated diff.
Approved with the documented owner-accepted concurrent-write limitation. There is no schema migration; the cross-Team guard and read-only PostgreSQL audit remain core tenant behavior and should retain human ownership as the paired Context Tree update lands.
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: approve
- Rationale: The final follow-up removes the audit's false-clean result for unresolved historical bindings while preserving provider-strength repository identity across every ordered binding path.
Risk level: B-low
- Path baseline:
packages/server/**without a schema change or one of the high-risk server services -> B-low - Semantic lift: none
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Teams could silently end up reading and writing the same Context Tree after a generated-name collision or equivalent repository spelling, while the remediation audit could miss historical aliases or unresolved bindings.
- Approach: Derive organization-specific GitHub repository names, reject an ordered binding when another Team owns the provider-strength repository identity, and make the historical audit distinguish confirmed sharing from bindings it cannot resolve.
- Impacted modules: Context Tree initialization, organization-settings binding writes, repository identity, the historical audit runner, and server tests.
Review findings
✅ 1. The audit now returns unresolved SSH/scp bindings separately, prints them as inconclusive, and exits non-zero; it emits a clean result only when no duplicate exists and every binding resolves. [packages/server/src/services/context-tree-binding-audit.ts, packages/server/scripts/audit-shared-context-tree-repos.ts]
✅ 2. The regression coverage keeps resolved HTTPS/SSH aliases together, keeps self-managed instances on different Web ports apart, and covers unresolved rows alongside confirmed sharing. [packages/server/src/__tests__/context-tree-binding-audit.test.ts]
Action taken
- Approved the exact reviewed head
bd5b49b2df8607a8b619d6a3ebb42682b48718d7.
Follow-up to #2115 / #2117. Same root cause — a derived identifier assuming display names are unique — but this one is reachable today and mixes two teams' context.
Problem
contextTreeRepoNamederived the Context Tree repo name from the team display name alone.slugifyRepoBasedrops every character outside[a-z0-9], so any display name without an ASCII alphanumeric falls back to the same"team"base:Display names are deliberately not unique, so this is not only a CJK problem — two teams both named "Design Team" under one GitHub account collide the same way. CJK just makes every name collide.
Provisioning then makes the collision silent. Both storage backends adopt an existing repo on the deterministic name:
ensureOrganizationRepofalls back to adoption on422 already exists, andensureUserRepoprobes for the repo before creating anything. Neither asks whether the repo already belongs to a different team —verifyInstallationCanAccessonly asks whether this installation can read it, which is trivially true for a sibling team under the same account.Walked through, with two teams under
acme-inc:team-context-tree→ creates it → binds it.team-context-tree→ create returns 422 → adopts → binds the same repo.No error. Both teams now read and write one Context Tree, so each team's decisions, constraints and ownership are visible and writable by the other.
Adoption itself is not the bug. Initialization is gated on
existing.kind !== "unbound", so a team that already has a tree cannot reach this code; adoption exists to recover this team's repo when an earlier run created it but failed before persisting the binding. A colliding name is what turned "recover my own repo" into "take over someone else's".Change
The name gets a per-organization discriminator, so the collision cannot happen:
The discriminator hashes the organization id rather than slicing it — ids are opaque text, and a uuidv7's leading bytes are a timestamp, so same-day organizations would share any prefix taken from one. It stays stable for a given organization, so a retry still adopts its own repo, and it is included in the length budget so names stay under GitHub's 100-character limit.
Every binding write refuses a repo another team holds, so a collision cannot pass unnoticed if one occurs anyway. A name only guesses at ownership;
organization_settingsis the fact. The check lives inassertContextTreeBindingTargetAuthorized, the seam that Cloud provisioning, manual Settings binding, and thetree initcallback all already call inside their transaction — the repository is the thing being claimed regardless of which surface names it. The initializer also keeps a pre-check so it fails before the GitHub round trip rather than creating a repo and rejecting the binding afterwards, returning 409context_tree_repo_owned_by_other_org.Ownership is decided on the repository's web origin and path, not on URL text.
context_tree.repoaccepts HTTPS,ssh://, and scp-like SSH spellings with an optional.git, and where that origin comes from depends on the reference. GitHub states it implicitly — one fixed origin, no port — so every spelling already agrees. An HTTP(S) reference states it outright, port included, and needs nothing else; requiring a connection there would discard the identity of a binding whose team later deletes its connection, and that binding still owns its repository. Only SSH and scp-like references, which carry no origin and whose transport port is never the forge's, resolve through the owning team'sgitlab_connections.instance_origin— for the target and every stored candidate alike.All three land on one identity for one repository, so a team on HTTPS and a team on SSH are seen to hold the same tree, while two self-managed instances on one host stay distinct.
An SSH reference whose team has no usable connection has no establishable origin. The guard leaves it unclaimed — a conflict goes unnoticed rather than a legitimate binding being refused on a guess — and the audit reports it rather than dropping it (below).
The audit groups on the same identity, so transport aliases of one repository are no longer reported as unrelated rows. Rows it cannot resolve are listed separately and make the run non-zero: legacy and hand-edited bindings may keep a repo with no provider and no matching connection, and dropping them would let two affected teams go unlisted while the operator reads a clean result. A clean result now means both that nothing is shared and that every binding resolved.
Both are needed. The discriminator alone would not cover repos created under the old naming, a repo somebody created by hand, or a leftover from a team that unbound its tree. The check alone would leave every collision to be recovered after a failed GitHub round trip.
contextTreeRepoNamemoves from the route file intocontext-tree-repo-provisioner, next to the adoption it constrains and where it can be tested directly.Existing data
The new rule does not unpick bindings already written.
packages/server/scripts/audit-shared-context-tree-repos.tsreports every repo more than one team is bound to, plus every binding whose forge it could not establish. It is read-only — one SELECT, no writes — and each row it finds needs an owner decision about which team keeps the tree, so nothing is repaired automatically.Tests
context-tree-repo-provisioner.test.ts: derivation unit tests — the readable base survives, two teams that derive the same base get different names (CJK pair and a duplicate English display name), the name is stable per organization, and it respects the 100-character limit.context-tree-initialize.test.ts: parking the derived repo on another team makes initialization return 409context_tree_repo_owned_by_other_org, mint only the installation token, create or adopt nothing, and leave the caller unbound — covered for the clone URL, HTTPS without.git,ssh://, and scp-like SSH spellings.org-settings.test.ts: aliases of one repository are refused on the manual Settings write path, and a team re-stating its own binding through a different spelling is still allowed. For a team connected to web origin:8443, an HTTPS binding conflicts withssh://…:2222, with scp-like SSH, and with the HTTPS spelling — each backed by a real connection row — while two teams connected to:8443and:9443bind the same host and path independently. A unit test pins the identity itself: the three GitLab spellings collapse to one web identity, a mismatched or unclassifiable origin yields none, and GitHub's SSH and HTTPS spellings agree without any connection.context-tree-binding-audit.test.ts: the audit groups HTTPS/ssh:///scp spellings of one repository together, keeps:8443and:9443apart, reports an unresolved legacy row instead of dropping it, and separates unresolved rows from a genuine sharing report.context-tree-initialize.test.ts/context-tree-routes-mocked.test.ts: expected repo names now come from the real derivation instead of hardcoded strings, so a change to the rule cannot pass by matching a stale copy.pnpm check,pnpm typecheck, and the full server suite (2882) pass.Accepted limitation
The SSH-versus-web-port gap previously listed here is closed — connection-aware resolution was implemented per the maintainer decision on this PR. One limitation remains, and the paired tree node records it as a current constraint:
QA risk
This touches Context Tree provisioning and the GitHub App installation path, so it is worth formal QA.
packages/qa/cases/has no case for two teams provisioning under one installation; happy to add one if you want it covered there.🤖 Generated with Claude Code
Context Tree
Paired tree change: agent-team-foundation/first-tree-context#871 (draft until this merges).