From 2cbcfd2ef3a985612327cfac1ee225f01f57dfe3 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Thu, 16 Jul 2026 15:22:40 +0000 Subject: [PATCH] feat(draft): resolve a repo's GitHub App by source_id, not the only-App guess (#72) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `inventory --emit-draft` wrote the `github_apps` binding by guessing: with exactly one App on the instance it bound every repo to it ("no other it could be"), and with none or several it left a REVIEW marker on all of them. The audit (#72) showed the binding is READABLE, so the guess was both unnecessary and, on a single-App instance, silently WRONG for any public repo (a repo cloned without a GitHub App got bound to the one App anyway). Every application carries the `source_id`/`source_type` of the App that clones it — `removeSensitiveData` hides neither (ApplicationsController v4.1.2) — and `GET /github-apps` returns each App's `id` and `name` (only `client_secret`/`webhook_secret` are hidden). So the draft now matches the two: each repo binds to the App its application's `source_id` names. A GitlabApp/public-repo source (or an instance that will not list its Apps) resolves to nothing and still gets a REVIEW marker — and a `source_id` that collides with an App id but carries a non-GithubApp `source_type` is not mistaken for one. The biggest gain is the multi-App instance the old heuristic could not handle at all: it wrote REVIEW on every repo; the lookup resolves each. semantics.md, the draft header, and the NO_API_COVERAGE row are corrected to match (the audit's #51 arc: a limitation filed as a defect gets fixed). `npm run check` clean · 511 tests pass. Co-Authored-By: Claude Opus 4.8 --- docs/semantics.md | 12 ++++--- src/cli.ts | 20 ++++++----- src/draft.ts | 66 +++++++++++++++++++++++----------- test/draft-cli.test.ts | 23 ++++++++---- test/draft.test.ts | 82 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 165 insertions(+), 38 deletions(-) diff --git a/docs/semantics.md b/docs/semantics.md index b52a35f..04435eb 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -1097,10 +1097,14 @@ registry's own parse-time refusal (*"a registry key has no meaning without its org"*) then stops the file being used until a human supplies it. That refusal is the design: the alternatives are inventing an org, or leaving the project out of the registry — and a project missing from the registry is one every fleet run -skips **in silence**. Likewise `github_apps`: nothing Coolify returns about an -application says which App cloned it, so cast binds every repo to the instance's -only GitHub App when there is exactly one (there is no other it could be), and -writes a `REVIEW-…` marker when there is not. +skips **in silence**. `github_apps`, by contrast, IS readable (#72): an +application carries the `source_id`/`source_type` of the App that clones it +(`removeSensitiveData` hides neither), and `GET /github-apps` returns each App's +`id` and `name` (only `client_secret`/`webhook_secret` are hidden) — so the draft +resolves each repo's App by matching the two, a lookup rather than the only-App +guess it used to make (which was silently wrong for any public repo even on a +single-App instance). A repo whose application has no GithubApp source, or an +instance that will not list its Apps, still gets a `REVIEW-…` marker. ## Teardown (`cast destroy`) diff --git a/src/cli.ts b/src/cli.ts index a752788..61fbdf1 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1867,13 +1867,14 @@ async function main(): Promise { otherEnvironments: others, }); } - // Which GitHub App clones a repo is NOT a property of any resource — no - // field Coolify returns about an application says so. What the instance can - // answer is which Apps exist; with exactly one, there is no other it could - // be. Best-effort: an instance that will not list them still gets a draft, - // with a REVIEW marker where the binding goes. + // Which GitHub App clones a repo IS readable (cast#72): an application + // carries `source_id`/`source_type` (removeSensitiveData hides neither), + // and GET /github-apps returns each App's `id` and `name` (only the + // secrets are hidden) — so bindingsDoc resolves the binding by matching the + // two, instead of guessing the only App. The list is still best-effort: an + // instance that will not answer it leaves a REVIEW marker on every repo. const githubApps = (await client.get("/github-apps").catch(() => [])) as - | Array<{ name?: unknown }> + | Array<{ id?: unknown; name?: unknown }> | undefined; const draftCtx = { env: envName, @@ -1882,8 +1883,11 @@ async function main(): Promise { team, server: sweepBinding.server, githubApps: (Array.isArray(githubApps) ? githubApps : []) - .map((a) => a?.name) - .filter((n): n is string => typeof n === "string"), + .filter( + (a): a is { id: number; name: string } => + typeof a?.id === "number" && typeof a?.name === "string", + ) + .map((a) => ({ id: a.id, name: a.name })), recipient, generatedAt: new Date().toISOString(), }; diff --git a/src/draft.ts b/src/draft.ts index c01b3b0..322c7f6 100644 --- a/src/draft.ts +++ b/src/draft.ts @@ -266,12 +266,13 @@ export type DraftContext = { baseUrl: string; team: { id: number; name: string }; server?: string; - // The GitHub Apps configured on the instance, by name. NOT a property of any - // resource: nothing Coolify returns about an application says which App clones - // it. With exactly one on the instance there is no other it could be, and cast - // binds every repo to it; with none or several it writes a REVIEW marker - // instead of picking. See bindingsDoc. - githubApps?: string[]; + // The GitHub Apps configured on the instance, each with the `id` an + // application's `source_id` points at (cast#72). bindingsDoc resolves a repo's + // App by that match — a READ, not the only-App guess this used to make — and + // falls back to a REVIEW marker only when a resource carries no resolvable + // source (a public repo, or an instance that would not list its Apps). See + // bindingsDoc. + githubApps?: Array<{ id: number; name: string }>; recipient?: string; generatedAt: string; }; @@ -806,7 +807,7 @@ const NO_API_COVERAGE: Array<[string, string]> = [ ], [ "which GitHub App clones a repo", - "nothing Coolify returns about an application says so. cast binds every repo to the instance's only App when there is exactly one, and writes a REVIEW marker when there is not.", + "resolved from the application's `source_id` against GET /github-apps (cast#72) — a lookup, not a guess. A repo whose application has no GithubApp source (a public repo) or an instance that will not list its Apps gets a REVIEW marker instead.", ], [ "anything configured in the UI with no manifest field", @@ -1078,11 +1079,11 @@ export function planDraft( "`projects:`, the registry: the list of what exists, which nothing before a", "whole-instance sweep was able to write down.", "", - "`github_apps` is NOT readable from a box: nothing Coolify returns about an", - `application says which App clones it. This instance has ${ctx.githubApps?.length ?? 0}, so cast`, - ctx.githubApps?.length === 1 - ? `bound every repo to the only one there is (${ctx.githubApps[0]}) — there is no other it could be.` - : "left a REVIEW marker on every repo rather than pick. `apply` will refuse until you fix them.", + "`github_apps` IS readable (cast#72): an application carries the `source_id`", + `of the App that clones it, and this instance lists ${ctx.githubApps?.length ?? 0}, so cast`, + "resolved each repo's App by that match. A repo whose application resolves to", + "none (a public repo, or an unlistable instance) gets a `REVIEW-…` marker;", + "`apply` refuses on those until you fix them.", "", "Do not copy it over a state file you already have. Merge the registry into", "yours, by hand, having decided which of these projects are yours to declare.", @@ -1128,6 +1129,28 @@ function registryKey(p: DraftProject): string { return slug(p.name); } +// The GitHub App a project's repo is cloned by, resolved from an APPLICATION's +// `source_id` against the instance's Apps (cast#72). Coolify hides neither +// `source_id` nor `source_type` on an application, and GET /github-apps returns +// each App's `id` — so this is a lookup, not a guess. Only a GithubApp source +// counts: a public-repo application has a different `source_type` and no App to +// bind, and a numeric `source_id` that happens to collide with an App id must +// not be mistaken for one. The first application that resolves wins; a project +// whose apps resolve to none gets a REVIEW marker (see bindingsDoc). +function githubAppNameForProject( + p: DraftProject, + appById: Map, +): string | undefined { + for (const r of p.resources) { + if (r.kind !== "application") continue; + if (!/github.?app/i.test(String(r.raw.source_type ?? ""))) continue; + const id = r.raw.source_id; + const name = typeof id === "number" ? appById.get(id) : undefined; + if (name) return name; + } + return undefined; +} + // The bindings the box implies — plus `projects:`, THE REGISTRY (#25): the list // of what exists, which nothing before a whole-instance sweep was in a position // to write down. A rebuild cannot even be attempted without it, because you @@ -1135,20 +1158,23 @@ function registryKey(p: DraftProject): string { function bindingsDoc(projects: DraftProject[], ctx: DraftContext) { const registry: Record = {}; const githubApps: Record = {}; - // With exactly one GitHub App on the instance there is no other one an - // application could have been cloned by, so binding every repo to it is a fact, - // not a guess. With none or several it IS a guess, and cast does not make it: - // a wrong App resolves to a real uuid and clones the wrong repo, silently - // (githubAppNameFor, #12). A REVIEW marker resolves to nothing, and `apply` - // says so. - const onlyApp = ctx.githubApps?.length === 1 ? ctx.githubApps[0] : undefined; + // Resolve each repo's App by the read, not a guess (cast#72): an application + // carries the `source_id` of the App that clones it, and ctx.githubApps carries + // each App's `id` and `name`. A guess is what this used to be — binding every + // repo to the only App there was — and it was silently wrong for any public + // repo (source_type is not a GithubApp) even on a single-App instance, and + // unmakeable on a multi-App one. A REVIEW marker resolves to nothing and + // `apply` says so; a wrong App resolves to a real uuid and clones the wrong + // repo, silently (githubAppNameFor, #12). + const appById = new Map((ctx.githubApps ?? []).map((a) => [a.id, a.name])); for (const p of projects) { // Only what the draft actually carries a manifest for. See planDraft. if (p.resources.length === 0) continue; const repo = registryKey(p); registry[repo] = { environments: [ctx.env] }; githubApps[repo] = - onlyApp ?? "REVIEW-which-github-app-in-coolify-clones-this-repo"; + githubAppNameForProject(p, appById) ?? + "REVIEW-which-github-app-in-coolify-clones-this-repo"; } return { environments: { diff --git a/test/draft-cli.test.ts b/test/draft-cli.test.ts index 986f7d1..8d9cb30 100644 --- a/test/draft-cli.test.ts +++ b/test/draft-cli.test.ts @@ -62,10 +62,12 @@ async function stubCoolify(opts: { ambiguous?: boolean } = {}): Promise { res.end(JSON.stringify(body)); }; if (path === "/teams/current") return json({ id: 0, name: "Root Team" }); - // Exactly one App: there is no other one an application could have been - // cloned by, so cast binds every repo to it rather than leave a marker. + // GET /github-apps returns each App's `id` and `name` (cast#72). The + // incubator app below carries `source_id: 7`, so the binding RESOLVES to + // this App by the read; the third-party public repo has no GithubApp source + // and gets a REVIEW marker instead of being wrongly bound to it. if (path === "/github-apps") - return json([{ uuid: "g1", name: "hdb-coolify" }]); + return json([{ id: 7, uuid: "g1", name: "hdb-coolify" }]); if (path === "/projects") return json([ { uuid: "p1", name: "Incubator" }, @@ -95,6 +97,10 @@ async function stubCoolify(opts: { ambiguous?: boolean } = {}): Promise { uuid: "a1", git_repository: "heavy-duty/incubator", git_branch: "main", + // Cloned by the GitHub App id 7 (hdb-coolify) — source_id/source_type + // survive serialization, so the binding is resolvable (cast#72). + source_id: 7, + source_type: "App\\Models\\GithubApp", build_pack: "dockercompose", base_directory: "/", docker_compose_location: "/docker-compose.yaml", @@ -386,10 +392,15 @@ describe("cast inventory --emit-draft (#27)", () => { // The bindings the sweep could actually read. expect(yaml).toContain("server: box-b"); expect(yaml).toContain("name: Root Team"); - // Which GitHub App clones a repo is not on any resource — but this instance - // has exactly one, and there is no other it could be. + // Which GitHub App clones a repo is READ from the application's source_id, + // not guessed (cast#72): the incubator app's source_id: 7 resolves to + // hdb-coolify, while the third-party PUBLIC repo (no GithubApp source) gets a + // REVIEW marker rather than being wrongly bound to the only App there is. expect(yaml).toContain("github_apps:"); - expect(yaml).toContain("third-party/la-familia: hdb-coolify"); + expect(yaml).toContain("heavy-duty/incubator: hdb-coolify"); + expect(yaml).toMatch( + /third-party\/la-familia: REVIEW-which-github-app-in-coolify-clones-this-repo/, + ); // The barber shop has no application, so the box knows no repo for it. cast // writes the bare project name — and the registry's own parse-time refusal diff --git a/test/draft.test.ts b/test/draft.test.ts index a4c2d4c..36933b5 100644 --- a/test/draft.test.ts +++ b/test/draft.test.ts @@ -57,6 +57,88 @@ const project = (over: Partial = {}): DraftProject => ({ ...over, }); +describe("github_apps binding — resolved by source_id, not guessed (cast#72)", () => { + const appProject = ( + name: string, + repo: string, + source?: { source_id: number; source_type: string }, + ): DraftProject => ({ + name, + coolifyEnv: "staging", + resources: [ + { + kind: "application", + name, + uuid: `u-${name}`, + raw: { + git_repository: `https://github.com/${repo}`, + git_branch: "main", + build_pack: "nixpacks", + base_directory: "/", + fqdn: "https://x.example.com", + ...source, + }, + env: {}, + }, + ], + unreadable: [], + otherEnvironments: [], + }); + const bindings = (plan: ReturnType) => + plan.files.find((f) => f.path === "environments.yaml")?.content ?? ""; + + // The payoff the old only-App heuristic could not deliver: with MORE THAN ONE + // App it used to write a REVIEW marker on every repo. source_id resolves each. + it("binds each repo to the App its source_id names, even with several Apps", () => { + const ghApp = "App\\Models\\GithubApp"; + const plan = planDraft( + [ + appProject("acme-api", "acme/api", { + source_id: 7, + source_type: ghApp, + }), + appProject("beta-web", "beta/web", { + source_id: 9, + source_type: ghApp, + }), + ], + { + ...ctx, + githubApps: [ + { id: 7, name: "acme-app" }, + { id: 9, name: "beta-app" }, + ], + }, + ); + const yaml = bindings(plan); + expect(yaml).toContain("acme/api: acme-app"); + expect(yaml).toContain("beta/web: beta-app"); + }); + + it("leaves a REVIEW marker for a public repo (no GithubApp source)", () => { + const plan = planDraft([appProject("pub", "acme/public")], { + ...ctx, + githubApps: [{ id: 7, name: "acme-app" }], + }); + expect(bindings(plan)).toMatch(/acme\/public: REVIEW-/); + }); + + it("does not mistake a non-GithubApp source whose id collides with an App id", () => { + const plan = planDraft( + [ + appProject("gitlab", "acme/gl", { + source_id: 7, + source_type: "App\\Models\\GitlabApp", + }), + ], + { ...ctx, githubApps: [{ id: 7, name: "acme-app" }] }, + ); + // id 7 exists as a GitHub App, but this app's source is a GitlabApp — the + // collision must not bind it to acme-app. + expect(bindings(plan)).toMatch(/acme\/gl: REVIEW-/); + }); +}); + describe("isProviderGenerated — the one judgment that must not be wrong", () => { it("recognizes the datastore families whose value points at the SOURCE box", () => { for (const key of [