Skip to content

fix: stop one team adopting another team's Context Tree repo - #2122

Merged
Gandy2025 merged 8 commits into
mainfrom
fix/context-tree-repo-name-collision
Aug 3, 2026
Merged

fix: stop one team adopting another team's Context Tree repo#2122
Gandy2025 merged 8 commits into
mainfrom
fix/context-tree-repo-name-collision

Conversation

@Gandy2025

@Gandy2025 Gandy2025 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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

contextTreeRepoName derived the Context Tree repo name from the team display name alone. slugifyRepoBase drops every character outside [a-z0-9], so any display name without an ASCII alphanumeric falls back to the same "team" base:

"ACME Robotics"  ->  acme-robotics-context-tree
"电商平台"        ->  team-context-tree
"设计团队"        ->  team-context-tree
"销售部"          ->  team-context-tree

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: ensureOrganizationRepo falls back to adoption on 422 already exists, and ensureUserRepo probes for the repo before creating anything. Neither asks whether the repo already belongs to a different team — verifyInstallationCanAccess only 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:

  1. 电商平台 initializes → derives team-context-tree → creates it → binds it.
  2. 设计团队 initializes → derives 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:

acme-robotics-2671b724-context-tree
team-4d0a91c7-context-tree
team-9f31ab05-context-tree

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_settings is the fact. The check lives in assertContextTreeBindingTargetAuthorized, the seam that Cloud provisioning, manual Settings binding, and the tree init callback 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 409 context_tree_repo_owned_by_other_org.

Ownership is decided on the repository's web origin and path, not on URL text. context_tree.repo accepts 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's gitlab_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.

contextTreeRepoName moves from the route file into context-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.ts reports 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 409 context_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 with ssh://…:2222, with scp-like SSH, and with the HTTPS spelling — each backed by a real connection row — while two teams connected to :8443 and :9443 bind 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 :8443 and :9443 apart, 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:

  • Concurrent cross-organization binding. Two organizations binding the same repository at the same instant can both pass the check; the caller's own row lock does not serialize a different organization's write. Closing it needs serialization on the repository identity. Gandy2025 accepted this residual risk, so the rule is stated as holding between ordered writes rather than as race-safe.

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).

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Gandy2025 and others added 2 commits July 31, 2026 18:26
…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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Gandy2025

Copy link
Copy Markdown
Contributor Author

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:

  • GitHub is unaffected — neither spelling carries a port there, so SSH and HTTPS already compare equal.
  • The gap needs a self-managed forge on a non-default web port, plus two teams binding one repository through different transports.
  • It fails open: a conflict goes unnoticed rather than a legitimate binding being refused.

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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@baixiaohang

Copy link
Copy Markdown
Collaborator

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 :8443 conflicts with both ssh://...:2222 and scp-like SSH for a Team connected to Web origin :8443, while HTTPS :8443 and :9443 remain distinct.

The existing CHANGES_REQUESTED review therefore stands. The separately accepted concurrent-write limitation is not being reopened by this decision.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@Gandy2025

Copy link
Copy Markdown
Contributor Author

Implemented the maintainer decision: ownership now resolves at provider strength through each team's own gitlab_connections.instance_origin, for the target and every stored candidate, in both the guard and the audit. The previously listed SSH-versus-web-port limitation is closed and removed from the PR body.

Regression coverage, each backed by a real connection row:

  • For a team connected to web origin :8443, an HTTPS binding conflicts with ssh://…:2222, with scp-like SSH, and with the HTTPS spelling.
  • Two teams connected to :8443 and :9443 bind the same host and path independently.
  • A unit test pins the identity: the three GitLab spellings collapse to one web identity, a mismatched or unclassifiable origin yields none, and GitHub's SSH and HTTPS spellings agree with no connection at all.

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.

pnpm check, pnpm typecheck, and the full server suite (2876) pass.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@Gandy2025

Copy link
Copy Markdown
Contributor Author

Both audit reviews are addressed on bd5b49b2.

Unresolved rows are no longer dropped. auditBindings returns them separately, the script lists them, and their presence makes the run non-zero — the outcome now says the run could not tell rather than that nothing is shared. A clean result means both that no repository is shared and that every binding resolved.

The partitioning moved to src/services/context-tree-binding-audit.ts, beside the ownership rule it depends on, so it is typechecked and tested with the service layer; the script stays the runner. Importing that module no longer opens a database connection.

Script-level coverage in context-tree-binding-audit.test.ts: HTTPS/ssh:///scp spellings of one repository group together; :8443 and :9443 stay apart; an unresolved legacy SSH row is reported rather than dropped; and unresolved rows are kept separate from a genuine sharing report.

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.

pnpm check, pnpm typecheck, and the full server suite (2882) pass.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
⚠️ 3. Simultaneous cross-organization writes remain the documented, owner-accepted limitation: this approval covers ordered-write exclusivity, not race-safe exclusivity.

Action taken

  • Approved the exact reviewed head bd5b49b2df8607a8b619d6a3ebb42682b48718d7.

@Gandy2025
Gandy2025 merged commit 756cd7a into main Aug 3, 2026
19 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants