From eb51ef532c4bcf2bd881672b93cf8493bc63066f Mon Sep 17 00:00:00 2001 From: Zach Dunn Date: Tue, 18 Aug 2026 19:55:59 -0400 Subject: [PATCH] feat(api): link adoption ledger for a deterministic noise guard (#709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a per-target D1 ledger (github_adopted_links, mirroring the ingest ledger pattern) recording which resolved source keys have been adopted into which PR/issue. The noise guard now counts TOTAL currently-adopted files from the ledger rather than the current pass's copy count, so two links pasted in two separate comments deterministically trip the >=2 threshold on the second one regardless of ordering. Idempotent: a rescan of the same source cheaply skips an already-ledgered, non-detached key (no re-copy). Un-adoption parity with ingest's detach: a rescan of one exact source (body or a single comment) that no longer references a previously-adopted key marks it detached and re-syncs the managed comment; the copy itself is never deleted, only hidden from the render via a gh.detached metadata flag (gatherAttachments now filters on it unconditionally). A re-pasted link un-detaches without a re-copy. A pinning test confirms the two-separate-comments scenario from the issue already synced correctly pre-ledger (the pre-adoption copy already counted as a "pre-existing attachment" via the prefix listing) — kept as regression coverage now that the guard reads the ledger instead. --- .../20260818200000_github_adopted_links.sql | 19 ++ apps/api/src/github-comment.test.ts | 6 +- apps/api/src/github-comment.ts | 30 ++- apps/api/src/github-link-adopt-ledger.ts | 192 ++++++++++++++++++ apps/api/src/github-link-adopt.test.ts | 106 +++++++++- apps/api/src/github-link-adopt.ts | 161 ++++++++++++--- .../github-link-adopt-ledger-sqlite.test.ts | 113 +++++++++++ .../test/helpers/fake-adopt-ledger-table.ts | 137 +++++++++++++ apps/api/test/usage-fake-d1.ts | 13 ++ 9 files changed, 743 insertions(+), 34 deletions(-) create mode 100644 apps/api/migrations/20260818200000_github_adopted_links.sql create mode 100644 apps/api/src/github-link-adopt-ledger.ts create mode 100644 apps/api/test/github-link-adopt-ledger-sqlite.test.ts create mode 100644 apps/api/test/helpers/fake-adopt-ledger-table.ts diff --git a/apps/api/migrations/20260818200000_github_adopted_links.sql b/apps/api/migrations/20260818200000_github_adopted_links.sql new file mode 100644 index 00000000..980859e6 --- /dev/null +++ b/apps/api/migrations/20260818200000_github_adopted_links.sql @@ -0,0 +1,19 @@ +-- Ledger of link-adopted files (issue #709, follow-up to #701/#707). One row +-- per (repo, kind, num, source_key) — the resolved workspace key a pasted +-- uploads.sh link resolved to, scoped to the PR/issue it was adopted into. +-- detached_at NULL == currently referenced by `source` (the body or the +-- specific comment it was last seen in). +CREATE TABLE github_adopted_links ( + repo TEXT NOT NULL, + kind TEXT NOT NULL, + num INTEGER NOT NULL, + source_key TEXT NOT NULL, + workspace TEXT NOT NULL, + object_key TEXT NOT NULL, + source TEXT NOT NULL, + created_at TEXT NOT NULL, + detached_at TEXT, + PRIMARY KEY (repo, kind, num, source_key) +); +CREATE INDEX github_adopted_links_source_idx ON github_adopted_links (repo, kind, num, source); +CREATE INDEX github_adopted_links_target_idx ON github_adopted_links (repo, kind, num, detached_at); diff --git a/apps/api/src/github-comment.test.ts b/apps/api/src/github-comment.test.ts index a1319893..4434008a 100644 --- a/apps/api/src/github-comment.test.ts +++ b/apps/api/src/github-comment.test.ts @@ -271,7 +271,7 @@ describe("gatherCommentBody attachment metadata (issue #365)", () => { expect(result.body).not.toContain("/"); }); - it("skips the D1 read entirely when githubCommentShowMetadata is false", async () => { + it("skips the path/state D1 read when githubCommentShowMetadata is false (issue #709: the gh.detached filter read still runs)", async () => { const { env, ws, workspaceName, bucket } = makeTestEnv(); await seed(env, bucket, { path: "/settings", state: "before" }); @@ -293,7 +293,9 @@ describe("gatherCommentBody attachment metadata (issue #365)", () => { { repo: "acme/web", num: 12, kind: "pull" }, ); - expect(metadataQueries).toBe(0); + // One query: the unconditional gh.detached filter (issue #709) — the + // path/state fetch itself is still skipped, since neither renders here. + expect(metadataQueries).toBe(1); expect(result.body).not.toContain("/settings"); }); diff --git a/apps/api/src/github-comment.ts b/apps/api/src/github-comment.ts index d7834ea2..7ab5a265 100644 --- a/apps/api/src/github-comment.ts +++ b/apps/api/src/github-comment.ts @@ -88,6 +88,16 @@ const COMMENT_META_KEYS = [ "video.height", ]; +/** + * Fetched unconditionally (unlike `COMMENT_META_KEYS`, which is skipped + * entirely when neither `metaPath` nor `metaState` is on): a link-adopted + * copy that's been detached (issue #709 — the source link was edited out of + * the PR/comment it was adopted from) must never render, regardless of the + * repo's meta display settings. The object is never deleted on detach, only + * hidden here. + */ +const DETACH_META_KEY = "gh.detached"; + /** The workspace's own objects under the stable gh key prefix. */ async function gatherAttachments( env: Env, @@ -140,12 +150,26 @@ async function gatherAttachments( return prefixItems; }), ); - const items: AttachmentItem[] = perPrefixItems.flat(); + let items: AttachmentItem[] = perPrefixItems.flat(); + + if (items.length === 0) return items; + + // Detach filter (issue #709) runs unconditionally — D1 rows are + // tenant-scoped by `workspaceName` (the caller's own slug), not by + // anything derived from `ws`, same trust boundary as gatherGalleries. The + // path/state/video.* fetch below is skipped when the repo's meta display + // settings are both off, but this one always runs since a detached copy + // must never render regardless. + const detachByKey = await getMetadataForKeys( + env.DB, + workspaceName, + items.map((item) => item.key), + { metaKeys: [DETACH_META_KEY] }, + ); + items = items.filter((item) => detachByKey.get(item.key)?.[DETACH_META_KEY] !== "true"); if (!showMetadata || items.length === 0) return items; - // D1 rows are tenant-scoped by `workspaceName` (the caller's own slug), not - // by anything derived from `ws` — same trust boundary as gatherGalleries. const metaByKey = await getMetadataForKeys( env.DB, workspaceName, diff --git a/apps/api/src/github-link-adopt-ledger.ts b/apps/api/src/github-link-adopt-ledger.ts new file mode 100644 index 00000000..11ca1044 --- /dev/null +++ b/apps/api/src/github-link-adopt-ledger.ts @@ -0,0 +1,192 @@ +/** + * Ledger of link-adopted files (`github_adopted_links` D1 table), the + * idempotency backbone for `github-link-adopt.ts`'s noise guard — mirrors + * `github-ingest-ledger.ts`'s pattern exactly, scoped by + * (repo, kind, num, sourceKey) instead of (repo, assetId) since a link + * adoption is inherently per-PR/issue (the same source object can be + * independently adopted into more than one target). One row per resolved + * source key; `detachedAt` NULL means the copy is currently referenced by + * `source` (the body or the specific comment it was last seen pasted in). + * When a rescan no longer finds the source key referenced from that exact + * source, the caller sets `detachedAt`; if it later reappears, `detachedAt` + * is cleared back to null rather than inserting a duplicate row — the copy + * itself is never deleted (issue #709: "deleting the copied object is NOT + * required — detach means removed from the managed comment"). + * + * Called from inside the webhook queue consumer, where a D1 failure must + * surface as a thrown error so the queue retries the delivery — nothing here + * swallows errors, same doctrine as the ingest ledger. + */ + +export interface AdoptLedgerRow { + repo: string; // lowercase owner/name + kind: "pull" | "issues"; + num: number; + sourceKey: string; // the resolved workspace key the pasted URL pointed to + workspace: string; + objectKey: string; // the adopted copy's key under the target's gh prefix + source: string; // "body" | "comment:" + createdAt: string; // ISO + detachedAt: string | null; +} + +interface AdoptLedgerDbRow { + repo: string; + kind: "pull" | "issues"; + num: number; + source_key: string; + workspace: string; + object_key: string; + source: string; + created_at: string; + detached_at: string | null; +} + +function normalizeRepo(repo: string): string { + return repo.toLowerCase(); +} + +function fromRow(row: AdoptLedgerDbRow): AdoptLedgerRow { + return { + repo: row.repo, + kind: row.kind, + num: row.num, + sourceKey: row.source_key, + workspace: row.workspace, + objectKey: row.object_key, + source: row.source, + createdAt: row.created_at, + detachedAt: row.detached_at, + }; +} + +const SELECT_COLUMNS = + "repo, kind, num, source_key, workspace, object_key, source, created_at, detached_at"; + +/** + * Records a newly-adopted link. `INSERT OR IGNORE` on the + * (repo, kind, num, source_key) primary key: a duplicate record (the same + * source key re-scanned before its row is read) is a silent no-op rather + * than an overwrite — `setLedgerDetached`/`setLedgerSource` are the explicit + * ways to update an existing row. + */ +export async function recordAdoptedLink( + db: D1Database, + row: Omit, +): Promise { + await db + .prepare( + `INSERT OR IGNORE INTO github_adopted_links + (repo, kind, num, source_key, workspace, object_key, source, created_at, detached_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, NULL)`, + ) + .bind( + normalizeRepo(row.repo), + row.kind, + row.num, + row.sourceKey, + row.workspace, + row.objectKey, + row.source, + row.createdAt, + ) + .run(); +} + +/** The ledger row for a single (repo, kind, num, source key), or null if never recorded. */ +export async function adoptLedgerRow( + db: D1Database, + repo: string, + kind: "pull" | "issues", + num: number, + sourceKey: string, +): Promise { + const row = await db + .prepare( + `SELECT ${SELECT_COLUMNS} FROM github_adopted_links + WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?`, + ) + .bind(normalizeRepo(repo), kind, num, sourceKey) + .first(); + return row ? fromRow(row) : null; +} + +/** All ledger rows for (repo, kind, num) currently attributed to `source` ("body" or "comment:"). */ +export async function adoptLedgerRowsForSource( + db: D1Database, + repo: string, + kind: "pull" | "issues", + num: number, + source: string, +): Promise { + const { results } = await db + .prepare( + `SELECT ${SELECT_COLUMNS} FROM github_adopted_links + WHERE repo = ? AND kind = ? AND num = ? AND source = ?`, + ) + .bind(normalizeRepo(repo), kind, num, source) + .all(); + return (results ?? []).map(fromRow); +} + +/** Every ledger row for (repo, kind, num), across every source — the basis + * for the noise guard's total-adopted count (detached rows excluded by the + * caller, same as ingest's own target-scoped reads). */ +export async function adoptLedgerRowsForTarget( + db: D1Database, + repo: string, + kind: "pull" | "issues", + num: number, +): Promise { + const { results } = await db + .prepare( + `SELECT ${SELECT_COLUMNS} FROM github_adopted_links WHERE repo = ? AND kind = ? AND num = ?`, + ) + .bind(normalizeRepo(repo), kind, num) + .all(); + return (results ?? []).map(fromRow); +} + +/** + * Flips `detachedAt` for a ledger row — set to a timestamp when a rescan no + * longer finds the source key referenced, or back to null when it + * reappears. + */ +export async function setAdoptLedgerDetached( + db: D1Database, + repo: string, + kind: "pull" | "issues", + num: number, + sourceKey: string, + detachedAt: string | null, +): Promise { + await db + .prepare( + `UPDATE github_adopted_links SET detached_at = ? + WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?`, + ) + .bind(detachedAt, normalizeRepo(repo), kind, num, sourceKey) + .run(); +} + +/** + * Moves an already-recorded link between sources (e.g. a comment gets + * edited and the link now lives under a different comment id, or moves from + * a comment into the body). + */ +export async function setAdoptLedgerSource( + db: D1Database, + repo: string, + kind: "pull" | "issues", + num: number, + sourceKey: string, + source: string, +): Promise { + await db + .prepare( + `UPDATE github_adopted_links SET source = ? + WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?`, + ) + .bind(source, normalizeRepo(repo), kind, num, sourceKey) + .run(); +} diff --git a/apps/api/src/github-link-adopt.test.ts b/apps/api/src/github-link-adopt.test.ts index 85677d38..a1bd7181 100644 --- a/apps/api/src/github-link-adopt.test.ts +++ b/apps/api/src/github-link-adopt.test.ts @@ -225,7 +225,7 @@ describe("adoptLinkedFiles", () => { expect(summary.synced).toBe(true); }); - it("resolving to no keys is a pure no-op (no workspace load, no sync)", async () => { + it("resolving to no keys is a no-op when nothing was ever adopted from this source", async () => { const seeded = await seededEnv(); const summary = await adoptLinkedFiles( seeded.env, @@ -234,10 +234,16 @@ describe("adoptLinkedFiles", () => { { repo: REPO, kind: "pull", num: NUM }, "nothing to see here", ); - expect(summary).toEqual({ adopted: [], skipped: [], synced: false }); + expect(summary).toEqual({ + adopted: [], + reattached: [], + detached: [], + skipped: [], + synced: false, + }); }); - it("is idempotent: re-adopting the same source overwrites the same destination key", async () => { + it("is idempotent: re-scanning the same source is a cheap skip, no re-copy (issue #709)", async () => { const seeded = await seededEnv(); await seedSource(seeded, "f/shot.png"); const first = await adoptLinkedFiles( @@ -254,9 +260,101 @@ describe("adoptLinkedFiles", () => { { repo: REPO, kind: "pull", num: NUM }, "https://storage.uploads.sh/acme/f/shot.png", ); - expect(first.adopted).toEqual(second.adopted); + expect(first.adopted).toEqual([`gh/acme/web/pull/${NUM}/shot.png`]); + // Ledgered on the first pass — the second pass finds the row and skips + // re-attaching entirely, rather than re-copying an idempotent overwrite. + expect(second.adopted).toEqual([]); + expect(second.reattached).toEqual([]); + expect(second.detached).toEqual([]); expect(seeded.bucket.store.size).toBe(2); // source + the one dest key, not two dest copies }); + + it("PIN (issue #709): two links pasted in two separate comments each trip the noise guard on the second", async () => { + const seeded = await seededEnv(); + await seedSource(seeded, "f/a.png"); + await seedSource(seeded, "f/b.png"); + seeded.kv.store.set("ghinst:acme/web", { value: "1" }); + + const pass1 = await adoptLinkedFiles( + seeded.env, + WS, + null, + { repo: REPO, kind: "pull", num: NUM }, + "https://storage.uploads.sh/acme/f/a.png", + "comment:1", + ); + // A lone adoption with nothing else staged is noise — no sync yet. + expect(pass1.adopted).toEqual([`gh/acme/web/pull/${NUM}/a.png`]); + expect(pass1.synced).toBe(false); + + const pass2 = await adoptLinkedFiles( + seeded.env, + WS, + null, + { repo: REPO, kind: "pull", num: NUM }, + "https://storage.uploads.sh/acme/f/b.png", + "comment:2", + ); + // The target now has two currently-adopted files total (ledger count), + // deterministically — not just "this pass's own copy count" — so this + // trips the guard even though only one link was scanned this pass. + expect(pass2.adopted).toEqual([`gh/acme/web/pull/${NUM}/b.png`]); + expect(pass2.synced).toBe(true); + }); + + it("un-adopts a link edited out of its source, then re-adopts it without a re-copy on re-paste", async () => { + const seeded = await seededEnv(); + await seedSource(seeded, "f/a.png"); + await seedSource(seeded, "f/b.png"); + seeded.kv.store.set("ghinst:acme/web", { value: "1" }); + + // Both links land in the SAME comment so the target starts at 2 adopted + // (trips the guard), then one gets edited out. + const posted = await adoptLinkedFiles( + seeded.env, + WS, + null, + { repo: REPO, kind: "pull", num: NUM }, + "https://storage.uploads.sh/acme/f/a.png and https://storage.uploads.sh/acme/f/b.png", + "comment:1", + ); + expect(posted.adopted.length).toBe(2); + expect(posted.synced).toBe(true); + + // Edited: the comment now only references b.png — a.png drops out. + const edited = await adoptLinkedFiles( + seeded.env, + WS, + null, + { repo: REPO, kind: "pull", num: NUM }, + "https://storage.uploads.sh/acme/f/b.png", + "comment:1", + ); + expect(edited.adopted).toEqual([]); + expect(edited.detached).toEqual([`gh/acme/web/pull/${NUM}/a.png`]); + expect(edited.synced).toBe(true); // comment re-synced to drop it + + const aMeta = await getFileMetadata(seeded.env.DB, WS, `gh/acme/web/pull/${NUM}/a.png`); + expect(aMeta["gh.detached"]).toBe("true"); + // Deletion is not required — the copy stays in storage. + expect(seeded.bucket.store.has(`${PREFIX}gh/acme/web/pull/${NUM}/a.png`)).toBe(true); + + // Re-pasted: un-detaches without a re-copy (no new attachExistingObject + // call, verified indirectly via a still-untouched source key + the same + // dest key coming back as `reattached`, not `adopted`). + const rePasted = await adoptLinkedFiles( + seeded.env, + WS, + null, + { repo: REPO, kind: "pull", num: NUM }, + "https://storage.uploads.sh/acme/f/a.png and https://storage.uploads.sh/acme/f/b.png", + "comment:1", + ); + expect(rePasted.adopted).toEqual([]); + expect(rePasted.reattached).toEqual([`gh/acme/web/pull/${NUM}/a.png`]); + const aMetaAfter = await getFileMetadata(seeded.env.DB, WS, `gh/acme/web/pull/${NUM}/a.png`); + expect(aMetaAfter["gh.detached"]).toBe("false"); + }); }); describe("adoptLinkedFilesForWebhook", () => { diff --git a/apps/api/src/github-link-adopt.ts b/apps/api/src/github-link-adopt.ts index 3174d2a5..457f0c83 100644 --- a/apps/api/src/github-link-adopt.ts +++ b/apps/api/src/github-link-adopt.ts @@ -35,12 +35,33 @@ * * Noise guard: a lone adopted image that's already visible inline in the * PR/comment doesn't warrant a managed comment repeating it — see - * `shouldSyncAfterAdopt`. + * `shouldSyncAfterAdopt`. The guard's adopted-count comes from the ledger + * (`github-link-adopt-ledger.ts`, issue #709) — the TOTAL non-detached rows + * for this target, not the current pass's copy count — so two links pasted + * in two separate comments correctly trip the threshold on the second one + * regardless of which comment either link landed in. + * + * The ledger is also the idempotency/un-adoption backbone, mirroring + * `github-ingest.ts`'s reconcile shape exactly: a source key already + * ledgered under this exact (target, source) is a cheap skip (no re-copy); + * a source key no longer found when this SOURCE (body or one comment) is + * rescanned is marked detached — the copy is never deleted, only hidden + * from the managed comment render (`gh.detached` metadata, same convention + * ingest uses) — and reappearing un-detaches it without a re-copy. */ import { attachExistingObject, resolveAttachSourceKey } from "./github-attach"; import { commentCacheKey, gatherCommentBody } from "./github-comment"; import type { GhTarget } from "./github-comment-render"; import { postManagedComment } from "./github-comment-service"; +import { setFileMetadata } from "./file-metadata"; +import { + adoptLedgerRow, + adoptLedgerRowsForSource, + adoptLedgerRowsForTarget, + recordAdoptedLink, + setAdoptLedgerDetached, + setAdoptLedgerSource, +} from "./github-link-adopt-ledger"; import { findRepoLinkStrict } from "./github-repo-links"; import { resolveRepoCommentOptions } from "./repo-comment-config"; import { storageConfig } from "./storage"; @@ -63,9 +84,16 @@ export interface AdoptSourceRef { } export interface AdoptSummary { - /** Destination keys copied/refreshed this pass (may already have existed — - * adoption always re-copies, an idempotent overwrite). */ + /** Destination keys newly copied this pass (a ledgered key is a cheap + * skip, never re-copied — see the module doc-comment). */ adopted: string[]; + /** Destination keys whose ledger row un-detached this pass (reappeared + * after having been edited out) — no re-copy, just a metadata/ledger flip. */ + reattached: string[]; + /** Destination keys whose ledger row detached this pass (no longer + * referenced from this exact source) — the copy stays in storage, only + * hidden from the managed comment render. */ + detached: string[]; /** Candidate URLs that looked like uploads.sh links but didn't resolve to * an object in this workspace (wrong workspace, unknown key, deleted). */ skipped: string[]; @@ -136,9 +164,15 @@ export async function resolveAdoptableKeys( * other attachments already staged/attached for this target, and no managed * comment yet, is already fully visible inline — posting a comment that just * repeats it is noise. Sync fires when: - * - two or more links resolved this pass, or - * - at least one resolved AND the target already has other attachments - * (staged/attached/previously-adopted) under its comment prefix, or + * - the target now has two or more currently-adopted links total (ledger + * count, issue #709 — deterministic across passes/sources, not just + * this pass's copy count), or + * - at least one link is adopted AND the target already has other + * attachments (staged/attached/non-ledger-adopted) under its comment + * prefix, or + * - a link detached this pass (the comment needs to drop it, even if that + * leaves fewer than two attachments — an emptying comment is exactly + * the "heal/refresh, don't leave it stale" case below), or * - a managed comment already exists for this target (heal/refresh it * rather than leave it stale). */ @@ -146,11 +180,13 @@ async function shouldSyncAfterAdopt( env: Env, workspaceName: string, target: GhTarget, - adoptedCount: number, + totalAdoptedCount: number, preexistingCount: number, + hadDetach: boolean, ): Promise { - if (adoptedCount === 0) return false; - if (adoptedCount >= 2) return true; + if (hadDetach) return true; + if (totalAdoptedCount === 0) return false; + if (totalAdoptedCount >= 2) return true; if (preexistingCount > 0) return true; const cachedCommentId = await env.GITHUB_CACHE.get(commentCacheKey(workspaceName, target)); @@ -158,13 +194,19 @@ async function shouldSyncAfterAdopt( } /** - * Copy every resolved-key adoption into `target`'s attachment prefix (via - * #702's `attachExistingObject`), then sync the managed comment only when - * `shouldSyncAfterAdopt` says there's something worth consolidating. The - * "does this target already have other attachments" check that feeds the - * noise guard is a `gatherCommentBody` call BEFORE any copy this pass makes, - * so a single lone adoption is judged against the PRE-adoption state, not - * inflated by its own copy. + * Reconciles ONE source (a PR/issue body or one comment) against `text`: + * resolved keys not yet ledgered for this exact (target, source) are copied + * into `target`'s attachment prefix (via #702's `attachExistingObject`) and + * recorded; an already-ledgered, non-detached key is a cheap skip (no + * re-copy); a previously-detached key that's referenced again is + * un-detached without a re-copy; a ledgered, non-detached key for this + * source that's no longer found in `text` is detached. The managed comment + * is synced only when `shouldSyncAfterAdopt` says there's something worth + * consolidating. + * + * `source` is the ledger's source pointer ("body" or "comment:") — + * defaults to "body" for direct callers (tests, and any future non-webhook + * entry point) that don't care about per-comment scoping. */ export async function adoptLinkedFiles( env: Env, @@ -172,24 +214,66 @@ export async function adoptLinkedFiles( mintingUserId: string | null, target: GhTarget, text: string, + source = "body", ): Promise { const { keys, skipped } = await resolveAdoptableKeys(env, workspaceName, text); - const summary: AdoptSummary = { adopted: [], skipped, synced: false }; - if (keys.length === 0) return summary; + const summary: AdoptSummary = { + adopted: [], + reattached: [], + detached: [], + skipped, + synced: false, + }; const ws = await loadWorkspaceRecord(env, workspaceName); if (!ws) return summary; - // Baseline BEFORE this pass's copies land, so a lone adoption isn't judged - // against a prefix count its own copy just inflated. + const db = env.DB; + const { repo, kind, num } = target; + const foundKeys = new Set(keys); + + // Baseline BEFORE this pass's copies land, so a lone FIRST-time adoption + // isn't judged against a prefix count its own copy just inflated. Legacy + // attachments outside the ledger (manual `attach --pr`, pre-#709 adoptions) + // still count here; ledgered adoptions feed the guard separately below. const before = await gatherCommentBody(env, ws, workspaceName, target); for (const key of keys) { + const row = await adoptLedgerRow(db, repo, kind, num, key); + if (row) { + if (row.detachedAt === null) { + // Already adopted and currently attached under some source — cheap + // skip, never re-copies. If the link moved between sources (e.g. an + // edit relocated it from the body into a comment), the ledger's + // source pointer follows. + if (row.source !== source) await setAdoptLedgerSource(db, repo, kind, num, key, source); + continue; + } + // Previously detached, now referenced again — un-detach without a + // re-copy; the object is still in storage untouched. + await setFileMetadata(db, workspaceName, row.objectKey, { "gh.detached": "false" }); + await setAdoptLedgerDetached(db, repo, kind, num, key, null); + if (row.source !== source) await setAdoptLedgerSource(db, repo, kind, num, key, source); + summary.reattached.push(row.objectKey); + continue; + } + try { const result = await attachExistingObject(env, ws, workspaceName, { source: key, target: { repo: target.repo, kind: target.kind, num: target.num }, }); + await setFileMetadata(db, workspaceName, result.key, { "gh.detached": "false" }); + await recordAdoptedLink(db, { + repo, + kind, + num, + sourceKey: key, + workspace: workspaceName, + objectKey: result.key, + source, + createdAt: new Date().toISOString(), + }); summary.adopted.push(result.key); } catch (err) { // Source vanished between resolve and copy (deleted concurrently), or a @@ -207,14 +291,35 @@ export async function adoptLinkedFiles( summary.skipped.push(key); } } - if (summary.adopted.length === 0) return summary; + + // Un-adoption: a ledgered, non-detached key for THIS source that's no + // longer found when this source is rescanned. Scoped by source (not the + // whole target) so a link still referenced from a different comment isn't + // detached just because this particular comment stopped mentioning it. + const existingForSource = await adoptLedgerRowsForSource(db, repo, kind, num, source); + for (const row of existingForSource) { + if (row.detachedAt !== null) continue; + if (foundKeys.has(row.sourceKey)) continue; + await setFileMetadata(db, workspaceName, row.objectKey, { "gh.detached": "true" }); + await setAdoptLedgerDetached(db, repo, kind, num, row.sourceKey, new Date().toISOString()); + summary.detached.push(row.objectKey); + } + + const changed = + summary.adopted.length > 0 || summary.reattached.length > 0 || summary.detached.length > 0; + if (!changed) return summary; + + const totalAdopted = (await adoptLedgerRowsForTarget(db, repo, kind, num)).filter( + (r) => r.detachedAt === null, + ).length; const sync = await shouldSyncAfterAdopt( env, workspaceName, target, - summary.adopted.length, + totalAdopted, before.count, + summary.detached.length > 0, ); if (sync) { await postManagedComment(env, ws, workspaceName, mintingUserId, target, {}); @@ -273,13 +378,19 @@ export async function adoptLinkedFilesForWebhook( if (!token) throw new Error("github installation token mint failed"); const text = await fetchSourceText(fetchImpl, token, ref); - if (text === null || !hasLinkCandidate(text)) return; - + // `text === null` (source deleted, e.g. a comment removed) still needs to + // reconcile — any links previously adopted from it must detach — so it + // scans as empty text rather than short-circuiting. A merely-linkless + // text is the same case (nothing new to adopt, but a prior adoption from + // this exact source may need to detach), so `hasLinkCandidate` is no + // longer a valid early-return here — only a cheap presort in the caller + // that decided `ev.adopt` was worth building at all. await adoptLinkedFiles( env, link.workspaceName, null, { repo: ref.repo, kind: ref.kind, num: ref.num }, - text, + text ?? "", + ref.source, ); } diff --git a/apps/api/test/github-link-adopt-ledger-sqlite.test.ts b/apps/api/test/github-link-adopt-ledger-sqlite.test.ts new file mode 100644 index 00000000..7b992ed8 --- /dev/null +++ b/apps/api/test/github-link-adopt-ledger-sqlite.test.ts @@ -0,0 +1,113 @@ +/// + +import { describe, expect, it } from "vitest"; +import { + adoptLedgerRow, + adoptLedgerRowsForSource, + adoptLedgerRowsForTarget, + recordAdoptedLink, + setAdoptLedgerDetached, + setAdoptLedgerSource, +} from "../src/github-link-adopt-ledger"; +import { database, SqliteD1 } from "./helpers/sqlite-d1"; + +const MIGRATIONS = ["migrations/20260818200000_github_adopted_links.sql"]; + +const row = (over: Partial[1]> = {}) => ({ + repo: "acme/app", + kind: "pull" as const, + num: 7, + sourceKey: "f/shot.png", + workspace: "acme", + objectKey: "gh/acme-app/pull-7/shot.png", + source: "body", + createdAt: "2026-08-18T00:00:00.000Z", + ...over, +}); + +describe("github link adopt ledger", () => { + it("records, reads back, and scopes by source", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const db = database(sqlite); + await recordAdoptedLink(db, row()); + await recordAdoptedLink(db, row({ sourceKey: "f/other.png", source: "comment:44" })); + expect((await adoptLedgerRow(db, "acme/app", "pull", 7, "f/shot.png"))?.objectKey).toBe( + "gh/acme-app/pull-7/shot.png", + ); + expect(await adoptLedgerRowsForSource(db, "acme/app", "pull", 7, "body")).toHaveLength(1); + } finally { + sqlite.close(); + } + }); + + it("detach and re-attach flip detached_at; duplicate record is ignored", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const db = database(sqlite); + await recordAdoptedLink(db, row()); + await recordAdoptedLink(db, row({ objectKey: "gh/other.png" })); // INSERT OR IGNORE + expect((await adoptLedgerRow(db, "acme/app", "pull", 7, "f/shot.png"))?.objectKey).toBe( + "gh/acme-app/pull-7/shot.png", + ); + await setAdoptLedgerDetached( + db, + "acme/app", + "pull", + 7, + "f/shot.png", + "2026-08-19T00:00:00.000Z", + ); + expect( + (await adoptLedgerRow(db, "acme/app", "pull", 7, "f/shot.png"))?.detachedAt, + ).not.toBeNull(); + await setAdoptLedgerDetached(db, "acme/app", "pull", 7, "f/shot.png", null); + expect( + (await adoptLedgerRow(db, "acme/app", "pull", 7, "f/shot.png"))?.detachedAt, + ).toBeNull(); + } finally { + sqlite.close(); + } + }); + + it("adoptLedgerRowsForTarget scopes by repo+kind+num across every source", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const db = database(sqlite); + await recordAdoptedLink(db, row()); // source: "body" + await recordAdoptedLink( + db, + row({ sourceKey: "f/other.png", source: "comment:44", objectKey: "gh/other-comment.png" }), + ); + await recordAdoptedLink( + db, + row({ sourceKey: "f/y.png", num: 8, objectKey: "gh/other-num.png" }), + ); // different num — must not be included + await recordAdoptedLink( + db, + row({ sourceKey: "f/z.png", kind: "issues", objectKey: "gh/other-kind.png" }), + ); // different kind — must not be included + + const rows = await adoptLedgerRowsForTarget(db, "acme/app", "pull", 7); + expect(rows).toHaveLength(2); + expect(new Set(rows.map((r) => r.sourceKey))).toEqual(new Set(["f/shot.png", "f/other.png"])); + } finally { + sqlite.close(); + } + }); + + it("setAdoptLedgerSource moves a link between sources", async () => { + const sqlite = new SqliteD1(MIGRATIONS); + try { + const db = database(sqlite); + await recordAdoptedLink(db, row()); + await setAdoptLedgerSource(db, "acme/app", "pull", 7, "f/shot.png", "comment:44"); + expect(await adoptLedgerRowsForSource(db, "acme/app", "pull", 7, "body")).toHaveLength(0); + expect(await adoptLedgerRowsForSource(db, "acme/app", "pull", 7, "comment:44")).toHaveLength( + 1, + ); + } finally { + sqlite.close(); + } + }); +}); diff --git a/apps/api/test/helpers/fake-adopt-ledger-table.ts b/apps/api/test/helpers/fake-adopt-ledger-table.ts new file mode 100644 index 00000000..840fbbc6 --- /dev/null +++ b/apps/api/test/helpers/fake-adopt-ledger-table.ts @@ -0,0 +1,137 @@ +/** + * Shared in-memory `github_adopted_links` table backing for route/queue + * tests (usage-fake-d1.ts) — mirrors the real D1 semantics (INSERT OR + * IGNORE keyed on (repo, kind, num, source_key); UPDATE for detach/source) + * without a full sqlite-backed D1. Structurally a twin of + * fake-ingest-ledger-table.ts, scoped by target instead of asset id. + */ + +export interface AdoptLedgerDbRow { + repo: string; + kind: "pull" | "issues"; + num: number; + source_key: string; + workspace: string; + object_key: string; + source: string; + created_at: string; + detached_at: string | null; +} + +export interface FakeRunResult { + success: true; + meta: { changes: number }; + results: []; +} + +export interface FakeAllResult { + success: true; + results: T[]; + meta: Record; +} + +function key(repo: string, kind: string, num: number, sourceKey: string): string { + return `${repo}::${kind}::${num}::${sourceKey}`; +} + +export class AdoptLedgerTable { + readonly rows = new Map(); + + tryRun(normalizedSql: string, args: unknown[]): FakeRunResult | undefined { + if (normalizedSql.startsWith("INSERT OR IGNORE INTO github_adopted_links")) { + const [repo, kind, num, sourceKey, workspace, objectKey, source, createdAt] = args as [ + string, + "pull" | "issues", + number, + string, + string, + string, + string, + string, + ]; + const k = key(repo, kind, num, sourceKey); + if (this.rows.has(k)) return { success: true, meta: { changes: 0 }, results: [] }; + this.rows.set(k, { + repo, + kind, + num, + source_key: sourceKey, + workspace, + object_key: objectKey, + source, + created_at: createdAt, + detached_at: null, + }); + return { success: true, meta: { changes: 1 }, results: [] }; + } + if ( + normalizedSql.startsWith("UPDATE github_adopted_links SET detached_at = ?") && + normalizedSql.includes("WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?") + ) { + const [detachedAt, repo, kind, num, sourceKey] = args as [ + string | null, + string, + "pull" | "issues", + number, + string, + ]; + const row = this.rows.get(key(repo, kind, num, sourceKey)); + if (!row) return { success: true, meta: { changes: 0 }, results: [] }; + row.detached_at = detachedAt; + return { success: true, meta: { changes: 1 }, results: [] }; + } + if ( + normalizedSql.startsWith("UPDATE github_adopted_links SET source = ?") && + normalizedSql.includes("WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?") + ) { + const [source, repo, kind, num, sourceKey] = args as [ + string, + string, + "pull" | "issues", + number, + string, + ]; + const row = this.rows.get(key(repo, kind, num, sourceKey)); + if (!row) return { success: true, meta: { changes: 0 }, results: [] }; + row.source = source; + return { success: true, meta: { changes: 1 }, results: [] }; + } + return undefined; + } + + tryFirst(normalizedSql: string, args: unknown[]): T | null | undefined { + if ( + normalizedSql.includes( + "FROM github_adopted_links WHERE repo = ? AND kind = ? AND num = ? AND source_key = ?", + ) + ) { + const [repo, kind, num, sourceKey] = args as [string, "pull" | "issues", number, string]; + return (this.rows.get(key(repo, kind, num, sourceKey)) as T) ?? null; + } + return undefined; + } + + tryAll(normalizedSql: string, args: unknown[]): FakeAllResult | undefined { + if ( + normalizedSql.includes( + "FROM github_adopted_links WHERE repo = ? AND kind = ? AND num = ? AND source = ?", + ) + ) { + const [repo, kind, num, source] = args as [string, "pull" | "issues", number, string]; + const results = [...this.rows.values()].filter( + (row) => row.repo === repo && row.kind === kind && row.num === num && row.source === source, + ); + return { success: true, results: results as T[], meta: {} }; + } + if ( + normalizedSql.includes("FROM github_adopted_links WHERE repo = ? AND kind = ? AND num = ?") + ) { + const [repo, kind, num] = args as [string, "pull" | "issues", number]; + const results = [...this.rows.values()].filter( + (row) => row.repo === repo && row.kind === kind && row.num === num, + ); + return { success: true, results: results as T[], meta: {} }; + } + return undefined; + } +} diff --git a/apps/api/test/usage-fake-d1.ts b/apps/api/test/usage-fake-d1.ts index 0b6c86b7..395b41bf 100644 --- a/apps/api/test/usage-fake-d1.ts +++ b/apps/api/test/usage-fake-d1.ts @@ -3,6 +3,7 @@ * and optional no-op auth_tokens lookups for route tests. */ +import { AdoptLedgerTable } from "./helpers/fake-adopt-ledger-table"; import { DeleteUsageClaimsTable } from "./helpers/fake-delete-usage-claims-table"; import { FileMetadataTable } from "./helpers/fake-file-metadata-table"; import { IngestLedgerTable } from "./helpers/fake-ingest-ledger-table"; @@ -57,6 +58,12 @@ export class UsageFakeD1 { get privatePrefixes() { return this.privatePrefixesTable.rows; } + // Backs `github_adopted_links` — the link-adoption noise guard's + // idempotency backbone (issue #709). + private adoptLedgerTable = new AdoptLedgerTable(); + get adoptLedger() { + return this.adoptLedgerTable.rows; + } prepare = (sql: string) => { const normalized = sql.replace(/\s+/g, " ").trim(); @@ -78,6 +85,8 @@ export class UsageFakeD1 { if (ledgerFirstResult !== undefined) return ledgerFirstResult; const prefixFirstResult = this.privatePrefixesTable.tryFirst(normalized, values); if (prefixFirstResult !== undefined) return prefixFirstResult; + const adoptFirstResult = this.adoptLedgerTable.tryFirst(normalized, values); + if (adoptFirstResult !== undefined) return adoptFirstResult; throw new Error(`unsupported first: ${normalized}`); }, all: async () => { @@ -91,6 +100,8 @@ export class UsageFakeD1 { if (ledgerAllResult) return ledgerAllResult; const prefixAllResult = this.privatePrefixesTable.tryAll(normalized, values); if (prefixAllResult) return prefixAllResult; + const adoptAllResult = this.adoptLedgerTable.tryAll(normalized, values); + if (adoptAllResult) return adoptAllResult; // Galleries aren't modeled by this fake (route/gallery-specific tests // bring their own D1 stand-in) — an empty page is a safe, honest // default for callers (e.g. the webhook auto-promote gather) that @@ -111,6 +122,8 @@ export class UsageFakeD1 { if (ledgerRunResult) return ledgerRunResult; const prefixRunResult = this.privatePrefixesTable.tryRun(normalized, values); if (prefixRunResult) return prefixRunResult; + const adoptRunResult = this.adoptLedgerTable.tryRun(normalized, values); + if (adoptRunResult) return adoptRunResult; const claimResult = this.deleteUsageClaimsTable.tryRun(normalized, values); if (claimResult) return claimResult; if (normalized.startsWith("INSERT OR IGNORE INTO workspace_usage")) {