From b307d5ea55102e7b74ad6327ebec6ebb3ad20ad0 Mon Sep 17 00:00:00 2001 From: Santhi Prakash Date: Fri, 21 Aug 2026 02:18:16 +0000 Subject: [PATCH] fix(updates): scope commit drift to monorepo rootDirectory (#637) A monorepo project with its own rootDirectory was offered an unnecessary redeploy when other projects in the same repo changed. The drift tracker compared the deployed SHA against the repo-wide branch HEAD (latestSha) with no path scoping, so a commit that touched only a sibling directory still showed 'Update available A -> B'. When the project has a rootDirectory, compute behind-ness from the GitHub compare of deployedSha..latestSha filtered to paths under that root (or a configured monorepo shared-path / root-config file). Uses the same leaf matching as the webhook and smart-route deploys so the update badge and deploy routing always agree on 'affects this project'. Conservative on every failure mode: missing ctx, compare API error, or a truncated compare response (>=300 files) keep 'behind' as-is so the badge never misses a real update because the network blinked. --- .../github/webhook-changed-files.test.ts | 79 ++++++++++ .../modules/github/webhook-changed-files.ts | 27 ++++ .../modules/projects/project-crud.service.ts | 48 +++++- .../src/modules/updates/updates.service.ts | 4 +- .../modules/updates/drift-evaluation.test.ts | 147 ++++++++++++++++++ 5 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 apps/api/src/modules/github/webhook-changed-files.test.ts diff --git a/apps/api/src/modules/github/webhook-changed-files.test.ts b/apps/api/src/modules/github/webhook-changed-files.test.ts new file mode 100644 index 000000000..525be8259 --- /dev/null +++ b/apps/api/src/modules/github/webhook-changed-files.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, test } from "vitest"; + +import { rootScopeAffected } from "./webhook-changed-files"; + +/** + * #637: a monorepo project with its own root directory must only show an + * update when the repo diff actually touches that project — directly under + * its root, at a shared/root-config path, or when no root scopes it at all. + */ +describe("rootScopeAffected", () => { + test("a file under the project root counts", () => { + expect( + rootScopeAffected(["apps/backend/src/api.ts"], { rootDirectory: "apps/backend" }), + ).toBe(true); + }); + + test("a diff confined to a sibling directory does not count", () => { + expect( + rootScopeAffected(["apps/client/src/ui.tsx"], { rootDirectory: "apps/backend" }), + ).toBe(false); + }); + + test("an empty diff scoped to an untouched root does not count", () => { + expect(rootScopeAffected([], { rootDirectory: "apps/backend" })).toBe(false); + }); + + test("a similarly-prefixed sibling directory does not count (path boundary)", () => { + expect( + rootScopeAffected(["apps/backend-utils/turbo.json"], { rootDirectory: "apps/backend" }), + ).toBe(false); + }); + + test("a leading/trailing slash root still matches its directory", () => { + expect( + rootScopeAffected(["apps/backend/nested/deep.ts"], { rootDirectory: "/apps/backend/" }), + ).toBe(true); + }); + + test("a repo-root config change affects every project", () => { + expect( + rootScopeAffected(["package.json"], { rootDirectory: "apps/backend" }), + ).toBe(true); + }); + + test("a configured shared path affects every project", () => { + expect( + rootScopeAffected( + ["packages/ui/button.tsx"], + { + rootDirectory: "apps/backend", + isMonorepo: true, + monorepoSharedPaths: ["packages/"], + }, + ), + ).toBe(true); + }); + + test("a shared path is inert without monorepo semantics", () => { + expect( + rootScopeAffected( + ["packages/ui/button.tsx"], + { + rootDirectory: "apps/backend", + monorepoSharedPaths: ["packages/"], + }, + ), + ).toBe(false); + }); + + test("a project without a scoping root is affected by any change", () => { + expect(rootScopeAffected(["anything/anywhere.ts"], { rootDirectory: null })).toBe(true); + expect(rootScopeAffected(["anything/anywhere.ts"], {})).toBe(true); + expect(rootScopeAffected(["anything/anywhere.ts"], { rootDirectory: "." })).toBe(true); + }); + + test("file exactly at the root path counts as under it", () => { + expect(rootScopeAffected(["apps/backend"], { rootDirectory: "apps/backend" })).toBe(true); + }); +}); diff --git a/apps/api/src/modules/github/webhook-changed-files.ts b/apps/api/src/modules/github/webhook-changed-files.ts index 8480bdc83..f19e5150d 100644 --- a/apps/api/src/modules/github/webhook-changed-files.ts +++ b/apps/api/src/modules/github/webhook-changed-files.ts @@ -202,6 +202,33 @@ export function routeServicesByChanges( return { mode: "services", serviceIds: matched }; } +/** + * Decide whether a changed-files set affects a project scoped to a root + * directory — the update scanner's counterpart to `routeServicesByChanges` + * (#637). A root-config or configured monorepo-shared-path change affects + * every project in the repo, and any file under the project's own + * rootDirectory affects it; a diff confined to other directories does not. + * + * The matching rule is identical to `routeServicesByChanges`/`serviceMatchesChanges` + * so the update badge and the smart-route deploy agree on "affects this + * project" by construction. + */ +export function rootScopeAffected( + files: Iterable, + opts: { + rootDirectory?: string | null; + isMonorepo?: boolean; + monorepoSharedPaths?: string[] | null; + } = {}, +): boolean { + const root = (opts.rootDirectory ?? "").trim(); + // No scoping root ("" or ".") → the project IS the repo: any change counts. + if (!root || root === ".") return true; + const set = files instanceof Set ? files : new Set(files); + if (classifyChangedFiles(set, opts).forceAll) return true; + return serviceMatchesChanges(root, set); +} + function unionCommitFiles(commits: GitHubPushPayload["commits"] = []): Set { const out = new Set(); for (const c of commits ?? []) { diff --git a/apps/api/src/modules/projects/project-crud.service.ts b/apps/api/src/modules/projects/project-crud.service.ts index 7b32e1f68..3da34289e 100644 --- a/apps/api/src/modules/projects/project-crud.service.ts +++ b/apps/api/src/modules/projects/project-crud.service.ts @@ -48,7 +48,9 @@ import { listBranches as listGitHubBranches, getLatestCommit, resolveWebhookStrategy, + compareCommits, } from "../github/github.service"; +import { rootScopeAffected } from "../github/webhook-changed-files"; import { getInstallationIdByOrg, getInstallUrl } from "../github/github.auth"; import { domainWebhookUrl } from "../../lib/public-url"; import { ensureSharedWebhook, findSharedWebhookId } from "./project-git-webhook"; @@ -2040,7 +2042,11 @@ export async function resolveDeployedDrift( * private registry) or a project with no successful deploy reports * `behind:false`, so we never show an "outdated" nudge we can't substantiate. */ -export async function evaluateDrift(p: Project, upstream: UpstreamDrift) { +export async function evaluateDrift( + p: Project, + upstream: UpstreamDrift, + ctx?: RequestContext | null, +) { if (!upstream.supported) return { supported: false as const }; // A cached upstream describes the source it was polled from. If the project has // since been repointed (different repo, branch, release source), it answers a @@ -2056,7 +2062,45 @@ export async function evaluateDrift(p: Project, upstream: UpstreamDrift) { // supplied (an abbreviated `--commit`, a tag), so only a PROVABLE difference is // drift — otherwise a project deployed at `1eeaf76` is told a new commit // `1eeaf76` is available, forever. See compareCommitSha. - const behind = compareCommitSha(latestSha, deployedSha) === "different"; + let behind = compareCommitSha(latestSha, deployedSha) === "different"; + + // Monorepo scoping (#637): a branch HEAD that moved without touching this + // project's rootDirectory (or a shared/root-config path) must not offer a + // redeploy. Uses the SAME leaf matching as the webhook and smart-route + // deploys so the update badge and deploy routing always agree on + // "affects this project". + // + // Three fail-soft cases that keep `behind` true: missing ctx (background + // caller that can't reach GitHub), compare API failure/network error, and + // a possibly-truncated compare response (GitHub caps at 300 files) — the + // badge must never miss a real update because the network blinked. + const scope = p.rootDirectory?.trim(); + if ( + behind && + scope && + scope !== "." && + ctx && + p.gitOwner && + p.gitRepo && + deployedSha && + latestSha + ) { + const compare = await compareCommits(ctx, p.gitOwner, p.gitRepo, deployedSha, latestSha).catch( + () => null, + ); + // 0 files means the diff between two different SHAs is empty (rare — + // GitHub's compare response can have a non-empty status but no file + // list). Both that and a non-truncated file list let us decide by + // scoping; a missing response or a hit on the 300-file cap leave + // `behind` as-is so the badge keeps the conservative answer. + if (compare && compare.files.length < 300) { + behind = rootScopeAffected(compare.files, { + rootDirectory: p.rootDirectory, + isMonorepo: p.framework === "monorepo", + monorepoSharedPaths: p.monorepoSharedPaths, + }); + } + } // Is the latest commit already deploying? Then there's nothing to redeploy — // it's in flight, so the nudge is suppressed. Computed live, which is why // pressing Update quiets every surface immediately. diff --git a/apps/api/src/modules/updates/updates.service.ts b/apps/api/src/modules/updates/updates.service.ts index c41dccc1b..62a9ab5a3 100644 --- a/apps/api/src/modules/updates/updates.service.ts +++ b/apps/api/src/modules/updates/updates.service.ts @@ -397,7 +397,7 @@ async function driftItem( ) { const resolved = await upstreamFor(actor, project, row); if (!resolved) return null; - const status: DriftStatus = await evaluateDrift(project, resolved.upstream).catch( + const status: DriftStatus = await evaluateDrift(project, resolved.upstream, actor).catch( () => ({ supported: false }) as DriftStatus, ); const view = presentation(status); @@ -464,7 +464,7 @@ export async function getProjectDrift( const project = await repos.project.findById(projectId); assertResourceInOrg(project, "Project", ctx.organizationId, projectId); if (!hasDeployedSide(project)) return { supported: false }; - return evaluateDrift(project, await pollUpstream(ctx, project)); + return evaluateDrift(project, await pollUpstream(ctx, project), ctx); } // ─── Applying ──────────────────────────────────────────────────────────────── diff --git a/apps/api/test/modules/updates/drift-evaluation.test.ts b/apps/api/test/modules/updates/drift-evaluation.test.ts index 1c5ba3129..a610d7f69 100644 --- a/apps/api/test/modules/updates/drift-evaluation.test.ts +++ b/apps/api/test/modules/updates/drift-evaluation.test.ts @@ -324,3 +324,150 @@ describe("image drift — digests keyed by the ref they were polled for", () => expect(status).toEqual({ supported: false }); }); }); + +/** + * Monorepo scoping (#637): a branch HEAD that moved without touching this + * project's rootDirectory must not offer a redeploy. The upstream half is + * polled; the comparison is `git diff deployedSha..latestSha`, and ANY file + * inside the project's rootDirectory (or a configured shared-path) still + * counts as drift — only a diff that DOES NOT touch this project is hidden. + * + * The compare call is the only place a new network round-trip is required, + * so every test in this block mocks {@link compareCommits} directly. + */ +const compareCommits = vi.hoisted(() => vi.fn()); + +vi.mock("../../../src/modules/github/github.service", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + compareCommits, + }; +}); + +const ctx = { userId: "u_1", organizationId: "org_1", label: "test" } as never; + +describe("commit drift — monorepo root scoping (#637)", () => { + beforeEach(() => { + compareCommits.mockReset(); + }); + + it("hides a diff that is confined to a sibling directory", async () => { + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: ["apps/client/src/ui.tsx"] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: false }); + expect(compareCommits).toHaveBeenCalledWith(ctx, "oblien", "openship", SHIPPED, NEWER); + }); + + it("keeps reporting drift when a file inside the project root changed", async () => { + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: ["apps/backend/src/api.ts"] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + }); + + it("does not false-positive on a similarly-prefixed sibling directory", async () => { + // apps/backend-utils/* must NOT match rootDirectory "apps/backend". + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: ["apps/backend-utils/turbo.json"] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: false }); + }); + + it("flags a repo-root config change as affecting every project", async () => { + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: ["package.json"] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + }); + + it("flags a configured monorepo-shared-path change as affecting every project", async () => { + const p = gitProject({ + rootDirectory: "apps/backend", + framework: "monorepo", + monorepoSharedPaths: ["packages/"], + }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: ["packages/ui/button.tsx"] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + }); + + it("is a no-op for a project with no scoping root", async () => { + // Single-app repo: any change IS drift. The compare call must not even be + // made — there's nothing to scope. + const p = gitProject({ rootDirectory: null }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + expect(compareCommits).not.toHaveBeenCalled(); + }); + + it("is a no-op when caller has no context (background sweep can't reach GitHub)", async () => { + // The badge must not be hidden because the network has dropped out: a + // background sweep that can't poll a compare keeps "behind" as the + // upstream half said it. (Conservative: better over-report than miss a + // real update.) + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), null); + + expect(status).toMatchObject({ behind: true }); + expect(compareCommits).not.toHaveBeenCalled(); + }); + + it("treats a truncated compare (>= 300 files) as still behind the headline", async () => { + // GitHub's compare API caps the file list at 300. Above that we can't + // actually see whether the diff touched the project root, so we keep + // "behind" — the badge must never miss a real update. + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + const files = Array.from({ length: 300 }, (_, i) => `apps/other/file-${i}.ts`); + compareCommits.mockResolvedValue({ files }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + }); + + it("treats a compare failure (network error / rate limit) as still behind", async () => { + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue(null); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: true }); + }); + + it("treats a compare returning an empty file list as not behind", async () => { + // 0 files between two SHAs that differ is the "shouldn't happen, but + // fail-soft" case — the upstream disagree is unbewebbable, so the + // conservative thing is to leave the badge alone. + const p = gitProject({ rootDirectory: "apps/backend" }); + deploymentRepo.findById.mockResolvedValue({ id: "dep_live", commitSha: SHIPPED }); + compareCommits.mockResolvedValue({ files: [] }); + + const status = await evaluateDrift(p, commitUpstream(p, NEWER), ctx); + + expect(status).toMatchObject({ behind: false }); + }); +});