From f77c1ddf4ddc5bc9f1a34adb3c67011b0dde991d Mon Sep 17 00:00:00 2001 From: "codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie)" Date: Thu, 30 Jul 2026 21:24:10 +0000 Subject: [PATCH] verify tangled pull page links Co-Authored-By: codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie) --- docs/adr-tangled-forge.md | 32 +++-- docs/design.md | 2 +- docs/radial-json.md | 7 +- docs/running-an-agent.md | 6 +- packages/daemon/src/cli.ts | 1 + packages/daemon/src/config.ts | 4 + packages/daemon/src/forge-tangled.ts | 136 +++++--------------- packages/daemon/src/forge.ts | 2 +- packages/daemon/src/index.ts | 1 + packages/daemon/src/merge-poll.ts | 37 +++++- packages/daemon/src/tangled-pages.ts | 53 ++++++++ packages/daemon/test/config.test.mjs | 4 +- packages/daemon/test/forge-tangled.test.mjs | 62 +++++---- packages/daemon/test/merge-poll.test.mjs | 40 +++++- 14 files changed, 220 insertions(+), 167 deletions(-) create mode 100644 packages/daemon/src/tangled-pages.ts diff --git a/docs/adr-tangled-forge.md b/docs/adr-tangled-forge.md index dab15d1..05f9635 100644 --- a/docs/adr-tangled-forge.md +++ b/docs/adr-tangled-forge.md @@ -210,26 +210,24 @@ Either way a read failure **throws** — `MergePoller` treats a throw as "back o Deliberately not done for **state polling**: gating on `sh.tangled.bobbin.getCoverage`. A stale index reports an older state, which for merge polling costs latency and nothing else — the next poll picks -it up — and a second request per poll to find that out is not worth it. D3 does gate display-number -derivation, where an incomplete position could link a different pull and is therefore not harmless. +it up — and a second request per poll to find that out is not worth it. -### D3 — `links.pr` carries the direct appview page when Bobbin can derive it +### D3 — revised again: a pull number must be proved, never counted The human-readable `https://tangled.org///pulls/` carries a number the *web* -appview assigns from creation order; it is in no record. Bobbin intentionally returns record rkeys -instead, but its own documentation gives the bridge: a client wanting a display number derives it -from creation order. `sh.tangled.repo.listPulls?subject=&order=asc` is that order. Radial -walks it to the pull's rkey and writes the one-based position as the direct page URL. The inverse -walk resolves a page URL back to its pull record, so merge polling and v2 branch reuse still operate -on the stable at-uri underneath. - -Creation-order positions are only safe from a complete index. Unlike state polling (D2), this path -therefore checks `sh.tangled.bobbin.getCoverage`: while Bobbin is warming, its own docs call list -counts lower bounds, and using one could link somebody else's pull. An unavailable, disabled, or -warming Bobbin leaves `links.pr` as the pull record's stable `at://` URI. The UI links that fallback -to the repository's pull list, exactly as it did before this decision changed. A just-written pull -may not appear in an otherwise-ready index yet; because Radial has its `createdAt`, it inserts that -one record into the complete creation order locally instead of waiting for firehose propagation. +appview assigns from database state; it is in no record and is not recoverable from record creation +order. Live evidence disproved the earlier rule: Bobbin reported `ready: true` at +136338/160175 events while returning an empty repository list, and large repositories showed page +numbers diverging sharply from list rank (including records absent from the index). Deletions also +leave gaps in appview numbering. Counting can therefore produce a valid page for somebody else's +pull. + +Radial now publishes a numbered URL only when the web page itself names the exact +`sh.tangled.repo.pull` at-uri. The reverse direction applies the same proof. The newest pull-list +entries are searched under a small, logged budget; failures, markup changes, and HTTP-200 404 pages +all fail closed to the stable at-uri. `pageLinks: false` disables the HTML dependency. Merge polling +also compares the reported head branch and, when available, repository identity before writing an +annotation, making legacy wrong links harmless. Consequence: `format: uri` validation in `core` had to accept an at-uri. It is an absolute URI by RFC 3986; WHATWG `new URL` refuses it only because the DID's colons parse as a port. **No lexicon diff --git a/docs/design.md b/docs/design.md index 6ccb3fd..374d4af 100644 --- a/docs/design.md +++ b/docs/design.md @@ -220,7 +220,7 @@ The adapter is daemon-side (it holds the operator's observation credentials); wh | how a repo is named | `owner/repo` | the repository's **own DID** — not its owner's; `at:///sh.tangled.repo/`'s `repoDid` maps one to the other, and the repo DID resolves to a knot that holds no atproto records | | how a turn pushes | https + `GH_TOKEN` | **ssh only**, with an ed25519 key registered to the agent's DID as an `sh.tangled.publicKey` record | | what a pull request IS | a page | an `sh.tangled.repo.pull` **record** in the author's own repo, with an append-only `rounds` array of gzipped `git format-patch` blobs | -| what `links.pr` carries | `…/pull/` | normally `…/pulls/`: the adapter derives the web appview's display number from Bobbin's repository creation order. If Bobbin is disabled, unavailable, or still warming, the stable pull-record `at://` URI remains the safe fallback | +| what `links.pr` carries | `…/pull/` | a verified `…/pulls/` only when that web page names the pull's exact at-uri; otherwise the stable pull-record `at://` URI | | pull state | the API | the newest `sh.tangled.repo.pull.status` record naming that pull — folded by **Bobbin**, tangled's public read-only XRPC appview (`https://api.tangled.org`), because the status lives in the repo of whoever ruled on it. Bobbin's `open` means "no ruling in my index", so it is corroborated against the two repos Radial can name; a `merged` or `closed` stands | | v2 of an implementation | push, `gh pr edit` | push, **append a round** to the same record | | who opens the pull request | the turn, with `gh` | the **daemon**, at submit time | diff --git a/docs/radial-json.md b/docs/radial-json.md index c4e240a..c77cc85 100644 --- a/docs/radial-json.md +++ b/docs/radial-json.md @@ -394,6 +394,7 @@ the sidecar socket, never from what it printed. | --- | --- | --- | | `hosts` | `["tangled.org"]` | The appview/knot hosts this adapter speaks for. A self-hosted knot adds its own; the default list is replaced, not extended. | | `api` | `"https://api.tangled.org"` | The [Bobbin](https://docs.tangled.org/bobbin) instance pull state is read from — tangled's read-only, unauthenticated XRPC appview. Point it at your own if you host one. `false` turns it off, and the daemon folds `sh.tangled.repo.pull.status` records off PDSes itself: the pull's author and the repository's **owner**, both ordinary accounts with ordinary PDSes. That is a working deployment, with one honest limit — a merge performed by a third-party collaborator lands in *their* repo, which only an appview folds. The same fold also runs whenever the appview is unreachable, has not indexed a pull yet, or reports `open` (which means "no ruling in my index", not "nobody ruled"), so leaving `api` unset costs nothing. | +| `pageLinks` | `true` | Read the web appview to verify numbered pull-page links. A number is published only when its page names the exact pull-record at-uri. `false` avoids HTML reads entirely: artifacts carry at-uris and existing numbered links are not resolvable. | | `knownHosts` | unset | `ssh-keyscan` output for those hosts, pinned. **Required before an implementation turn can push.** Radial never falls back to `StrictHostKeyChecking=no` — an unpinned push is a MITM-able push. Observation alone needs none of this. | Three things a tangled project needs that a GitHub one does not: @@ -432,9 +433,9 @@ you know it took. The project's `gitUrl` is the browse/clone URL (`https://tangled.org/@owner/repo`); the ssh push remote is derived from it and handed to the turn as `$RADIAL_PUSH_REMOTE`. On this forge the daemon opens the pull request itself — tangled ships no CLI a container could use — so `links.pr` carries -the direct web-appview page Bobbin derives from repository creation order. If Bobbin is disabled, -unavailable, or warming, it carries the stable `at://` URI of the `sh.tangled.repo.pull` record -instead (design §10, `docs/adr-tangled-forge.md`). +a direct web-appview page only after that page proves it names the exact pull-record at-uri. Any +miss or appview failure carries the stable `at://` URI instead (design §10, +`docs/adr-tangled-forge.md`). ## `artifactTypes` and the registry diff --git a/docs/running-an-agent.md b/docs/running-an-agent.md index 26ee2b1..97a7b3f 100644 --- a/docs/running-an-agent.md +++ b/docs/running-an-agent.md @@ -288,9 +288,9 @@ walkthrough. Two differences are visible in the records. First, a tangled turn pushes over ssh and does *not* open its own pull request — Radial writes the `sh.tangled.repo.pull` record itself, as the same identity that signs the artifact, because tangled ships no CLI a container could use. `links.pr` -therefore carries that record's direct `https://…/pulls/` page when Bobbin -can derive the web appview's number, with the stable `at://` URI as the -fallback while Bobbin is disabled, unavailable, or warming. Second, a second +therefore carries that record's direct `https://…/pulls/` page only when +the page proves it names the record's exact at-uri, with that stable at-uri as +the fallback on any miss. Second, a second version appends a *round* to the same pull record instead of opening a new one. Merges are observed through [Bobbin](https://docs.tangled.org/bobbin), tangled's public read-only API appview, which needs no credential and no diff --git a/packages/daemon/src/cli.ts b/packages/daemon/src/cli.ts index 524e97b..69577ad 100644 --- a/packages/daemon/src/cli.ts +++ b/packages/daemon/src/cli.ts @@ -994,6 +994,7 @@ async function runCommand(args: string[]): Promise { const tangled = new TangledForge({ reader: new FetchRepoTransport(), ...(bobbin ? { bobbin } : {}), + pageFetch: configured.pageLinks === false ? false : fetch, ...(configured.hosts ? { hosts: configured.hosts } : {}), resolveHandle: (handle) => resolveHandleDid(handle, { resolveTxt: nodeResolveTxt }), actor: () => { diff --git a/packages/daemon/src/config.ts b/packages/daemon/src/config.ts index 38f7a2b..58276c9 100644 --- a/packages/daemon/src/config.ts +++ b/packages/daemon/src/config.ts @@ -141,6 +141,8 @@ export type ForgeConfig = * `false` turns it off, and the adapter folds `sh.tangled.repo.pull.status` records off PDSes * itself (correct, but it can only scan the DIDs it can guess). */ api?: string | false + /** Verify direct pull-page links through the web appview. Default true; false uses at-URIs. */ + pageLinks?: boolean /** `ssh-keyscan` lines for those hosts, pinned. Required before an implementation turn can * push: Radial will not fall back to `StrictHostKeyChecking=no`, which would make every push * MITM-able (design §13). */ @@ -266,10 +268,12 @@ function forgeValue(value: unknown, where: string): ForgeConfig | undefined { const hosts = strings(value.hosts, `${where}.hosts`) const knownHosts = strings(value.knownHosts, `${where}.knownHosts`) const api = appviewValue(value.api, `${where}.api`) + const pageLinks = bool(value.pageLinks, `${where}.pageLinks`) return { kind: 'tangled', ...(hosts !== undefined ? { hosts } : {}), ...(api !== undefined ? { api } : {}), + ...(pageLinks !== undefined ? { pageLinks } : {}), ...(knownHosts !== undefined ? { knownHosts } : {}), } } diff --git a/packages/daemon/src/forge-tangled.ts b/packages/daemon/src/forge-tangled.ts index e86e5f9..31820bd 100644 --- a/packages/daemon/src/forge-tangled.ts +++ b/packages/daemon/src/forge-tangled.ts @@ -38,6 +38,7 @@ import type { ForeignRecordReader } from '@radial/atproto' import type { StrongRef } from '@radial/core' import type { BobbinPull, BobbinReader } from './bobbin.js' import { defaultGitExec, type GitExec } from './bundle-writer.js' +import { numberForPull, pullAtNumber, type PageFetch } from './tangled-pages.js' import type { ForgeAdapter, OpenPullInput, @@ -63,8 +64,6 @@ const PATCH_CONTENT_TYPE = 'application/gzip' * asking about is recent; this only bounds a repo with an enormous unrelated history. */ const STATUS_PAGE_LIMIT = 5 const PAGE_SIZE = 100 -const DISPLAY_PAGE_SIZE = 1000 -const DISPLAY_PAGE_LIMIT = 100 const object = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value) @@ -274,6 +273,8 @@ export interface TangledForgeOptions { * `sh.tangled.repo.pull.status` records off PDSes itself, which can only scan the DIDs it is able * to guess, and cannot scan a DID whose `#atproto_pds` is a knot (see `getPullRequestState`). */ bobbin?: BobbinReader + /** Fetches the web appview for exact page ↔ pull-record mapping. False disables page links. */ + pageFetch?: PageFetch | false /** Hosts this adapter speaks for. Default `tangled.org`; a self-hosted knot adds its own. */ hosts?: readonly string[] /** handle → DID. Needed because a tangled URL names an owner by handle while a pull record names @@ -310,6 +311,8 @@ export class TangledForge implements ForgeAdapter { readonly #options: TangledForgeOptions readonly #didCache = new Map() readonly #repoCache = new Map() + readonly #pageByPull = new Map() + readonly #pullByPage = new Map() constructor(options: TangledForgeOptions) { this.#options = options @@ -406,51 +409,32 @@ export class TangledForge implements ForgeAdapter { return did } - /** The record behind either spelling `links.pr` may carry. Tangled's web appview derives its - * display number from repository creation order; Bobbin exposes that same order with rkeys, so a - * page number can be translated back without scraping HTML. */ + /** The record behind either spelling `links.pr` may carry. An appview number exists only in the + * page database, so a page URL is accepted only when that page proves the record URI it names. */ async #pullAtUri(prUrl: string): Promise<{ did: string; rkey: string; uri: string }> { const atUri = parsePullAtUri(prUrl) if (atUri) return { ...atUri, uri: prUrl } const page = parseTangledPullUrl(prUrl, this.hosts) if (page) { - const repo = (await this.#repoIdentity(page.location)).repo - const uri = await this.#pullUriAtNumber(repo, page.number) + const cached = this.#pullByPage.get(prUrl) + const fetchPage = this.#options.pageFetch + const uri = cached ?? (fetchPage ? await pullAtNumber(fetchPage, page.location, page.number) : undefined) if (uri) { const parsed = parsePullAtUri(uri) - if (parsed) return { ...parsed, uri } + if (parsed) { + this.#pullByPage.set(prUrl, uri) + this.#pageByPull.set(uri, prUrl) + return { ...parsed, uri } + } } throw new Error( `cannot resolve ${prUrl} to a sh.tangled.repo.pull record: an appview page URL carries an ` + - 'appview-assigned number, and Bobbin could not map that creation-order position to a pull rkey', + 'appview-assigned number and the page did not prove which pull record it names', ) } throw new Error(`not a tangled pull request URL: ${prUrl}`) } - /** The pull at a web-appview display position. Bobbin deliberately serves rkeys instead of - * sequential IDs, but its ascending repository list is the creation order the web appview uses. - * Coverage must be ready: while Bobbin is warming, list counts are documented lower bounds and a - * position from them could silently name the wrong pull. */ - async #pullUriAtNumber(repo: string, number: number): Promise { - const bobbin = this.#options.bobbin - if (!bobbin || !(await this.#bobbinReady(`pull #${number} in ${repo}`))) return undefined - let seen = 0 - let cursor: string | undefined - for (let page = 0; page < DISPLAY_PAGE_LIMIT; page += 1) { - const result = await bobbin.listPulls({ - did: repo, - order: 'asc', - limit: DISPLAY_PAGE_SIZE, - ...(cursor ? { cursor } : {}), - }) - if (number <= seen + result.items.length) return result.items[number - seen - 1]?.uri - seen += result.items.length - cursor = result.cursor - if (!cursor || result.items.length === 0) break - } - return undefined - } /** The pull record alone, for a question that does not need its state. Bobbin's `getPull` is a * live fetch rather than an index read, so it is exact even for a record written seconds ago; the @@ -598,81 +582,26 @@ export class TangledForge implements ForgeAdapter { ) } - async #bobbinReady(subject: string): Promise { - const bobbin = this.#options.bobbin - if (!bobbin) return false - try { - const coverage = await bobbin.getCoverage() - if (coverage.ready) return true - this.#options.log?.( - `cannot derive the tangled web-appview number for ${subject} while Bobbin is warming up ` + - `(${coverage.eventsProcessed}/${coverage.lastCursor} events replayed)`, - ) - } catch (error) { - this.#options.log?.( - `cannot derive the tangled web-appview number for ${subject}: ` + - `${error instanceof Error ? error.message : String(error)}`, - ) - } - return false - } - - /** A direct page for this pull, derived from its creation-order position in Bobbin. If the pull - * is newer than Bobbin's index, insert it into the complete order by `createdAt`; this avoids - * making artifact submission wait for firehose propagation. Any incomplete/unavailable index - * falls back to the stable at-uri instead of risking a link to somebody else's pull. */ - async #pullPageUrl( - location: TangledLocation, - repo: string, - uri: string, - createdAt: string | undefined, - ): Promise { - const bobbin = this.#options.bobbin - if (!bobbin || !(await this.#bobbinReady(uri))) return undefined - const targetTime = createdAt ? Date.parse(createdAt) : Number.NaN - let before = 0 - let cursor: string | undefined + async #pullPageUrl(location: TangledLocation, uri: string): Promise { + const cached = this.#pageByPull.get(uri) + if (cached) return cached + const fetchPage = this.#options.pageFetch + if (!fetchPage) return undefined try { - for (let page = 0; page < DISPLAY_PAGE_LIMIT; page += 1) { - const result = await bobbin.listPulls({ - did: repo, - order: 'asc', - limit: DISPLAY_PAGE_SIZE, - ...(cursor ? { cursor } : {}), - }) - for (const item of result.items) { - if (item.uri === uri) return this.#pullPage(location, before + 1) - const value = object(item.value) ? item.value : {} - const itemTime = Date.parse(string(value.createdAt) ?? '') - if ( - Number.isFinite(targetTime) && - Number.isFinite(itemTime) && - (targetTime < itemTime || (targetTime === itemTime && uri < item.uri)) - ) { - return this.#pullPage(location, before + 1) - } - before += 1 - } - cursor = result.cursor - if (!cursor || result.items.length === 0) { - return Number.isFinite(targetTime) ? this.#pullPage(location, before + 1) : undefined - } + const page = await numberForPull(fetchPage, location, uri, { + ...(this.#options.log ? { log: this.#options.log } : {}), + }) + if (page) { + this.#pageByPull.set(uri, page) + this.#pullByPage.set(page, uri) } - this.#options.log?.( - `cannot derive the tangled web-appview number for ${uri}: more than ` + - `${DISPLAY_PAGE_LIMIT * DISPLAY_PAGE_SIZE} pulls precede it`, - ) + return page } catch (error) { this.#options.log?.( - `cannot derive the tangled web-appview number for ${uri}: ` + - `${error instanceof Error ? error.message : String(error)}`, + `cannot verify a tangled web-appview page for ${uri}: ${error instanceof Error ? error.message : String(error)}`, ) + return undefined } - return undefined - } - - #pullPage(location: TangledLocation, number: number): string { - return `https://${location.host}/${location.owner}/${location.repo}/pulls/${number}` } /** The repository owner behind a project's remote, for the status scan — or undefined when there @@ -833,9 +762,8 @@ export class TangledForge implements ForgeAdapter { { swapRecord: existing.cid }, ) this.#options.log?.(`appended round ${rounds.length} to tangled pull ${existing.uri}`) - const createdAt = object(existing.value) ? string(existing.value.createdAt) : undefined return { - url: (await this.#pullPageUrl(location, targetRepo, existing.uri, createdAt)) ?? existing.uri, + url: (await this.#pullPageUrl(location, existing.uri)) ?? existing.uri, record: ref, } } @@ -856,7 +784,7 @@ export class TangledForge implements ForgeAdapter { const ref = await actor.createForeign(TANGLED_PULL, record) this.#options.log?.(`opened tangled pull ${ref.uri} against ${targetRepo}#${input.base}`) return { - url: (await this.#pullPageUrl(location, targetRepo, ref.uri, now)) ?? ref.uri, + url: (await this.#pullPageUrl(location, ref.uri)) ?? ref.uri, record: ref, } } diff --git a/packages/daemon/src/forge.ts b/packages/daemon/src/forge.ts index 73b2507..8316dd1 100644 --- a/packages/daemon/src/forge.ts +++ b/packages/daemon/src/forge.ts @@ -62,7 +62,7 @@ export interface OpenPullInput { export interface OpenedPull { /** What lands in `artifact.links.pr`. A `format: uri` string — an `at://` record URI is as valid * a value there as an `https://` page. Tangled normally derives the web page through Bobbin and - * falls back to the record URI when that index cannot safely supply a display number. */ + * falls back to the record URI unless the web page itself proves its display number. */ url: string /** The pull record itself, when the adapter wrote one. Persisted on the turn ledger so a retry * after a crash between the record write and the artifact write finds it without a list. */ diff --git a/packages/daemon/src/index.ts b/packages/daemon/src/index.ts index 50ddcf8..68a5dcb 100644 --- a/packages/daemon/src/index.ts +++ b/packages/daemon/src/index.ts @@ -15,6 +15,7 @@ export * from './forge.js' export * from './forge-auth.js' export * from './forge-github.js' export * from './forge-tangled.js' +export * from './tangled-pages.js' export * from './github-auth.js' export * from './harness.js' export * from './init.js' diff --git a/packages/daemon/src/merge-poll.ts b/packages/daemon/src/merge-poll.ts index 623dbd3..ff030b0 100644 --- a/packages/daemon/src/merge-poll.ts +++ b/packages/daemon/src/merge-poll.ts @@ -22,7 +22,8 @@ export interface ForgeStateSource { getPullRequestState( prUrl: string, context?: { gitUrl?: string }, - ): Promise<{ state: 'open' | 'merged' | 'closed'; mergedAt?: string }> + ): Promise<{ state: 'open' | 'merged' | 'closed'; mergedAt?: string; headRef?: string; headRepoFullName?: string }> + projectIdentity?(gitUrl: string): Promise } export interface MergeCandidate { @@ -200,7 +201,12 @@ export class MergePoller { async #poll(candidate: MergeCandidate): Promise { const { artifact, prUrl, actor } = candidate - let result: { state: 'open' | 'merged' | 'closed'; mergedAt?: string } + let result: { + state: 'open' | 'merged' | 'closed' + mergedAt?: string + headRef?: string + headRepoFullName?: string + } try { result = await this.#deps.adapter.getPullRequestState( prUrl, @@ -220,6 +226,33 @@ export class MergePoller { return } + const branch = artifact.value.links?.branch + if (branch && result.headRef !== branch) { + this.#deps.log?.( + `merge identity mismatch for ${prUrl}: expected head ${branch}, got ${result.headRef ?? 'unknown'}; backing off`, + ) + this.#backoff(prUrl) + return + } + if (branch && candidate.gitUrl && this.#deps.adapter.projectIdentity) { + try { + const project = await this.#deps.adapter.projectIdentity(candidate.gitUrl) + if (result.headRepoFullName !== project) { + this.#deps.log?.( + `merge identity mismatch for ${prUrl}: expected repository ${project}, got ${result.headRepoFullName ?? 'unknown'}; backing off`, + ) + this.#backoff(prUrl) + return + } + } catch (error) { + this.#deps.log?.( + `merge identity check failed for ${prUrl}: ${error instanceof Error ? error.message : String(error)}; backing off`, + ) + this.#backoff(prUrl) + return + } + } + const key = artifactKey(artifact) const record: MergeRecord = { $type: COLLECTIONS.merge, diff --git a/packages/daemon/src/tangled-pages.ts b/packages/daemon/src/tangled-pages.ts new file mode 100644 index 0000000..f786909 --- /dev/null +++ b/packages/daemon/src/tangled-pages.ts @@ -0,0 +1,53 @@ +import type { TangledLocation } from './forge-tangled.js' + +export interface PageFetchResponse { + text(): Promise +} + +export type PageFetch = (url: string) => Promise + +const PULL_URI = /at:\/\/did:[^/"'<\s]+\/sh\.tangled\.repo\.pull\/[^/"'<\s]+/ +const pullPage = (location: TangledLocation, number: number): string => + `https://${location.host}/${location.owner}/${location.repo}/pulls/${number}` + +/** Resolve an appview number only when the page itself names the pull record. Tangled returns its + * 404 page with HTTP 200, so the body proof, rather than the response status, is authoritative. */ +export async function pullAtNumber( + fetchPage: PageFetch, + location: TangledLocation, + number: number, +): Promise { + const body = await (await fetchPage(pullPage(location, number))).text() + return PULL_URI.exec(body)?.[0] +} + +/** Find a newly opened pull in the appview's newest-first list, verifying every candidate page. + * The cap is deliberately small: a fresh pull is normally the first row, and exhausting the cap is + * logged rather than silently turning an incomplete search into an assertion. */ +export async function numberForPull( + fetchPage: PageFetch, + location: TangledLocation, + uri: string, + options: { limit?: number; log?: (message: string) => void } = {}, +): Promise { + const listUrl = `https://${location.host}/${location.owner}/${location.repo}/pulls` + const body = await (await fetchPage(listUrl)).text() + const numbers: number[] = [] + const seen = new Set() + const href = /\/pulls\/([1-9][0-9]*)/g + for (const match of body.matchAll(href)) { + const number = Number(match[1]) + if (!seen.has(number)) { + seen.add(number) + numbers.push(number) + } + } + const limit = options.limit ?? 5 + for (const number of numbers.slice(0, limit)) { + if ((await pullAtNumber(fetchPage, location, number)) === uri) return pullPage(location, number) + } + if (numbers.length > limit) { + options.log?.(`tangled page-link search for ${uri} exhausted its ${limit}-candidate budget`) + } + return undefined +} diff --git a/packages/daemon/test/config.test.mjs b/packages/daemon/test/config.test.mjs index 72e11ab..fbfd56f 100644 --- a/packages/daemon/test/config.test.mjs +++ b/packages/daemon/test/config.test.mjs @@ -351,11 +351,11 @@ const SPACES = ['at://did:plc:human/com.disnetdev.radial.space/space1'] it('parses a list of forges, and keeps the single legacy "forge" spelling working', () => { const list = parseRunConfig({ spaces: SPACES, - forges: [{ kind: 'github' }, { kind: 'tangled', hosts: ['tangled.org'], knownHosts: ['tangled.org ssh-ed25519 AAAA'] }], + forges: [{ kind: 'github' }, { kind: 'tangled', hosts: ['tangled.org'], pageLinks: false, knownHosts: ['tangled.org ssh-ed25519 AAAA'] }], }) assert.deepEqual(list.forges, [ { kind: 'github' }, - { kind: 'tangled', hosts: ['tangled.org'], knownHosts: ['tangled.org ssh-ed25519 AAAA'] }, + { kind: 'tangled', hosts: ['tangled.org'], pageLinks: false, knownHosts: ['tangled.org ssh-ed25519 AAAA'] }, ]) assert.equal('forge' in list, false) diff --git a/packages/daemon/test/forge-tangled.test.mjs b/packages/daemon/test/forge-tangled.test.mjs index e45f4ee..3ae1221 100644 --- a/packages/daemon/test/forge-tangled.test.mjs +++ b/packages/daemon/test/forge-tangled.test.mjs @@ -433,23 +433,23 @@ it('a lookup failure THROWS, so the poller backs off instead of reporting "not m await assert.rejects(() => broken.getPullRequestState(PULL_URI), /PDS unreachable/) }) -it('resolves an appview page number back to its pull rkey through Bobbin creation order', async () => { - const older = pullRecord({ - createdAt: '2026-06-01T00:00:00.000Z', - source: { branch: 'radial/impl-older' }, - }) - older.uri = `at://${AGENT}/${TANGLED_PULL}/3kolder` - const appview = bobbin( - { state: 'open' }, - { repositoryItems: [{ uri: older.uri, cid: older.cid, value: older.value }, pullRecord()] }, - ) - const adapter = forge({ reader: reader({}), bobbin: appview }) +it('resolves an appview page only when its HTML names the pull record', async () => { + const appview = bobbin({ state: 'open' }) + const pageFetch = async () => ({ text: async () => `pull` }) + const adapter = forge({ reader: reader({}), bobbin: appview, pageFetch }) const state = await adapter.getPullRequestState('https://tangled.org/@alice/widget/pulls/2', { gitUrl: GIT_URL, }) assert.equal(state.state, 'open') - assert.ok(appview.calls.includes(`listPulls ${REPO} asc`), appview.calls) assert.ok(appview.calls.includes(`listPullsBy ${AGENT}`), appview.calls) + const wrong = forge({ + reader: reader({}), + pageFetch: async () => ({ text: async () => '

404

' }), + }) + await assert.rejects( + () => wrong.getPullRequestState('https://tangled.org/@alice/widget/pulls/99'), + /page did not prove/, + ) await assert.rejects(() => adapter.getPullRequestState('https://github.com/a/b/pull/1'), /not a tangled pull request URL/) }) @@ -670,22 +670,20 @@ it('opens a pull record with the patch gzipped into its first round', async () = assert.ok(git.calls[0].includes('-C /run/checkout'), git.calls[0]) }) -it('links a newly opened pull to its web-appview page by Bobbin creation order', async () => { - const older = [ - pullRecord({ createdAt: '2026-07-08T00:00:00.000Z' }), - pullRecord({ createdAt: '2026-07-09T00:00:00.000Z' }), - ].map((record, index) => ({ - ...record, - uri: `at://did:plc:other/${TANGLED_PULL}/3kolder${index}`, - })) - const appview = bobbin( - { state: 'open' }, - { indexed: false, repositoryItems: older }, - ) +it('links a newly opened pull only to a web-appview page that proves its at-uri', async () => { + const calls = [] + const pageFetch = async (url) => { + calls.push(url) + return { + text: async () => url.endsWith('/pulls') + ? 'new' + : `${PULL_URI}`, + } + } const { actor } = actorSpy() const adapter = forge({ reader: reader({}), - bobbin: appview, + pageFetch, actor: () => actor, git: gitStub().exec, now: () => '2026-07-10T00:00:00.000Z', @@ -694,23 +692,23 @@ it('links a newly opened pull to its web-appview page by Bobbin creation order', const opened = await adapter.openPullRequest(openInput) assert.equal(opened.url, 'https://tangled.org/@alice/widget/pulls/3') - assert.deepEqual(appview.calls, ['getCoverage', `listPulls ${REPO} asc`]) + assert.equal(calls.length, 2) }) -it('keeps the stable pull at-uri when Bobbin is warming instead of deriving a wrong page', async () => { +it('keeps the stable pull at-uri when the page does not prove the record', async () => { const logs = [] - const appview = bobbin({ state: 'open' }, { ready: false }) const { actor } = actorSpy() const adapter = forge({ reader: reader({}), - bobbin: appview, + pageFetch: async (url) => ({ + text: async () => url.endsWith('/pulls') ? 'wrong' : '

404

', + }), actor: () => actor, git: gitStub().exec, log: (message) => logs.push(message), }) assert.equal((await adapter.openPullRequest(openInput)).url, PULL_URI) - assert.ok(logs.some((message) => message.includes('while Bobbin is warming up')), logs) }) it('a v2 on the same branch APPENDS a round to the same record, pinned with swapRecord', async () => { @@ -733,7 +731,7 @@ it('a v2 on the same branch APPENDS a round to the same record, pinned with swap assert.deepEqual(writes.put[0].value.target, { repo: REPO, repoDid: REPO, branch: 'main' }) }) -it('the agent’s OWN pull lookup stays on its PDS; Bobbin is only used to derive the page link', async () => { +it('the agent’s OWN pull lookup stays on its PDS and never consults Bobbin for numbering', async () => { // A Bobbin miss here would not degrade — it would open a SECOND pull request for a chain that // already has one. The agent's own PDS cannot lag behind a record the agent just wrote to it. const { actor, writes } = actorSpy() @@ -744,7 +742,7 @@ it('the agent’s OWN pull lookup stays on its PDS; Bobbin is only used to deriv assert.equal(writes.created.length, 0) assert.equal(writes.put[0].value.rounds.length, 2) assert.deepEqual(pds.calls, [`${TANGLED_REPO}@${OWNER}`, `${TANGLED_PULL}@${AGENT}`]) - assert.deepEqual(appview.calls, ['getCoverage', `listPulls ${REPO} asc`]) + assert.deepEqual(appview.calls, []) }) it('a pull for a different branch or a different repo is a different chain, so a new record', async () => { diff --git a/packages/daemon/test/merge-poll.test.mjs b/packages/daemon/test/merge-poll.test.mjs index 1292761..4c124e9 100644 --- a/packages/daemon/test/merge-poll.test.mjs +++ b/packages/daemon/test/merge-poll.test.mjs @@ -12,12 +12,12 @@ const OTHER = 'did:plc:other' // and the actors' did/artifactTypes/client, so a hand-built index gives precise control over // membership, dedup, and backoff. --- -function implArtifact(did, rkey, { pr, type = 'implementation', cid = `cid-${rkey}` } = {}) { +function implArtifact(did, rkey, { pr, branch, type = 'implementation', cid = `cid-${rkey}` } = {}) { return { did, uri: `at://${did}/${COLLECTIONS.artifact}/${rkey}`, cid, - value: { $type: COLLECTIONS.artifact, type, links: pr ? { pr } : {} }, + value: { $type: COLLECTIONS.artifact, type, links: pr ? { pr, ...(branch ? { branch } : {}) } : {} }, } } @@ -342,6 +342,42 @@ describe('MergePoller', () => { assert.equal(creates[0].value.mergedAt, '2026-01-02T00:00:00Z') }) + it('writes no merge when a merged result names a different artifact branch or repository', async () => { + const creates = [] + const logs = [] + const artifact = implArtifact(AGENT_A, 'impl', { + pr: 'https://forge/pr/1', + branch: 'radial/impl-right', + }) + const index = makeIndex({ artifacts: [artifact] }) + const wrongBranch = { + getPullRequestState: async () => ({ + state: 'merged', + headRef: 'radial/impl-wrong', + headRepoFullName: 'repo', + }), + projectIdentity: async () => 'repo', + } + const p1 = poller(wrongBranch, clock().now, (message) => logs.push(message)) + p1.pump(index, registry([fakeActor(AGENT_A, ['implementation'], creates)])) + await p1.drain() + assert.equal(creates.length, 0) + + const wrongRepo = { + getPullRequestState: async () => ({ + state: 'merged', + headRef: 'radial/impl-right', + headRepoFullName: 'other-repo', + }), + projectIdentity: async () => 'repo', + } + const p2 = poller(wrongRepo, clock().now, (message) => logs.push(message)) + p2.pump(index, registry([fakeActor(AGENT_A, ['implementation'], creates)])) + await p2.drain() + assert.equal(creates.length, 0) + assert.equal(logs.filter((message) => message.includes('identity mismatch')).length, 2) + }) + it('dedups within the process across ticks before ingestion (alreadyWritten)', async () => { const creates = [] const actor = fakeActor(AGENT_A, ['implementation'], creates) -- 2.51.2