From 6e48175d46b57a1edeb4298867392b0edf4a9af5 Mon Sep 17 00:00:00 2001 From: Tim Disney Date: Sun, 26 Jul 2026 10:13:28 -0700 Subject: [PATCH] =?UTF-8?q?Continue=20the=20same=20PR=20after=20review=20?= =?UTF-8?q?=E2=80=94=20merge,=20branch-existence=20and=20wording=20fixes?= =?UTF-8?q?=20(#7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update the same PR after review instead of opening a new one A v2 implementation turn already reused its predecessor's branch and PR, but every guard had to hold and one of them was unreachable by default. Close the five ways a revision ended up on a second pull request. - `radiald init` now scaffolds a `run` block with `forge: { kind: 'github' }`, and `writeConfig` serializes `run` at all — it dropped it, so a scaffold could not carry a forge even if the builder produced one. `run.spaces` may be empty in the file; only `radiald run` demands one. - Without a forge adapter the dispatcher now decides reuse from the index: an in-namespace branch, a PR in this project's repo, and no trusted `merge` annotation on the predecessor. It logs which signal answered and what that signal cannot see. - The compose card and the successor button say which PR the next version lands in, hedged — the browser can see a merge annotation, not a PR closed by hand. A `request_changes` verdict makes the successor the primary button; every sibling stays live. - `RequestDraft.basedOn` is a list, and a successor inherits its predecessor's other-type bases. A v2 was arriving with the findings against v1 and none of the plan those findings judge it against. - Two v2 requests off one predecessor no longer push to one branch concurrently; the second is reported as contended and launches on a later pump. - The implementation prompt fetches the base branch and merges it when behind — a reused branch is checked out shallow and single-branch. - The ledger records the selected branch, and the reuse decision logs on the success path too. Co-Authored-By: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) * Resolve the review findings on the v2 PR-continuation change The reused-branch checkout could not merge. `checkoutRepo` clones `--depth 1`, which grafts the branch tip as parentless, so `git merge` against the base fails with "refusing to merge unrelated histories" — on exactly the continuation path this feature exists for, and the prompt's fetch deepened only the base, which is the wrong side. A continued branch is now cloned with `MERGE_CLONE_DEPTH` history and its base comes down beside it as `origin/`; the prompt merges that and is told to `--unshallow` (never `--allow-unrelated-histories`) if the fork point is deeper still. Proven end to end against real git over `file://`: the shallow clone refuses, the new one merges. The `merged` guard on the no-forge fallback was dead where it mattered. Merge annotations come from the merge poller, which only runs with a forge configured, so a forgeless daemon can never see one — and a predecessor branch deleted after its PR merged would be cloned again on every retry until the request gave up. The fallback now also asks the remote: one `git ls-remote`, same credentials as the clone, no forge API. Docs, the startup advisory and the reuse log say what that signal is and is not worth. `freshChainNote` hangs on every other-type button, most of which produce neither a branch nor a pull request, so it no longer promises them — it is a fact about chains, and `prContinuity` keeps the branch/PR sentence. `prContinuity` also mirrors the daemon's third guard: a pull request outside the project's own repository is not one the turn will continue. `scrubbedGitEnv` drops an ambient `GH_TOKEN`/`GITHUB_TOKEN` rather than passing it to a checkout of any host. That is what the non-GitHub-checkout assertion always meant, and it clears the red test every turn run inside Radial's own containers was seeing. Co-Authored-By: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) --------- Co-authored-by: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) --- docs/design.md | 1 + docs/plan.md | 2 +- packages/core/src/fixture.ts | 8 +- packages/core/test/bundle.test.mjs | 58 +++ packages/daemon/src/bundle-writer.ts | 94 ++++- packages/daemon/src/cli.ts | 48 +++ packages/daemon/src/config.ts | 19 +- packages/daemon/src/dispatch.ts | 105 +++++- packages/daemon/src/harness.ts | 9 +- packages/daemon/src/ledger.ts | 26 +- packages/daemon/src/turn.ts | 18 +- packages/daemon/test/bundle-writer.test.mjs | 197 ++++++++++- packages/daemon/test/cli.test.mjs | 42 ++- packages/daemon/test/config.test.mjs | 35 +- packages/daemon/test/dispatch.test.mjs | 330 ++++++++++++++++++ packages/daemon/test/harness.test.mjs | 16 + packages/daemon/test/ledger.test.mjs | 51 +++ packages/daemon/test/turn.test.mjs | 54 +++ .../ui/src/lib/components/NewRequest.svelte | 37 +- .../ui/src/lib/components/UnitDetail.svelte | 27 +- packages/ui/src/lib/requests.test.ts | 244 ++++++++++++- packages/ui/src/lib/requests.ts | 209 ++++++++++- packages/ui/src/lib/write.test.ts | 4 +- readme.md | 25 ++ 24 files changed, 1593 insertions(+), 66 deletions(-) diff --git a/docs/design.md b/docs/design.md index 205075a..5170146 100644 --- a/docs/design.md +++ b/docs/design.md @@ -191,6 +191,7 @@ ForgeAdapter: The adapter is daemon-side (it holds the operator's observation credentials); turn-side forge writes use the turn's forge token instead (§10, §13). The UI only ever consumes URLs from records. +- **One PR per version chain.** A version chain *is* a branch *is* a pull request: asking for another version of an implementation — the way a review's findings get resolved — continues the predecessor's branch and updates its PR, and a request that starts a new chain gets a new PR. That mapping is expressed entirely by `basedOn`; there is no forge concern on the protocol and no way to ask for one outcome from a basis that implies the other. The daemon selects the branch, and reuses the predecessor's only when the request resolves to exactly one same-type predecessor under the same target, that predecessor links a branch inside the daemon's own `radial/impl-` namespace and a PR in this project's repository, and the PR is still live work. With a `ForgeAdapter` wired, "still live" is `getPullRequestState` saying `open` with head ref, head repository and base ref all matching; without one it is the index plus one `git ls-remote` — no trusted `merge` annotation on the predecessor, and the branch still present on the remote — and the daemon logs which of the two answered. Be precise about what the no-forge signal is worth: `merge` annotations come from the merge poller, which only runs when a forge is configured, so in a forgeless deployment the merge check is structurally dead and the `ls-remote` is the guard that carries it. What that probe buys is not tidiness but recoverability — a predecessor branch deleted after its PR merged makes the reuse clone fail identically on every retry, so without the probe such a v2 becomes permanently undispatchable rather than merely landing on a fresh branch. What remains invisible without a forge: a merged branch that still exists reads like an open one, and so does a PR closed without merging; the turn recovers from the latter by opening a PR when the branch has none. Anything else is a fresh `radial/impl-` branch and a new PR. A continued branch is also the one checkout that must merge — its base is fetched alongside a deepened clone, because `--depth 1` grafts the tip as parentless and git then refuses the merge as unrelated histories. Rewriting history to keep one clean commit is explicitly rejected: `links.commit` is load bearing — reviews pin an exact version and checkruns fetch its sha — so a reviewed commit must stay reachable. - **Checks run on every implementation artifact.** On seeing one land, the daemon runs the project's `checks` against the linked commit in a clean container and posts a `checkrun`. Any member can technically post a checkrun, so the UI always surfaces the authoring DID alongside the results. Results annotate rather than gate: a red checkrun is flagged prominently on the card (and included in any reviewer's bundle), and the obvious next click is "request revision" — but nothing bounces automatically. - **Radial never merges.** A human merges the PR on the forge; the daemon observes the merge through the adapter and annotates the artifact as merged. - **Diff-reading is delegated to the forge permanently** — this is a deliberate long-term stance, not a v1 shortcut: the web UI links to the PR for reading code, and Radial never grows a native diff/annotation UI. Findings posted via Radial are structured (`path`, `line`, `severity`, `body`) so the UI can render them alongside the link and revision turns can consume them mechanically. Optionally the adapter mirrors findings as PR comments for reviewers who live in the forge. diff --git a/docs/plan.md b/docs/plan.md index b95d606..c6c88d6 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -71,7 +71,7 @@ The turn model (design §9), for **one built-in type only: `plan`**, with the re Close the loop through code (design §10). - ForgeAdapter interface + GitHub implementation (`openPullRequest`, `updatePullRequest`, `getPullRequestState`, `compareUrl`). Daemon-side credentials only. *(Superseded in Phase 4.5: turns act on the forge directly under the operator's forge auth; the adapter narrows to observation.)* -- Implementation turns: branch, commit, push, open/update PR, post the artifact with `links: {branch, commit, pr}`; v2 turns consume structured review findings from the bundle. +- Implementation turns: branch, commit, push, open/update PR, post the artifact with `links: {branch, commit, pr}`; v2 turns consume structured review findings from the bundle. A v2 continues its predecessor's branch and PR when the daemon can establish the PR is still live work — confirmed against the forge where an adapter is wired, and otherwise from the index plus a `git ls-remote` (an in-namespace branch, a PR in this project's repository, no `merge` annotation on the predecessor, and the branch still on the remote), with the daemon logging which signal decided it. The index fallback exists because `run.forge` is easy to omit and its absence silently produced a second PR for every revision; the `ls-remote` is there because the merge check cannot fire without a forge (nothing writes the annotations), and a deleted predecessor branch would otherwise make a v2 crash-loop instead of falling back to a fresh one. A continued branch is cloned deep with its base fetched, since a `--depth 1` checkout cannot merge at all. - Check runner: on an implementation artifact landing, run project `checks` against the linked commit in a clean container, post `checkrun`. Results annotate the artifact — nothing bounces automatically (§10). - Merge observation: poll PR state; on merge, the daemon annotates the artifact as merged. Radial never merges; goal closure stays a human edit. - **Thin UI spike** (deliberately minimal, replaced wholesale in Phase 6): a single page over the daemon's index — goal timeline plus registry-generated request buttons. The buttons-as-workflow interaction is the product's core bet, so it should meet real use before the remaining infrastructure phases finish. diff --git a/packages/core/src/fixture.ts b/packages/core/src/fixture.ts index ab849fd..c3d2629 100644 --- a/packages/core/src/fixture.ts +++ b/packages/core/src/fixture.ts @@ -608,7 +608,7 @@ export function fixtureSpace(): FixtureSpace { body: 'Removed `packages/daemon/src/egress.ts` and the internal-network wiring in `container.ts`. Turn containers now attach to the default bridge, and `--network host` is rejected at the call site rather than by convention.\n\nThe env a turn receives is built by one function, `turnEnv()`, which takes the forge token and the model key and nothing else. There is no path by which a session credential reaches it, and the test asserts the exact key set rather than asserting an absence.', links: { - branch: 'radial/possession-boundary', + branch: 'radial/impl-6a1f0c94d3b7', commit: '8f3c1d94a27b6e05f2a1c8b3d7e04916a2c5b8d1', pr: 'https://github.com/disnet/radial-ng/pull/241', }, @@ -701,7 +701,7 @@ export function fixtureSpace(): FixtureSpace { body: "The trigger lives in the daemon's index-diff loop: on each ingestion cycle, any artifact new to the index whose type has `autoReview` enabled gets a review request written under the daemon's agent identity.\n\nTerminality is structural rather than guarded — the diff only inspects `com.disnetdev.radial.artifact` records, and the record a review turn emits is a `com.disnetdev.radial.review`. There is no counter to get wrong.", links: { - branch: 'radial/auto-review', + branch: 'radial/impl-2d9b47e10c85', commit: 'c07a5be3f19d42a8b6015d3e7f24c9081b5a6e3f', pr: 'https://github.com/disnet/radial-ng/pull/228', }, @@ -749,7 +749,7 @@ export function fixtureSpace(): FixtureSpace { body: 'Six golden scenarios — pinned versions, edits, removal ignoring the removed member’s records, re-add restoring them, competing claims, cross-goal system artifacts — each shuffled 500 ways and asserted structurally equal.\n\nThe shuffle is seeded and the seed prints on failure, so a red run reproduces exactly. This is the property the whole design leans on: two materializers that ingested the same records compute the same view.', links: { - branch: 'radial/index-property-tests', + branch: 'radial/impl-b3e05f7a2c61', commit: '51ab9de0c4f8730b2e6a1c94d5f807b3a6e2c1d9', pr: 'https://github.com/disnet/radial-ng/pull/196', }, @@ -854,7 +854,7 @@ export function fixtureSpace(): FixtureSpace { type: 'implementation', body: '## What landed\n\n`Diagnostics.svelte` renders `index.ignored` and `index.edits` inside a `
` at the foot of the pane. Grouping and the reason strings come from the fold — the component reads them, it does not compose them.\n\n## Note\n\nBuilt on plan v1, which has no verdict yet. If the verdict changes the grouping this is a small change; if it changes the reason strings it is a change in `core`, not here.', links: { - branch: 'radial/ignored-disclosure', + branch: 'radial/impl-91c4de08b7a2', commit: 'c07f3ba91d5e264a8b03f7c1e9d4a2b6f8017c53', pr: 'https://github.com/disnet/radial-ng/pull/263', }, diff --git a/packages/core/test/bundle.test.mjs b/packages/core/test/bundle.test.mjs index 9e13b92..6715b3c 100644 --- a/packages/core/test/bundle.test.mjs +++ b/packages/core/test/bundle.test.mjs @@ -591,6 +591,37 @@ function implRequestScenario() { return { records, spaceUri: space.uri, requestUri: reqV2.uri, requestCid: reqV2.cid, artV1Ref: ref(artV1), links } } +// A plan, an implementation v1 built on it that a reviewer asked for changes on, and TWO competing +// v2 requests off it: one naming only the predecessor, one naming the predecessor and the plan. +function implSuccessorScenario() { + const records = [] + const add = (r) => (records.push(r), r) + const space = add(make(ROOT, COLLECTIONS.space, 'space', 'cid-space', { $type: COLLECTIONS.space, name: 'S', description: '', createdAt: '2026-01-01T00:00:00Z' })) + add(make(ROOT, COLLECTIONS.addMember, 'm-h', 'cid-mh', { $type: COLLECTIONS.addMember, space: ref(space), did: HUMAN, kind: 'human', role: 'member', createdAt: '2026-01-01T00:00:01Z' })) + add(make(ROOT, COLLECTIONS.addMember, 'm-a', 'cid-ma', { $type: COLLECTIONS.addMember, space: ref(space), did: AGENT_A, kind: 'agent', role: 'agent', createdAt: '2026-01-01T00:00:02Z' })) + add(make(ROOT, COLLECTIONS.artifactType, 't-plan', 'cid-t-plan', { $type: COLLECTIONS.artifactType, space: ref(space), name: 'plan', brief: 'p', outputSpec: { format: 'markdown', description: 'x' }, scope: 'goal', createdAt: '2026-01-01T00:00:09Z' })) + add(make(ROOT, COLLECTIONS.artifactType, 't-impl', 'cid-t-impl', { $type: COLLECTIONS.artifactType, space: ref(space), name: 'implementation', brief: 'impl', outputSpec: { format: 'markdown', description: 'x' }, scope: 'goal', createdAt: '2026-01-01T00:00:10Z' })) + const project = add(make(HUMAN, COLLECTIONS.project, 'proj', 'cid-proj', { $type: COLLECTIONS.project, space: ref(space), name: 'p', gitUrl: 'https://github.com/acme/widget.git', defaultBranch: 'main', checks: [], autoReview: {}, createdAt: '2026-01-01T00:01:00Z' })) + const goal = add(make(HUMAN, COLLECTIONS.goal, 'goal', 'cid-goal', { $type: COLLECTIONS.goal, space: ref(space), project: ref(project), title: 'G', body: 'b', createdAt: '2026-01-01T00:02:00Z' })) + const planReq = add(make(HUMAN, COLLECTIONS.artifactRequest, 'req-plan', 'cid-req-plan', { $type: COLLECTIONS.artifactRequest, goal: ref(goal), type: 'plan', basedOn: [], assignee: AGENT_A, createdAt: '2026-01-01T00:03:00Z' })) + const plan = add(make(AGENT_A, COLLECTIONS.artifact, 'plan-v1', 'cid-plan-v1', { $type: COLLECTIONS.artifact, request: ref(planReq), goal: ref(goal), type: 'plan', body: 'the plan', links: {}, createdAt: '2026-01-01T00:04:00Z' })) + const implReq = add(make(HUMAN, COLLECTIONS.artifactRequest, 'req-impl', 'cid-req-impl', { $type: COLLECTIONS.artifactRequest, goal: ref(goal), type: 'implementation', basedOn: [ref(plan)], assignee: AGENT_A, createdAt: '2026-01-01T00:05:00Z' })) + const impl = add(make(AGENT_A, COLLECTIONS.artifact, 'impl-v1', 'cid-impl-v1', { $type: COLLECTIONS.artifact, request: ref(implReq), goal: ref(goal), type: 'implementation', body: 'v1', links: { branch: 'radial/impl-abc', commit: 'abadcafe', pr: 'https://github.com/acme/widget/pull/1' }, createdAt: '2026-01-01T00:06:00Z' })) + add(make(HUMAN, COLLECTIONS.review, 'rev-impl', 'cid-rev-impl', { $type: COLLECTIONS.review, subject: ref(impl), verdict: 'request_changes', findings: [{ severity: 'error', body: 'the guard is inverted' }], createdAt: '2026-01-01T00:07:00Z' })) + const both = add(make(HUMAN, COLLECTIONS.artifactRequest, 'req-v2-both', 'cid-req-v2-both', { $type: COLLECTIONS.artifactRequest, goal: ref(goal), type: 'implementation', basedOn: [ref(impl), ref(plan)], createdAt: '2026-01-01T00:08:00Z' })) + const implOnly = add(make(HUMAN, COLLECTIONS.artifactRequest, 'req-v2-one', 'cid-req-v2-one', { $type: COLLECTIONS.artifactRequest, goal: ref(goal), type: 'implementation', basedOn: [ref(impl)], createdAt: '2026-01-01T00:09:00Z' })) + return { + records, + spaceUri: space.uri, + planRef: ref(plan), + implRef: ref(impl), + bothUri: both.uri, + bothCid: both.cid, + implOnlyUri: implOnly.uri, + implOnlyCid: implOnly.cid, + } +} + describe('buildTurnBundle (generalized types)', () => { it('builds a bundle for a non-plan (implementation) request and carries basedOn links', () => { const scenario = implRequestScenario() @@ -603,6 +634,33 @@ describe('buildTurnBundle (generalized types)', () => { assert.deepEqual(bundle.basedOn.map((a) => a.uri), [scenario.artV1Ref.uri]) assert.deepEqual(bundle.basedOn[0].links, scenario.links) }) + + // A v2 asked with ONLY its predecessor arrives holding the findings against v1 and none of the + // plan those findings judge it against: the ancestry walk follows `prev` within one type and + // never crosses to the plan the chain was built from. Naming both bases is the whole fix, and it + // is a property of the request, so this is what a compose card has to write. + it('carries the plan into a v2 bundle only when the v2 request names it too', () => { + const scenario = implSuccessorScenario() + const store = new MemoryRecordStore() + scenario.records.forEach((r) => store.put(r)) + const index = materialize(store, { spaceUri: scenario.spaceUri, asOf: AS_OF }) + + const withPlan = buildTurnBundle(index, scenario.bothUri, scenario.bothCid) + assert.deepEqual(withPlan.basedOn.map((a) => a.uri), [scenario.planRef.uri, scenario.implRef.uri]) + assert.equal(withPlan.basedOn.find((a) => a.uri === scenario.planRef.uri).body, 'the plan') + // The findings arrive either way; the specification is what the single-ref request loses. + assert.deepEqual( + withPlan.reviewFindings.map((r) => [r.subject.uri, r.verdict]), + [[scenario.implRef.uri, 'request_changes']], + ) + + const implOnly = buildTurnBundle(index, scenario.implOnlyUri, scenario.implOnlyCid) + assert.deepEqual(implOnly.basedOn.map((a) => a.uri), [scenario.implRef.uri]) + assert.deepEqual( + implOnly.reviewFindings.map((r) => [r.subject.uri, r.verdict]), + [[scenario.implRef.uri, 'request_changes']], + ) + }) }) // --- Review requests -------------------------------------------------------- diff --git a/packages/daemon/src/bundle-writer.ts b/packages/daemon/src/bundle-writer.ts index 5a05757..c1f9e4e 100644 --- a/packages/daemon/src/bundle-writer.ts +++ b/packages/daemon/src/bundle-writer.ts @@ -118,11 +118,19 @@ export function defaultGitExec(): GitExec { * business being reachable from it). GIT_CONFIG_SYSTEM/GIT_CONFIG_GLOBAL are pinned to /dev/null * so a url.*.insteadOf rewrite sitting in a system or global gitconfig can't silently redirect * the already-validated https URL elsewhere. + * + * `GH_TOKEN`/`GITHUB_TOKEN` are dropped for the same reason as SSH_AUTH_SOCK: the only checkout + * entitled to a GitHub token is one whose URL has been validated as github.com https, and that one + * gets it put back deliberately by `gitEnvForCheckout`. Inheriting the daemon's own token would + * hand it to a checkout of any other host. (It also makes the assertion below true in fact rather + * than only in a fixture: Radial's own turn containers export `GH_TOKEN`, so a daemon test that + * read it off the ambient environment failed for a reason that had nothing to do with its subject.) */ export function scrubbedGitEnv(env: Record): Record { const scrubbed: Record = {} for (const [key, value] of Object.entries(env)) { if (value === undefined || key.startsWith('GIT_') || key === 'SSH_AUTH_SOCK') continue + if (key === 'GH_TOKEN' || key === 'GITHUB_TOKEN') continue scrubbed[key] = value } scrubbed.GIT_TERMINAL_PROMPT = '0' @@ -160,10 +168,24 @@ function checkoutError(operation: string, result: { code: number; stderr: string return new Error(`${operation} failed (exit ${result.code}): ${stderr}`) } +/** How much history a checkout that will have to MERGE gets. A `--depth 1` clone grafts its one + * commit as parentless, so `git merge` against any other ref reports "refusing to merge unrelated + * histories" — there is no merge base to find. Fifty commits on each side is enough for a branch + * Radial itself opened off the base a few days ago, and cheap next to `--unshallow` on a large + * repository; a turn that needs more is told how to deepen further by the implementation prompt. */ +export const MERGE_CLONE_DEPTH = 50 + /** * Shallow, single-branch, no-tags, no-submodules clone of an allowlisted-scheme git URL, with * hooks and credential helpers disabled so a malicious repo can't execute code or exfiltrate * credentials during checkout. Returns the resolved commit at HEAD. + * + * `depth` and `alsoFetch` exist for one case: a turn continuing a predecessor's branch, which has + * to merge its base into it before it can implement anything. Both sides of that merge need history + * — deepening only the base leaves the branch tip grafted and the merge still fails — so the branch + * is cloned `--depth depth` and each `alsoFetch` ref is fetched to the same depth as + * `refs/remotes/origin/`. Defaults (depth 1, nothing else fetched) leave every other turn's + * checkout exactly as it was. */ export async function checkoutRepo(input: { gitUrl: string @@ -172,21 +194,27 @@ export async function checkoutRepo(input: { allowedSchemes?: string[] exec?: GitExec githubToken?: string + depth?: number + alsoFetch?: string[] }): Promise<{ path: string; commit: string }> { const allowedSchemes = input.allowedSchemes ?? ['https'] assertGitUrlAllowed(input.gitUrl, allowedSchemes) const exec = input.exec ?? defaultGitExec() const checkoutEnv = gitEnvForCheckout(input.gitUrl, input.githubToken) const { env } = checkoutEnv + const depth = String(input.depth ?? 1) + const hardening = [ + '-c', + 'core.hooksPath=/dev/null', + ...(checkoutEnv.authenticated ? [] : ['-c', 'credential.helper=']), + ] const clone = await exec( [ 'git', - '-c', - 'core.hooksPath=/dev/null', - ...(checkoutEnv.authenticated ? [] : ['-c', 'credential.helper=']), + ...hardening, 'clone', '--depth', - '1', + depth, '--single-branch', '--no-tags', '--branch', @@ -197,11 +225,69 @@ export async function checkoutRepo(input: { { env }, ) if (clone.code !== 0) throw checkoutError('git clone', clone, input.githubToken) + for (const ref of input.alsoFetch ?? []) { + if (ref === input.ref) continue + const fetched = await exec( + [ + 'git', + ...hardening, + '-C', + input.dest, + 'fetch', + '--depth', + depth, + '--no-tags', + 'origin', + `+refs/heads/${ref}:refs/remotes/origin/${ref}`, + ], + { env }, + ) + if (fetched.code !== 0) throw checkoutError(`git fetch ${ref}`, fetched, input.githubToken) + } const rev = await exec(['git', '-C', input.dest, 'rev-parse', 'HEAD'], { env }) if (rev.code !== 0) throw checkoutError('git rev-parse', rev, input.githubToken) return { path: input.dest, commit: rev.stdout.trim() } } +/** + * Does `branch` still exist on the remote? One `git ls-remote --exit-code --heads`, with the same + * validated URL, scrubbed environment and env-only credential helper a clone gets — no forge API and + * no extra credential. + * + * It answers the one question the ATProto index cannot: a branch whose pull request merged and whose + * head a human (or `delete_branch_on_merge`) deleted is still linked by the artifact that made it. + * Cloning that ref fails, and a turn that fails there fails identically on every retry, so without + * this check a v2 off such a predecessor is undispatchable rather than merely on a fresh branch. + * Exits: 0 found, 2 no matching ref, anything else is a real failure and throws — the caller treats + * a throw the same way it treats `false`, by opening a fresh branch. + */ +export async function remoteBranchExists(input: { + gitUrl: string + branch: string + allowedSchemes?: string[] + exec?: GitExec + githubToken?: string +}): Promise { + assertGitUrlAllowed(input.gitUrl, input.allowedSchemes ?? ['https']) + const exec = input.exec ?? defaultGitExec() + const checkoutEnv = gitEnvForCheckout(input.gitUrl, input.githubToken) + const result = await exec( + [ + 'git', + ...(checkoutEnv.authenticated ? [] : ['-c', 'credential.helper=']), + 'ls-remote', + '--exit-code', + '--heads', + input.gitUrl, + `refs/heads/${input.branch}`, + ], + { env: checkoutEnv.env }, + ) + if (result.code === 0) return true + if (result.code === 2) return false + throw checkoutError('git ls-remote', result, input.githubToken) +} + /** * Checks out one *exact* commit (not a branch tip) into a fresh repo: `git init` + a shallow * `git fetch ` + `git checkout FETCH_HEAD`. Unlike `checkoutRepo`, this pins the tree to diff --git a/packages/daemon/src/cli.ts b/packages/daemon/src/cli.ts index 4fc10a2..cf26a1d 100644 --- a/packages/daemon/src/cli.ts +++ b/packages/daemon/src/cli.ts @@ -23,6 +23,7 @@ import { type DaemonRunConfig, type RadialConfig, } from './config.js' +import { remoteBranchExists } from './bundle-writer.js' import { CheckDispatcher } from './check-dispatch.js' import { CheckLedger } from './check-ledger.js' import { runCheckRun, type CheckRunInput } from './check-runner.js' @@ -326,8 +327,39 @@ export async function reconcileCheckOrphans(runner: ContainerRunner, ledger: Che } } +/** + * The startup warning for a daemon that produces implementations without a live forge adapter. + * + * The adapter is what lets the daemon ask whether a predecessor's pull request is still open before + * continuing its branch; without it a v2 turn decides from the local index alone, which cannot see a + * pull request somebody closed without merging. This is the setting that decides whether resolving a + * review's findings updates the pull request under review or opens a second one, and nothing in the + * daemon's output used to name it — so an operator who omitted `run.forge` had no way to connect the + * two. Pure and exported so the wording is testable rather than buried in a long-running command. + */ +export function forgeAdvisory(input: { + /** A live adapter resolved (configured AND GitHub authentication available). */ + adapter: boolean + /** `run.forge` is set in the config, whatever came of it. */ + configured: boolean + producesImplementation: boolean + configPath: string +}): string | undefined { + if (input.adapter || !input.producesImplementation) return undefined + const consequence = + 'implementation v2 decides whether to continue its predecessor’s pull request from the local index and the git remote alone — nothing writes merge annotations without a forge, so a merged pull request whose branch survives reads like an open one, and a pull request closed without merging is invisible to it.' + return input.configured + ? `GitHub authentication is unavailable, so ${consequence}` + : `no run.forge configured — merge observation is off, and ${consequence} Add "run": { "forge": { "kind": "github" } } to ${input.configPath}.` +} + async function runCommand(args: string[]): Promise { const { configPath, config, run } = await requireRunConfig(values(args, '--config')[0]) + // `run.spaces` may be empty in the file (the init scaffold writes `spaces: []` so the block it + // emits is loadable); a daemon with nothing to poll is only an error here, where polling starts. + if (run.spaces.length === 0) { + throw new Error(`${configPath} has an empty "run.spaces"; add the "at://" URI of the space to serve`) + } const interval = Number(values(args, '--interval')[0] ?? 5_000) if (!Number.isFinite(interval) || interval < 100) throw new Error('--interval must be at least 100ms') @@ -387,6 +419,13 @@ async function runCommand(args: string[]): Promise { ) } } + const advisory = forgeAdvisory({ + adapter: Boolean(forge), + configured: Boolean(run.forge), + producesImplementation: actors.all.some((actor) => actor.artifactTypes.includes('implementation')), + configPath, + }) + if (advisory) console.warn(advisory) const harness = new ClaudeCodeHarness() const boundRunTurn = (input: TurnInput) => @@ -410,6 +449,15 @@ async function runCommand(args: string[]): Promise { ...(run.memory !== undefined ? { memory: run.memory } : {}), turnTransport: run.turnTransport, ...(forge ? { forge } : {}), + // The branch probe the no-forge reuse fallback leans on, carrying the daemon's token so it can + // answer for a private repository too. + branchExists: ({ gitUrl, branch }) => + remoteBranchExists({ + gitUrl, + branch, + allowedSchemes: run.gitSchemes, + ...(githubToken ? { githubToken } : {}), + }), implementationEnabled: !!githubToken, runTurn: boundRunTurn, log: (message) => console.log(message), diff --git a/packages/daemon/src/config.ts b/packages/daemon/src/config.ts index a11494e..21dbb62 100644 --- a/packages/daemon/src/config.ts +++ b/packages/daemon/src/config.ts @@ -149,8 +149,12 @@ function forgeValue(value: unknown, where: string): { kind: 'github' } | undefin /** Validates the raw `run` block; applies no runtime defaults (the daemon's run path applies those). */ export function parseRunConfig(value: unknown): DaemonRunConfig { if (!object(value)) throw new TypeError('run config must be an object') + // An EMPTY list parses: the `radiald init` scaffold writes `spaces: []` so the block it emits + // (and the `forge` inside it) is loadable before an operator has pasted their space URI in. The + // non-empty requirement belongs to `radiald run`, which is the only command that needs one, and + // it is enforced there rather than making every other command refuse a fresh scaffold. const spaces = strings(value.spaces, 'run.spaces') - if (!spaces || spaces.length === 0) throw new TypeError('run.spaces must be a non-empty array of "at://" URIs') + if (!spaces) throw new TypeError('run.spaces must be an array of "at://" URIs') for (const space of spaces) { if (!space.startsWith('at://')) throw new TypeError(`run.spaces entries must be "at://" URIs, got "${space}"`) } @@ -356,12 +360,19 @@ export function extraIdentities(config: RadialConfig, profiles: string[]): strin * A starting config for a new operator: one identity, one profile per built-in artifact type * (`plan` and `implementation`, §11) plus a reviewer, each inheriting the shared harness and * models. Separate profiles are what let an operator route a different model to reviews. + * + * The `run` block is emitted with the forge already wired. It is the one setting an operator has + * no way to guess is load bearing: without it the daemon cannot confirm a predecessor's pull + * request, so a second version of an implementation opens a SECOND pull request instead of + * updating the one under review. `spaces` is left empty for the operator to fill in — every other + * `run` default is computed at runtime and deliberately not frozen into the file. */ export function buildDefaultConfig(identifier: string): RadialConfig { return { identifier, harness: 'claude', models: ['claude-opus-4-8=high'], + run: { spaces: [], forge: { kind: 'github' } }, agents: { planner: { artifactTypes: ['plan'] }, implementer: { artifactTypes: ['implementation'] }, @@ -380,12 +391,16 @@ export async function writeConfig( throw new Error(`${path} already exists; pass --force to overwrite it`) } await mkdir(dirname(path), { recursive: true, mode: 0o700 }) - // Defaults first, agents last: the file reads top-down from identity to per-profile overrides. + // Defaults first, then `run`, agents last: the file reads top-down from identity through the + // daemon's own settings to per-profile overrides. `run` is serialized rather than dropped — + // dropping it meant the scaffold could not carry a `forge`, and an operator who never noticed + // that got a fresh pull request for every implementation v2. const body = { identifier: config.identifier, ...(config.pds ? { pds: config.pds } : {}), ...(config.harness ? { harness: config.harness } : {}), ...(config.models ? { models: config.models } : {}), + ...(config.run ? { run: config.run } : {}), agents: config.agents, } await writeFile(path, `${JSON.stringify(body, null, 2)}\n`) diff --git a/packages/daemon/src/dispatch.ts b/packages/daemon/src/dispatch.ts index ad47fdf..91076a8 100644 --- a/packages/daemon/src/dispatch.ts +++ b/packages/daemon/src/dispatch.ts @@ -16,6 +16,7 @@ import { type TurnBundle, } from '@radial/core' import type { ActorRegistry, LoadedActor } from './actors.js' +import { remoteBranchExists } from './bundle-writer.js' import { parseGitHubPullUrl, parseGitHubRepo, type ForgeAdapter } from './forge.js' import type { TurnLedger } from './ledger.js' import { implBranchName, planArtifactRkey, type TurnRequestAnchor } from './turn-socket.js' @@ -27,6 +28,11 @@ export interface ArtifactPredecessor { ref: StrongRef branch?: string prUrl?: string + /** True when a trusted `merge` annotation names this exact version. It is the index's answer to + * "is that pull request finished with", and the only signal the reuse decision has when no forge + * adapter is configured. Eventually consistent: the merge poller may not have written the record + * yet, which is why a configured forge always outranks it. */ + merged?: boolean } export interface PredecessorResolution { @@ -105,6 +111,14 @@ function resolvePredecessor( ...index.projects.flatMap((view) => view.artifacts), ] const byRef = new Map(allArtifacts.map((artifact) => [refKey(artifact), artifact])) + // Merged-ness of the candidate, carried alongside its links so `#launch` can decide reuse from + // the index when no forge adapter is wired. Trusted merge annotations only (the materializer has + // already filtered them); several daemons may each post an equivalent one, so this is a set. + const merged = new Set( + [...index.goals, ...index.projects].flatMap((view) => + view.merges.map((merge) => refKey(merge.value.artifact)), + ), + ) const candidates: ArtifactPredecessor[] = [] let hasUnresolvedBasedOn = false @@ -121,6 +135,7 @@ function resolvePredecessor( ref: { uri: found.uri, cid: found.cid }, ...(found.value.links?.branch ? { branch: found.value.links.branch } : {}), ...(found.value.links?.pr ? { prUrl: found.value.links.pr } : {}), + ...(merged.has(refKey(found)) ? { merged: true } : {}), }) } } @@ -386,6 +401,11 @@ export interface DispatcherDeps { * matching this project) before reusing its branch. Without it, a v2 turn always opens a fresh * `radial/impl-` branch + new PR. */ forge?: Pick + /** Does a branch still exist on the project's remote? Defaults to bundle-writer's + * `remoteBranchExists` (one `git ls-remote`, same credentials as the clone). It is what makes the + * no-forge fallback below safe: without it, a predecessor whose branch was deleted after its PR + * merged sends every retry of a v2 into the same failing clone. */ + branchExists?: (input: { gitUrl: string; branch: string }) => Promise /** Disable implementation launches when daemon GitHub possession is unavailable. */ implementationEnabled?: boolean log?: (message: string) => void @@ -400,6 +420,15 @@ export class TurnDispatcher { readonly #deps: DispatcherDeps readonly #inFlight = new Map; label: string }>() readonly #lastReject = new Map() + /** Predecessor branches currently spoken for, `branch → the request uri holding it`. Two v2 + * requests off ONE predecessor resolve the same branch, and `#inFlight` is keyed by request uri, + * so with `concurrency > 1` they would push to it at the same time. Reserved on the CANDIDATE + * branch — before the async reuse guard runs — because the reservation has to be taken + * synchronously to be a reservation at all. Over-serializing a pair whose reuse is later declined + * costs one pump interval; interleaving two turns on one branch costs the branch. */ + readonly #activeBranches = new Map() + /** Requests skipped for branch contention, so the log says it once rather than every pump. */ + readonly #contended = new Set() constructor(deps: DispatcherDeps) { this.#deps = deps @@ -430,6 +459,8 @@ export class TurnDispatcher { } }) for (const uri of this.#lastReject.keys()) if (!seenRejects.has(uri)) this.#lastReject.delete(uri) + const selected = new Set(dispatchable.map((item) => item.request.uri)) + for (const uri of this.#contended) if (!selected.has(uri)) this.#contended.delete(uri) if (dispatchable.length > 0) { this.#deps.log?.( `${dispatchable.length} dispatchable request(s); ${this.#inFlight.size}/${this.#deps.concurrency} in flight`, @@ -467,17 +498,46 @@ export class TurnDispatcher { continue } + // Contention, not an error: skip and retry on a later pump, leaving the ledger row untouched + // (this runs BEFORE markRunning, so the request stays plainly eligible). + const contested = + item.artifactType.name === IMPLEMENTATION_TYPE ? item.predecessor?.artifact?.branch : undefined + if (contested && this.#activeBranches.has(contested)) { + if (!this.#contended.has(uri)) { + this.#contended.add(uri) + this.#deps.log?.( + `not dispatching ${uri}: predecessor branch ${contested} is already being pushed by ${this.#activeBranches.get(contested)}; retrying on a later pump`, + ) + } + continue + } + this.#contended.delete(uri) + const label = turnContainerLabel(uri, cid) const runDir = this.#deps.runDirFor(uri) this.#deps.ledger.markRunning(uri, cid, { containerLabel: label, checkoutPath: join(runDir, 'checkout') }) this.#deps.log?.(`dispatching turn ${uri} (label ${label})`) + if (contested) this.#activeBranches.set(contested, uri) const promise = this.#launch(item, { anchor, runDir, label }) - .finally(() => this.#inFlight.delete(uri)) + .finally(() => { + this.#inFlight.delete(uri) + if (contested && this.#activeBranches.get(contested) === uri) this.#activeBranches.delete(contested) + }) this.#inFlight.set(uri, { promise, label }) } } + /** The injected remote probe, or a plain `git ls-remote` under this dispatcher's scheme allowlist. + * The default carries no credential, so a private repository wants the injected one (cli.ts binds + * the daemon's token into it); an unauthenticated probe there throws, and the caller's catch + * treats a throw the way it treats a missing branch — a fresh branch, logged. */ + async #branchExists(gitUrl: string, branch: string): Promise { + const probe = this.#deps.branchExists + if (probe) return probe({ gitUrl, branch }) + return remoteBranchExists({ gitUrl, branch, allowedSchemes: this.#deps.allowedSchemes }) + } + /** Prepare and run one turn, then settle the ledger. */ async #launch( item: Dispatchable, @@ -501,24 +561,61 @@ export class TurnDispatcher { // else (a "main" branch link, a cross-fork or closed/merged PR, a mismatched base) → a fresh // `radial/impl-` branch + new PR. The daemon never checks out or pushes a branch outside // the radial/impl- namespace. - if (predecessor?.prUrl && predecessor.branch?.startsWith('radial/impl-') && this.#deps.forge) { + // + // Without a forge adapter the live confirmation is unavailable, and refusing to reuse there + // was the single likeliest reason a v2 turn opened a second PR: `run.forge` is easy to omit + // and nothing said so. So the index plus the git remote answer instead — an in-namespace + // branch, a PR in this project's repo, no trusted `merge` annotation on the predecessor, and + // a branch that still exists on the remote. + // + // That last check does the work here. A `merge` annotation is written by the merge poller, + // which only runs when a forge is configured (cli.ts), so in the deployment this fallback + // exists for there is never one to find — `predecessor.merged` cannot fire, and on its own + // this would reuse a branch forever, merged or not. Reusing a merged-but-still-present branch + // is survivable (the push lands, `gh pr create` opens a PR whose diff is just the new work); + // reusing a DELETED one is not, because the clone fails identically on every retry until the + // request gives up. One `git ls-remote` is the whole difference, and it needs no forge. + // + // The one thing still invisible from here is a PR closed WITHOUT merging on a branch that + // survives; that self-corrects, because the container runs `gh pr create` when `gh pr view` + // finds no open PR for the branch. + if (predecessor?.prUrl && predecessor.branch?.startsWith('radial/impl-')) { try { const projectRepo = parseGitHubRepo(gitUrl) const projectFullName = `${projectRepo.owner}/${projectRepo.repo}` - const { repo: prRepo, number } = parseGitHubPullUrl(predecessor.prUrl) - if (prRepo.owner.toLowerCase() === projectRepo.owner.toLowerCase() && prRepo.repo.toLowerCase() === projectRepo.repo.toLowerCase()) { + const { repo: prRepo } = parseGitHubPullUrl(predecessor.prUrl) + if (prRepo.owner.toLowerCase() !== projectRepo.owner.toLowerCase() || prRepo.repo.toLowerCase() !== projectRepo.repo.toLowerCase()) { + this.#deps.log?.(`predecessor PR ${predecessor.prUrl} for ${uri} is not in ${projectFullName}; opening a fresh branch`) + } else if (this.#deps.forge) { const pr = await this.#deps.forge.getPullRequestState(predecessor.prUrl) if (pr.state === 'open' && pr.headRef === predecessor.branch && pr.headRepoFullName.toLowerCase() === projectFullName.toLowerCase() && pr.baseRef === base) { checkoutRef = predecessor.branch branch = predecessor.branch + this.#deps.log?.(`continuing ${predecessor.branch} for ${uri}: the forge confirms ${predecessor.prUrl} is open on ${base}`) } else { this.#deps.log?.(`predecessor PR ${predecessor.prUrl} for ${uri} not a clean open reuse; opening a fresh branch`) } + } else if (predecessor.merged) { + this.#deps.log?.(`predecessor ${predecessor.ref.uri} for ${uri} is recorded as merged; opening a fresh branch`) + } else if (!(await this.#branchExists(gitUrl, predecessor.branch))) { + this.#deps.log?.( + `predecessor branch ${predecessor.branch} for ${uri} no longer exists on ${gitUrl}; opening a fresh branch`, + ) + } else { + checkoutRef = predecessor.branch + branch = predecessor.branch + this.#deps.log?.( + `continuing ${predecessor.branch} for ${uri} on the index signal (no run.forge configured, no merge record for the predecessor, branch still on the remote). ` + + 'Without a forge nothing writes merge annotations, so a merged branch that still exists reads the same as an open one here, and a PR closed without merging is invisible — the turn opens a new PR if none is open for the branch.', + ) } } catch (error) { this.#deps.log?.(`could not validate predecessor PR ${predecessor.prUrl} for ${uri}: ${error instanceof Error ? error.message : String(error)}; opening a fresh branch`) + checkoutRef = undefined + branch = implBranchName(uri, cid) } } + this.#deps.ledger.markBranch(uri, branch) } else if (item.review?.commit) { // A review turn takes the non-impl path: no branch, read-only checkout, pinned to the exact // commit the subject implementation links so the reviewer reads precisely the reviewed code. diff --git a/packages/daemon/src/harness.ts b/packages/daemon/src/harness.ts index c20156d..2b37267 100644 --- a/packages/daemon/src/harness.ts +++ b/packages/daemon/src/harness.ts @@ -79,12 +79,13 @@ function buildPrompt(input: { bundleDir: string; workdir: string; implementation 'Prepare it from the checkout before implementing:', `1. \`cd ${input.workdir}\``, '2. `gh auth setup-git`. If `git ls-remote --exit-code origin "refs/heads/$RADIAL_BRANCH"` succeeds, fetch and check out that branch; otherwise create it from the current checkout. This supports both fresh work and a reused predecessor branch.', - '3. If `$RADIAL_BRANCH` is already checked out, leave it checked out. Now implement the requested change directly in that working tree.', + '3. The checkout is shallow. When `$RADIAL_BRANCH` already existed the daemon cloned it with history and fetched the base for you, so `origin/$RADIAL_BASE_BRANCH` is already there: if the branch is behind it, run `git merge "origin/$RADIAL_BASE_BRANCH"` before implementing. Should git answer `refusing to merge unrelated histories`, the clone is still too shallow to find a merge base — run `git fetch --unshallow origin` and retry the merge. Never pass `--allow-unrelated-histories`; it would graft two disconnected trees together. Read the pull request\'s cumulative diff with `gh pr diff` rather than diffing against a base you do not have.', + '4. If `$RADIAL_BRANCH` is already checked out, leave it checked out. Now implement the requested change directly in that working tree.', '', 'When your implementation is complete:', - '4. Commit your work with a `Co-Authored-By: $RADIAL_AGENT_NAME ($RADIAL_AGENT_DID) <$RADIAL_AGENT_EMAIL>` trailer and push `$RADIAL_BRANCH`.', - '5. Find an existing PR for `$RADIAL_BRANCH`; use `gh pr edit` when one exists, otherwise `gh pr create --base "$RADIAL_BASE_BRANCH"`. Its body must include the literal Markdown link `[Radial artifact]($RADIAL_ARTIFACT_URI)`.', - '6. Write a concise implementation summary to a file, then run `radial artifact submit --body-file --branch "$RADIAL_BRANCH" --commit "$(git rev-parse HEAD)" --pr "$(gh pr view --json url -q .url)"`.', + '5. Commit your work with a `Co-Authored-By: $RADIAL_AGENT_NAME ($RADIAL_AGENT_DID) <$RADIAL_AGENT_EMAIL>` trailer and push `$RADIAL_BRANCH`.', + '6. Find an existing PR for `$RADIAL_BRANCH`; use `gh pr edit` when one exists, otherwise `gh pr create --base "$RADIAL_BASE_BRANCH"`. Its body must include the literal Markdown link `[Radial artifact]($RADIAL_ARTIFACT_URI)`. Never open a second pull request for a branch that already has one — a reused branch means this turn continues the pull request its predecessor opened.', + '7. Write a concise implementation summary to a file, then run `radial artifact submit --body-file --branch "$RADIAL_BRANCH" --commit "$(git rev-parse HEAD)" --pr "$(gh pr view --json url -q .url)"`.', '', 'If you cannot complete the brief and need input first: run `radial message post --body ""` instead, do NOT commit, and stop.', '', diff --git a/packages/daemon/src/ledger.ts b/packages/daemon/src/ledger.ts index d0f4fe6..cd4c51a 100644 --- a/packages/daemon/src/ledger.ts +++ b/packages/daemon/src/ledger.ts @@ -11,6 +11,10 @@ export interface TurnRow { nextEligibleAt?: string containerLabel?: string checkoutPath?: string + /** The push branch the dispatcher selected for this turn: a fresh `radial/impl-` or the + * predecessor's branch it decided to continue. Recorded after the fact (`markRunning` runs before + * the reuse decision is made) so an operator can confirm which branch a turn actually took. */ + branch?: string acceptedRef?: StrongRef updatedAt: string } @@ -29,6 +33,7 @@ interface Row { next_eligible_at: string | null container_label: string | null checkout_path: string | null + branch: string | null accepted_ref_uri: string | null accepted_ref_cid: string | null updated_at: string | null @@ -43,6 +48,7 @@ function toRow(row: Row): TurnRow { ...(row.next_eligible_at ? { nextEligibleAt: row.next_eligible_at } : {}), ...(row.container_label ? { containerLabel: row.container_label } : {}), ...(row.checkout_path ? { checkoutPath: row.checkout_path } : {}), + ...(row.branch ? { branch: row.branch } : {}), ...(row.accepted_ref_uri && row.accepted_ref_cid ? { acceptedRef: { uri: row.accepted_ref_uri, cid: row.accepted_ref_cid } } : {}), @@ -68,6 +74,7 @@ export class TurnLedger { next_eligible_at TEXT, container_label TEXT, checkout_path TEXT, + branch TEXT, accepted_ref_uri TEXT, accepted_ref_cid TEXT, submission_json TEXT, @@ -78,6 +85,9 @@ export class TurnLedger { if (!columns.some((column) => column.name === 'submission_json')) { this.#database.exec('ALTER TABLE turns ADD COLUMN submission_json TEXT') } + if (!columns.some((column) => column.name === 'branch')) { + this.#database.exec('ALTER TABLE turns ADD COLUMN branch TEXT') + } this.#retryBound = options.retryBound ?? 3 this.#cooldownMs = options.cooldownMs ?? 300_000 @@ -125,20 +135,32 @@ export class TurnLedger { .prepare( `INSERT INTO turns ( request_uri, request_cid, state, attempts, next_eligible_at, - container_label, checkout_path, submission_json, updated_at - ) VALUES (?, ?, 'running', 0, NULL, ?, ?, NULL, ?) + container_label, checkout_path, branch, submission_json, updated_at + ) VALUES (?, ?, 'running', 0, NULL, ?, ?, NULL, NULL, ?) ON CONFLICT(request_uri) DO UPDATE SET request_cid = excluded.request_cid, state = 'running', next_eligible_at = NULL, container_label = excluded.container_label, checkout_path = excluded.checkout_path, + branch = NULL, submission_json = NULL, updated_at = excluded.updated_at`, ) .run(uri, cid, fields.containerLabel, fields.checkoutPath, now) } + /** + * Records which branch this turn is pushing to. Separate from `markRunning` because the reuse + * decision needs a live forge call (or the index) and so is only known after the row is already + * `running`; it never changes the state, so it is safe to call on a row that has since settled. + */ + markBranch(uri: string, branch: string): void { + this.#database + .prepare('UPDATE turns SET branch = ? WHERE request_uri = ?') + .run(branch, uri) + } + markFulfilled(uri: string, ref: StrongRef): void { const old = this.get(uri) const now = this.#now() diff --git a/packages/daemon/src/turn.ts b/packages/daemon/src/turn.ts index a533cda..b996623 100644 --- a/packages/daemon/src/turn.ts +++ b/packages/daemon/src/turn.ts @@ -2,7 +2,7 @@ import { appendFile, chmod, mkdir, rm } from 'node:fs/promises' import { dirname, join } from 'node:path' import { COLLECTIONS, type ArtifactRequestRecord, type ArtifactTypeRecord, type TurnBundle, type StrongRef } from '@radial/core' import type { LoadedActor } from './actors.js' -import { checkoutCommit, checkoutRepo, writeBundle } from './bundle-writer.js' +import { checkoutCommit, checkoutRepo, MERGE_CLONE_DEPTH, writeBundle } from './bundle-writer.js' import { RADIAL_HOME_TMPFS, type ContainerOutputStream, type ContainerRunner, type ContainerSpec } from './container.js' import { composeBrief, type Harness } from './harness.js' import { TurnSocketServer, implArtifactRkey, planArtifactRkey, type TurnMode, type TurnObservation, type TurnRequestAnchor, type TurnRequestContext } from './turn-socket.js' @@ -115,6 +115,10 @@ export interface TurnDeps { dest: string allowedSchemes: string[] githubToken?: string + /** Set only for a continued implementation branch, which must merge and so needs real history. */ + depth?: number + /** Refs fetched alongside the clone as `origin/` — the base branch, for that same merge. */ + alsoFetch?: string[] }) => Promise<{ path: string; commit: string }> /** Commit-pinned checkout, used for review turns whose subject links an exact commit. Defaults to * bundle-writer's checkoutCommit. */ @@ -389,12 +393,22 @@ export async function runTurn(input: TurnInput, deps: TurnDeps): Promise`, ready to merge. + const continuing = isImpl && ref !== input.bundle.project.defaultBranch const cloned = await doCheckout({ gitUrl: input.bundle.project.gitUrl, - ref: input.checkoutRef ?? input.bundle.project.defaultBranch, + ref, dest: checkoutDir, allowedSchemes: input.allowedSchemes, ...(deps.githubToken ? { githubToken: deps.githubToken } : {}), + ...(continuing + ? { depth: MERGE_CLONE_DEPTH, alsoFetch: [input.bundle.project.defaultBranch] } + : {}), }) baseCommit = cloned.commit } diff --git a/packages/daemon/test/bundle-writer.test.mjs b/packages/daemon/test/bundle-writer.test.mjs index ee201a3..0c4fcd0 100644 --- a/packages/daemon/test/bundle-writer.test.mjs +++ b/packages/daemon/test/bundle-writer.test.mjs @@ -4,7 +4,15 @@ import { mkdtemp, readFile, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { it } from 'node:test' -import { assertGitUrlAllowed, checkoutCommit, checkoutRepo, writeBundle } from '../dist/index.js' +import { + assertGitUrlAllowed, + checkoutCommit, + checkoutRepo, + MERGE_CLONE_DEPTH, + remoteBranchExists, + scrubbedGitEnv, + writeBundle, +} from '../dist/index.js' const bundle = { request: { uri: 'at://req/1', cid: 'req-cid', type: 'plan', basedOn: [] }, @@ -192,6 +200,124 @@ it('does not expose a GitHub token to a non-GitHub checkout', async () => { assert.equal(calls[0].opts.env.GIT_CONFIG_COUNT, undefined) }) +it('never inherits an ambient GitHub token — only a validated github.com checkout is given one', () => { + // Radial's own turn containers export GH_TOKEN, and a daemon running in one would otherwise hand + // its token to a checkout of any host at all. `gitEnvForCheckout` puts it back for github.com + // https, and nowhere else — which is what the test above asserts. + const scrubbed = scrubbedGitEnv({ GH_TOKEN: 'ghp_ambient', GITHUB_TOKEN: 'ghp_ambient2', PATH: '/usr/bin' }) + assert.equal(scrubbed.GH_TOKEN, undefined) + assert.equal(scrubbed.GITHUB_TOKEN, undefined) + assert.equal(scrubbed.PATH, '/usr/bin') +}) + +it('a continued branch is cloned deep and brings its base down as origin/, so it can merge', async () => { + // A --depth 1 clone grafts its tip as parentless: `git merge` against the base then fails with + // "refusing to merge unrelated histories", which is precisely the case a reused branch is in. + // Deepening the base alone does not help — the BRANCH is the grafted side. + const calls = [] + const exec = async (argv, opts) => { + calls.push({ argv, opts }) + return argv.includes('rev-parse') + ? { code: 0, stdout: 'deadbeef\n', stderr: '' } + : { code: 0, stdout: '', stderr: '' } + } + + await checkoutRepo({ + gitUrl: 'https://example.test/radial-ng.git', + ref: 'radial/impl-abc', + dest: '/tmp/continued', + depth: MERGE_CLONE_DEPTH, + alsoFetch: ['main'], + exec, + }) + + assert.equal(calls.length, 3) + assert.match(calls[0].argv.join(' '), /--depth 50 --single-branch --no-tags --branch radial\/impl-abc/) + assert.equal( + calls[1].argv.join(' '), + 'git -c core.hooksPath=/dev/null -c credential.helper= -C /tmp/continued fetch --depth 50 --no-tags origin +refs/heads/main:refs/remotes/origin/main', + ) + assert.ok(calls[2].argv.includes('rev-parse')) +}) + +it('does not re-fetch the ref it just cloned, and fails loudly when the base fetch does', async () => { + const cloned = [] + await checkoutRepo({ + gitUrl: 'https://example.test/radial-ng.git', + ref: 'main', + dest: '/tmp/same-ref', + depth: MERGE_CLONE_DEPTH, + alsoFetch: ['main'], + exec: async (argv) => { + cloned.push(argv) + return { code: 0, stdout: 'deadbeef\n', stderr: '' } + }, + }) + assert.equal(cloned.length, 2) // clone + rev-parse, no fetch + + await assert.rejects( + checkoutRepo({ + gitUrl: 'https://example.test/radial-ng.git', + ref: 'radial/impl-abc', + dest: '/tmp/bad-base', + depth: MERGE_CLONE_DEPTH, + alsoFetch: ['main'], + exec: async (argv) => + argv.includes('fetch') + ? { code: 128, stdout: '', stderr: "fatal: couldn't find remote ref main" } + : { code: 0, stdout: '', stderr: '' }, + }), + /git fetch main failed \(exit 128\).*couldn't find remote ref/, + ) +}) + +it('remoteBranchExists answers from ls-remote exit codes and throws on a real failure', async () => { + const calls = [] + const probe = (result) => async (argv, opts) => { + calls.push({ argv, opts }) + return result + } + + assert.equal( + await remoteBranchExists({ + gitUrl: 'https://github.com/o/r.git', + branch: 'radial/impl-v1', + githubToken: 'secret-token', + exec: probe({ code: 0, stdout: 'sha\trefs/heads/radial/impl-v1\n', stderr: '' }), + }), + true, + ) + // The token travels in the environment, never in argv — same rule as the clone. + assert.equal(calls[0].argv.join(' ').includes('secret-token'), false) + assert.equal(calls[0].opts.env.GH_TOKEN, 'secret-token') + assert.deepEqual(calls[0].argv.slice(-4), ['--exit-code', '--heads', 'https://github.com/o/r.git', 'refs/heads/radial/impl-v1']) + + // Exit 2 is git's "no matching refs": the branch is gone, which is an answer, not a failure. + assert.equal( + await remoteBranchExists({ + gitUrl: 'https://github.com/o/r.git', + branch: 'radial/impl-gone', + exec: probe({ code: 2, stdout: '', stderr: '' }), + }), + false, + ) + + await assert.rejects( + remoteBranchExists({ + gitUrl: 'https://github.com/o/r.git', + branch: 'radial/impl-v1', + exec: probe({ code: 128, stdout: '', stderr: 'fatal: repository not found' }), + }), + /git ls-remote failed \(exit 128\)/, + ) + + // The URL is validated before any process starts, exactly as a checkout's is. + await assert.rejects( + remoteBranchExists({ gitUrl: 'file:///etc', branch: 'x', exec: probe({ code: 0, stdout: '', stderr: '' }) }), + /scheme "file" is not allowed/, + ) +}) + const SHA = '1234567890abcdef1234567890abcdef12345678' it('checkoutCommit inits, fetches the exact sha, and checks out FETCH_HEAD (hardened, GIT_* scrubbed)', async () => { @@ -343,3 +469,72 @@ it('checkoutRepo clones a real local repo over file:// (opt-in) and resolves a r const readme = await readFile(join(dest, 'README.md'), 'utf8') assert.equal(readme, '# test repo\n') }) + +it('a continued-branch checkout can actually merge its base — against real git, over file://', async () => { + // The assertion the string-shaped tests could not make. A --depth 1 clone of the reused branch + // grafts its tip as parentless, so `git merge` finds no merge base and refuses; the merge is the + // first thing a continuing turn does, so that failure lands on exactly the case reuse exists for. + // Both halves are exercised here: the shallow clone fails, the checkout the daemon now makes for + // a continued branch succeeds. + const source = await mkdtemp(join(tmpdir(), 'radial-continued-')) + const run = (args, cwd = source) => { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) + return result + } + const ok = (args, cwd = source) => { + const result = run(args, cwd) + assert.equal(result.status, 0, result.stderr) + return result + } + ok(['init', '--initial-branch=main']) + ok(['config', 'user.email', 'test@example.test']) + ok(['config', 'user.name', 'Radial Test']) + await writeFile(join(source, 'README.md'), '# v1\n') + ok(['add', '.']) + ok(['commit', '-m', 'base']) + // The predecessor's branch, then the base moving on beneath it — the ordinary state of a branch a + // review has been sitting on. + ok(['checkout', '-b', 'radial/impl-v1']) + await writeFile(join(source, 'feature.md'), 'work\n') + ok(['add', '.']) + ok(['commit', '-m', 'predecessor work']) + ok(['checkout', 'main']) + await writeFile(join(source, 'other.md'), 'someone else\n') + ok(['add', '.']) + ok(['commit', '-m', 'base moves on']) + + const mergeIn = async (dest) => { + ok(['config', 'user.email', 'test@example.test'], dest) + ok(['config', 'user.name', 'Radial Test'], dest) + return run(['merge', 'origin/main'], dest) + } + + // What a plain shallow clone can do about it: nothing. + const shallow = join(await mkdtemp(join(tmpdir(), 'radial-shallow-')), 'checkout') + await checkoutRepo({ + gitUrl: `file://${source}`, + ref: 'radial/impl-v1', + dest: shallow, + alsoFetch: ['main'], + allowedSchemes: ['https', 'file'], + }) + const refused = await mergeIn(shallow) + assert.notEqual(refused.status, 0) + assert.match(refused.stderr, /unrelated histories/) + + // And what the continued-branch checkout does: merges. + const deep = join(await mkdtemp(join(tmpdir(), 'radial-deep-')), 'checkout') + await checkoutRepo({ + gitUrl: `file://${source}`, + ref: 'radial/impl-v1', + dest: deep, + depth: MERGE_CLONE_DEPTH, + alsoFetch: ['main'], + allowedSchemes: ['https', 'file'], + }) + const merged = await mergeIn(deep) + assert.equal(merged.status, 0, merged.stderr) + // Both sides are present afterwards: the predecessor's work and the base it was behind. + assert.equal(await readFile(join(deep, 'feature.md'), 'utf8'), 'work\n') + assert.equal(await readFile(join(deep, 'other.md'), 'utf8'), 'someone else\n') +}) diff --git a/packages/daemon/test/cli.test.mjs b/packages/daemon/test/cli.test.mjs index fc98472..9d4a2f9 100644 --- a/packages/daemon/test/cli.test.mjs +++ b/packages/daemon/test/cli.test.mjs @@ -3,7 +3,7 @@ import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { it } from 'node:test' -import { resetRunIndex, resolveRunConfig } from '../dist/cli.js' +import { forgeAdvisory, resetRunIndex, resolveRunConfig } from '../dist/cli.js' // `egress.start()` itself is Docker-only and is not exercised here (see docker-smoke.test.mjs, // which is env-gated behind RADIAL_DOCKER_TESTS). What *is* unit-testable without Docker is that @@ -85,3 +85,43 @@ it('resolveRunConfig applies check-runner defaults (checkImage falls back to the // checkImage defaults to the resolved run image when not set explicitly. assert.equal(resolved.checkImage, 'radial-turn:custom') }) + +it('warns, in the operator’s terms, when implementations run without a forge adapter', () => { + const path = '/home/op/.config/radial/radial.json' + const missing = forgeAdvisory({ + adapter: false, + configured: false, + producesImplementation: true, + configPath: path, + }) + // The consequence, not the mechanism: this is the setting that decides whether resolving a + // review's findings updates the pull request under review or opens a second one. + assert.match(missing, /no run\.forge configured/) + assert.match(missing, /implementation v2 decides whether to continue its predecessor/) + assert.match(missing, /closed without merging is invisible/) + // And the part an operator cannot infer: with no forge nothing writes merge annotations, so the + // merge check the fallback appears to make is one it can never actually make. + assert.match(missing, /nothing writes merge annotations without a forge/) + assert.ok(missing.includes(path)) + + // Configured but unauthenticated is the same consequence for a different reason. + const unauthenticated = forgeAdvisory({ + adapter: false, + configured: true, + producesImplementation: true, + configPath: path, + }) + assert.match(unauthenticated, /GitHub authentication is unavailable/) + assert.match(unauthenticated, /continue its predecessor/) + + // Silence where there is nothing to warn about: a live adapter, or a daemon that produces no + // implementations at all. + assert.equal( + forgeAdvisory({ adapter: true, configured: true, producesImplementation: true, configPath: path }), + undefined, + ) + assert.equal( + forgeAdvisory({ adapter: false, configured: false, producesImplementation: false, configPath: path }), + undefined, + ) +}) diff --git a/packages/daemon/test/config.test.mjs b/packages/daemon/test/config.test.mjs index a530ea9..91f90f6 100644 --- a/packages/daemon/test/config.test.mjs +++ b/packages/daemon/test/config.test.mjs @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { mkdtemp, mkdir, writeFile } from 'node:fs/promises' +import { mkdtemp, mkdir, readFile, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { it } from 'node:test' @@ -166,6 +166,39 @@ it('scaffolds a config that round-trips straight into agent registration', async for (const profile of Object.keys(loaded.agents)) assertProfileName(profile) }) +it('scaffolds a run block with the forge already wired, and preserves it across a round trip', async () => { + // `run.forge` is the setting that decides whether resolving a review's findings updates the pull + // request under review or opens a second one. It was neither emitted by the builder nor + // serialized by the writer, so every operator hand-wrote `run` and it was easy to omit. + const directory = await mkdtemp(join(tmpdir(), 'radial-scaffold-run-')) + const path = join(directory, 'radial.json') + await writeConfig(path, buildDefaultConfig('agent.example')) + + const raw = JSON.parse(await readFile(path, 'utf8')) + assert.deepEqual(raw.run, { spaces: [], forge: { kind: 'github' } }) + // Top-down: identity, defaults, run, agents. + assert.deepEqual(Object.keys(raw), ['identifier', 'harness', 'models', 'run', 'agents']) + + const loaded = await loadConfig(path) + assert.deepEqual(loaded.run, { spaces: [], forge: { kind: 'github' } }) + + // And it survives being written back out — a `run` the writer drops is a `forge` an operator + // loses the next time anything rewrites the file. + const again = join(directory, 'again.json') + await writeConfig(again, loaded) + assert.deepEqual((await loadConfig(again)).run, { spaces: [], forge: { kind: 'github' } }) +}) + +it('parses an empty run.spaces (the scaffold) but still demands an array', () => { + // A scaffold nothing can load is a scaffold nothing can edit: `radiald init`, `turn reset` and + // friends all have to work on a file whose spaces the operator has not filled in yet. Only + // `radiald run` needs a space, and it is the one that refuses. + assert.deepEqual(parseRunConfig({ spaces: [] }).spaces, []) + assert.throws(() => parseRunConfig({}), /run\.spaces must be an array/) + assert.throws(() => parseRunConfig({ spaces: 'at://x' }), /run\.spaces must be an array/) + assert.throws(() => parseRunConfig({ spaces: ['nope'] }), /run\.spaces entries must be "at:\/\/" URIs/) +}) + // --- FIX #11 / FIX #12: run.memory and run.allowlist parsing -------------- it('parseRunConfig validates and threads through run.memory', () => { diff --git a/packages/daemon/test/dispatch.test.mjs b/packages/daemon/test/dispatch.test.mjs index 1403360..7ac2897 100644 --- a/packages/daemon/test/dispatch.test.mjs +++ b/packages/daemon/test/dispatch.test.mjs @@ -1673,3 +1673,333 @@ it('rejects an ambiguous project-scoped request by anchoring the explanation to void launched ledger.close() }) + +// --- v2 branch reuse without a forge adapter, and branch contention ---------- + +/** A trusted merge annotation naming an implementation artifact. */ +const mergeOf = (art, rkey) => + mk(AGENT, COLLECTIONS.merge, rkey, `cid-${rkey}`, { + $type: COLLECTIONS.merge, + artifact: ref(art), + pr: art.value.links.pr, + mergedAt: '2026-01-01T00:03:00Z', + createdAt: '2026-01-01T00:03:00Z', + }) + +/** The v2 reuse scenario, on a github project so the repo guard can run. */ +function reuseScenario({ predecessorLinks, extra = [], basedOn } = {}) { + const { goal } = buildScenario() + const predecessor = implPredecessor(goal, 'impl-v1', predecessorLinks) + const { records, request } = buildScenario({ + requestOverrides: { type: 'implementation', basedOn: basedOn ?? [ref(predecessor.art)] }, + extra: [IMPL_TYPE, predecessor.req, predecessor.art, ...extra(predecessor)], + }) + const githubRecords = records.map((record) => + record.collection === COLLECTIONS.project + ? { ...record, value: { ...record.value, gitUrl: 'https://github.com/o/r.git' } } + : record, + ) + return { predecessor, request, index: materialize(store(githubRecords), { spaceUri: SPACE_URI }) } +} + +function reuseDispatcher(ledger, { forge, branchExists = async () => true } = {}) { + const captured = [] + const logs = [] + const dispatcher = new TurnDispatcher({ + ledger, + concurrency: 2, + runDirFor: () => '/tmp/radial-v2-index', + image: 'radial-turn:test', + timeoutMs: 30_000, + allowedSchemes: ['https'], + implementationEnabled: true, + ...(forge ? { forge } : {}), + // No test may reach the real `git ls-remote`; the default says the branch is still there, which + // is the state every pre-existing assertion here was written against. + branchExists, + log: (message) => logs.push(message), + runTurn: async (input) => { + captured.push(input) + return { + outcome: 'fulfilled', + acceptedRef: { uri: `at://did:plc:agent/artifact/${captured.length}`, cid: `cid-${captured.length}` }, + label: 'radial.turn.impl', + } + }, + }) + return { dispatcher, captured, logs } +} + +it('resolves a predecessor’s merged-ness off the index, so reuse can be decided without a forge', () => { + const links = { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' } + const unmerged = reuseScenario({ predecessorLinks: links, extra: () => [] }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + assert.equal( + selectDispatchable(unmerged.index, actors, ledger)[0].predecessor.artifact.merged, + undefined, + ) + + const merged = reuseScenario({ + predecessorLinks: links, + extra: (predecessor) => [mergeOf(predecessor.art, 'mrg-1')], + }) + assert.equal( + selectDispatchable(merged.index, actors, ledger)[0].predecessor.artifact.merged, + true, + ) + ledger.close() +}) + +it('with no forge adapter, continues an in-namespace branch whose predecessor carries no merge record', async () => { + // The single likeliest reason a v2 opened a second pull request: `run.forge` is easy to omit, the + // reuse branch was unreachable without it, and nothing said so. The index answers instead. + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const { dispatcher, captured, logs } = reuseDispatcher(ledger) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(captured[0].checkoutRef, 'radial/impl-v1') + assert.equal(captured[0].branch, 'radial/impl-v1') + assert.equal(ledger.get(scenario.request.uri).branch, 'radial/impl-v1') + // The log says which signal it used and what that signal cannot see, rather than claiming certainty. + const line = logs.find((entry) => entry.includes('on the index signal')) + assert.ok(line, `expected an index-signal log, got: ${logs.join(' | ')}`) + assert.match(line, /closed without merging/) + ledger.close() +}) + +it('with no forge adapter, opens a fresh branch when the predecessor’s branch is gone from the remote', async () => { + // The guard that actually protects the no-forge fallback. `merged` cannot fire there — merge + // annotations are written by the merge poller, which only runs with a forge configured — so + // without this probe a v2 off a merged predecessor whose head branch was deleted would clone a + // ref that no longer exists, crash, and crash identically on every retry until it gave up. + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const probed = [] + const { dispatcher, captured, logs } = reuseDispatcher(ledger, { + branchExists: async (input) => { + probed.push(input) + return false + }, + }) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.deepEqual(probed, [{ gitUrl: 'https://github.com/o/r.git', branch: 'radial/impl-v1' }]) + assert.equal(captured[0].checkoutRef, undefined) + assert.equal(captured[0].branch, implBranchName(scenario.request.uri, scenario.request.cid)) + assert.equal(ledger.get(scenario.request.uri).branch, captured[0].branch) + assert.ok( + logs.some((entry) => entry.includes('no longer exists on https://github.com/o/r.git')), + logs.join(' | '), + ) + ledger.close() +}) + +it('opens a fresh branch when the branch probe itself fails, rather than gambling on the clone', async () => { + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const { dispatcher, captured, logs } = reuseDispatcher(ledger, { + branchExists: async () => { + throw new Error('ls-remote: could not read Username') + }, + }) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(captured[0].checkoutRef, undefined) + assert.equal(captured[0].branch, implBranchName(scenario.request.uri, scenario.request.cid)) + assert.ok(logs.some((entry) => entry.includes('could not read Username')), logs.join(' | ')) + ledger.close() +}) + +it('does not probe the remote when a forge can answer for the pull request itself', async () => { + // With a forge the PR state names the head ref, so the branch's existence is already established; + // the probe is a fallback, not a second opinion. + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + let probes = 0 + const { dispatcher, captured } = reuseDispatcher(ledger, { + forge: { + getPullRequestState: async () => ({ + state: 'open', + headRef: 'radial/impl-v1', + headRepoFullName: 'o/r', + baseRef: 'main', + }), + }, + branchExists: async () => { + probes += 1 + return true + }, + }) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(probes, 0) + assert.equal(captured[0].checkoutRef, 'radial/impl-v1') + ledger.close() +}) + +it('with no forge adapter, opens a fresh branch once the predecessor is recorded as merged', async () => { + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: (predecessor) => [mergeOf(predecessor.art, 'mrg-1')], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const { dispatcher, captured } = reuseDispatcher(ledger) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(captured[0].checkoutRef, undefined) + assert.equal(captured[0].branch, implBranchName(scenario.request.uri, scenario.request.cid)) + assert.equal(ledger.get(scenario.request.uri).branch, captured[0].branch) + ledger.close() +}) + +it('never reuses a branch outside the reserved namespace, forge or no forge', async () => { + const scenario = reuseScenario({ + predecessorLinks: { branch: 'main', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const { dispatcher, captured } = reuseDispatcher(ledger) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(captured[0].checkoutRef, undefined) + assert.equal(captured[0].branch, implBranchName(scenario.request.uri, scenario.request.cid)) + ledger.close() +}) + +it('records the branch a forge-confirmed reuse took on the ledger row', async () => { + const scenario = reuseScenario({ + predecessorLinks: { branch: 'radial/impl-v1', pr: 'https://github.com/o/r/pull/7' }, + extra: () => [], + }) + const ledger = new TurnLedger() + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const { dispatcher, logs } = reuseDispatcher(ledger, { + forge: { + getPullRequestState: async () => ({ + state: 'open', + headRef: 'radial/impl-v1', + headRepoFullName: 'o/r', + baseRef: 'main', + }), + }, + }) + + dispatcher.pump(scenario.index, actors) + await dispatcher.drain() + + assert.equal(ledger.get(scenario.request.uri).branch, 'radial/impl-v1') + // The success path logs too; before this only the DECLINE paths said anything. + assert.ok(logs.some((entry) => entry.includes('the forge confirms')), logs.join(' | ')) + ledger.close() +}) + +it('serializes two v2 requests that resolve the same predecessor branch', async () => { + // Both resolve one predecessor, so both would push to one branch. `#inFlight` is keyed by request + // uri and does not see it; with concurrency 2 they would interleave. + const { goal } = buildScenario() + const predecessor = implPredecessor(goal, 'impl-v1', { + branch: 'radial/impl-v1', + pr: 'https://github.com/o/r/pull/7', + }) + const second = mk(HUMAN, COLLECTIONS.artifactRequest, 'req-v2b', 'cid-req-v2b', { + $type: COLLECTIONS.artifactRequest, + goal: ref(goal), + type: 'implementation', + basedOn: [ref(predecessor.art)], + assignee: AGENT, + createdAt: '2026-01-01T00:03:00Z', + }) + const { records, request } = buildScenario({ + requestOverrides: { type: 'implementation', basedOn: [ref(predecessor.art)] }, + extra: [IMPL_TYPE, predecessor.req, predecessor.art, second], + }) + const githubRecords = records.map((record) => + record.collection === COLLECTIONS.project + ? { ...record, value: { ...record.value, gitUrl: 'https://github.com/o/r.git' } } + : record, + ) + const index = materialize(store(githubRecords), { spaceUri: SPACE_URI }) + const actors = registryFor([actorFor(AGENT, ['implementation'])]) + const ledger = new TurnLedger() + + let release + const gate = new Promise((resolve) => { + release = resolve + }) + const started = [] + const logs = [] + const dispatcher = new TurnDispatcher({ + ledger, + concurrency: 2, + runDirFor: () => '/tmp/radial-v2-contended', + image: 'radial-turn:test', + timeoutMs: 30_000, + allowedSchemes: ['https'], + implementationEnabled: true, + branchExists: async () => true, + log: (message) => logs.push(message), + runTurn: async (input) => { + started.push(input.request.uri) + await gate + return { + outcome: 'fulfilled', + acceptedRef: { uri: `at://did:plc:agent/artifact/${started.length}`, cid: `cid-${started.length}` }, + label: 'radial.turn.impl', + } + }, + }) + + // Both are dispatchable and concurrency is 2, so only the branch guard can hold the second back. + assert.equal(selectDispatchable(index, actors, ledger).length, 2) + dispatcher.pump(index, actors) + await new Promise((resolve) => setImmediate(resolve)) + assert.equal(started.length, 1) + assert.ok(logs.some((entry) => entry.includes('is already being pushed by')), logs.join(' | ')) + // Contention is not a failure: the skipped request's ledger row is untouched and still eligible. + const waiting = [request.uri, second.uri].find((uri) => uri !== started[0]) + assert.equal(ledger.get(waiting), undefined) + assert.equal(ledger.eligible(waiting), true) + + release() + await dispatcher.drain() + + // Once the branch is free the next pump takes it. + dispatcher.pump(index, actors) + await new Promise((resolve) => setImmediate(resolve)) + assert.deepEqual(started.slice(1), [waiting]) + release() + await dispatcher.drain() + ledger.close() +}) diff --git a/packages/daemon/test/harness.test.mjs b/packages/daemon/test/harness.test.mjs index cd60ab2..497e5db 100644 --- a/packages/daemon/test/harness.test.mjs +++ b/packages/daemon/test/harness.test.mjs @@ -132,4 +132,20 @@ it('implementation prompt owns the GitHub branch, commit, push, and PR workflow' assert.match(prompt, /--branch "\$RADIAL_BRANCH" --commit/) assert.match(prompt, /--pr "\$\(gh pr view/) assert.match(prompt, /Do not merge/) + + // A reused branch arrives cloned with history and with its base already fetched (turn.ts), so the + // merge is `git merge origin/` and not a fetch-then-merge of FETCH_HEAD — which, against a + // --depth 1 clone, git refuses as unrelated histories. The escape hatch for a fork point deeper + // than the clone is --unshallow, never --allow-unrelated-histories. + assert.match(prompt, /git merge "origin\/\$RADIAL_BASE_BRANCH"/) + assert.match(prompt, /refusing to merge unrelated histories/) + assert.match(prompt, /git fetch --unshallow origin/) + assert.match(prompt, /Never pass `--allow-unrelated-histories`/) + assert.match(prompt, /gh pr diff/) + assert.ok( + prompt.indexOf('git merge "origin/$RADIAL_BASE_BRANCH"') < prompt.indexOf('implement the requested change'), + 'the base must be merged in before files are changed', + ) + // And never a second pull request for a branch that already has one — the whole point of reuse. + assert.match(prompt, /Never open a second pull request for a branch that already has one/) }) diff --git a/packages/daemon/test/ledger.test.mjs b/packages/daemon/test/ledger.test.mjs index f70b461..029dd5f 100644 --- a/packages/daemon/test/ledger.test.mjs +++ b/packages/daemon/test/ledger.test.mjs @@ -184,3 +184,54 @@ it('migrates legacy finishing handoffs and submission-bearing gave_up rows back await rm(dir, { recursive: true, force: true }) } }) + +it('adds the branch column to an existing database and records which branch a turn took', async () => { + // Which branch a turn pushed to is the fact an operator needs to confirm a v2 continued its + // predecessor's pull request rather than opening a second one, and it was nowhere: the row + // carried only the container label and the checkout path. + const dir = await mkdtemp(join(tmpdir(), 'radial-ledger-branch-')) + const path = join(dir, 'ledger.sqlite') + const database = new DatabaseSync(path) + database.exec(` + CREATE TABLE turns ( + request_uri TEXT PRIMARY KEY, + request_cid TEXT NOT NULL, + state TEXT, + attempts INTEGER NOT NULL DEFAULT 0, + next_eligible_at TEXT, + container_label TEXT, + checkout_path TEXT, + accepted_ref_uri TEXT, + accepted_ref_cid TEXT, + submission_json TEXT, + updated_at TEXT + ) STRICT; + `) + database + .prepare('INSERT INTO turns (request_uri, request_cid, state, attempts, updated_at) VALUES (?, ?, ?, ?, ?)') + .run('at://req/old', 'cid-old', 'fulfilled', 1, '2026-01-01T00:00:00Z') + database.close() + + try { + const ledger = new TurnLedger(path) + assert.equal(ledger.get('at://req/old').branch, undefined) + + ledger.markRunning('at://req/1', 'c1', { containerLabel: 'l', checkoutPath: '/tmp/x' }) + assert.equal(ledger.get('at://req/1').branch, undefined) + ledger.markBranch('at://req/1', 'radial/impl-abc123def456') + assert.equal(ledger.get('at://req/1').branch, 'radial/impl-abc123def456') + // Settling never drops it — the row is the record of what the turn did. + ledger.markFulfilled('at://req/1', { uri: 'at://a/b/c', cid: 'cid' }) + assert.equal(ledger.get('at://req/1').branch, 'radial/impl-abc123def456') + ledger.close() + + // It survives a reopen, and a retry re-decides rather than inheriting the old answer. + const reopened = new TurnLedger(path) + assert.equal(reopened.get('at://req/1').branch, 'radial/impl-abc123def456') + reopened.markRunning('at://req/1', 'c1', { containerLabel: 'l2', checkoutPath: '/tmp/y' }) + assert.equal(reopened.get('at://req/1').branch, undefined) + reopened.close() + } finally { + await rm(dir, { recursive: true, force: true }) + } +}) diff --git a/packages/daemon/test/turn.test.mjs b/packages/daemon/test/turn.test.mjs index daa51e4..a34247e 100644 --- a/packages/daemon/test/turn.test.mjs +++ b/packages/daemon/test/turn.test.mjs @@ -241,6 +241,60 @@ it('implementation turn injects only operator GitHub/model credentials and synch }) }) +it('a continued implementation branch is cloned with history and with its base fetched; every other turn stays shallow', async () => { + // The reused branch is the one checkout that has to MERGE, and a --depth 1 clone cannot: its tip + // is grafted as parentless, so git refuses the merge as unrelated histories. The fix has to be + // here, in the clone — the prompt cannot deepen a history the container was never given. + const { actor } = await makeActor('did:plc:agent-continued') + const implementationBundle = { + ...BUNDLE, + request: { ...BUNDLE.request, type: 'implementation' }, + project: { ...BUNDLE.project, gitUrl: 'https://github.com/acme/radial-ng.git' }, + } + const runner = new FakeContainerRunner(async () => ({ exitCode: 1, timedOut: false })) + const checkouts = [] + const spy = async (input) => { + checkouts.push(input) + await mkdir(input.dest, { recursive: true }) + return { path: input.dest, commit: 'deadbeef' } + } + const impl = (extra) => + baseInput('', { + actor, + bundle: implementationBundle, + artifactType: IMPL_ARTIFACT_TYPE, + requestRecord: IMPL_REQUEST_RECORD, + branch: 'radial/impl-v1', + ...extra, + }) + + await withRunDir(async (runDir) => { + await runTurn({ ...impl({ checkoutRef: 'radial/impl-v1' }), runDir }, { + runner, + harness: new ClaudeCodeHarness(), + checkout: spy, + }) + }) + assert.equal(checkouts[0].ref, 'radial/impl-v1') + assert.equal(checkouts[0].depth, 50) + assert.deepEqual(checkouts[0].alsoFetch, ['main']) + + // A fresh implementation branch is cut from the base itself: nothing to merge, nothing to deepen. + await withRunDir(async (runDir) => { + await runTurn({ ...impl({}), runDir }, { runner, harness: new ClaudeCodeHarness(), checkout: spy }) + }) + assert.equal(checkouts[1].ref, 'main') + assert.equal(checkouts[1].depth, undefined) + assert.equal(checkouts[1].alsoFetch, undefined) + + // And a plan turn's read-only checkout is untouched by any of this. + await withRunDir(async (runDir) => { + await runTurn(baseInput(runDir, { actor }), { runner, harness: new ClaudeCodeHarness(), checkout: spy }) + }) + assert.equal(checkouts[2].depth, undefined) + assert.equal(checkouts[2].alsoFetch, undefined) +}) + it('crashed: a container that produces no socket observation is classified crashed regardless of exit code', async () => { const { actor } = await makeActor('did:plc:agent-crash') await withRunDir(async (runDir) => { diff --git a/packages/ui/src/lib/components/NewRequest.svelte b/packages/ui/src/lib/components/NewRequest.svelte index 804bbf1..eab916e 100644 --- a/packages/ui/src/lib/components/NewRequest.svelte +++ b/packages/ui/src/lib/components/NewRequest.svelte @@ -6,6 +6,7 @@ import { capableAgents, captureBasis, + prContinuity, requestArgs, softPolicyNote, successorLabel, @@ -35,10 +36,19 @@ const type = $derived(artifactTypes(space.index).get(draft.type)) const label = $derived(typeLabel(draft.type)) - // The exact version this is asked against, named as the human reads it. A basis of the same type - // is a successor — a v3, or for an append-only type a record that supersedes rather than revises. - const basis = $derived(draft.basedOn ? findVersion(space.index, draft.basedOn) : undefined) + // The exact versions this is asked against, named as the human reads them. A basis of the same + // type is the successor's predecessor — a v3, or for an append-only type a record that supersedes + // rather than revises; the rest is inherited provenance (the plan a v2 is still built on). + const bases = $derived( + (draft.basedOn ?? []) + .map((reference) => findVersion(space.index, reference)) + .filter((found) => found !== undefined), + ) + const basis = $derived(bases.find((found) => found.unit.type === draft.type) ?? bases[0]) const successor = $derived(basis !== undefined && basis.unit.type === draft.type) + // Which pull request this lands in, read off the predecessor and hedged: a version chain is a + // branch is a pull request, and only the daemon can see whether that one is still open. + const continuity = $derived(successor ? prContinuity(basis?.version) : undefined) // A capture (§8) is this same card, opened from a goal artifact and written against the project. // It is derived from the draft rather than flagged on it, by the rule `capturedFrom()` reads back @@ -122,19 +132,24 @@ {:else}

{type?.outputSpec.description ?? 'A type the registry no longer carries.'} - {#if basis} - · based on {versionLabel(basis.unit, basis.version)} + {#if bases.length > 0} + · based on {#each bases as found, position (found.version.artifact.uri)}{#if position > 0}, {/if}{versionLabel(found.unit, found.version)}{/each} {#if successor && durabilityOf(space.index, draft.type) === 'append-only'} — the new record supersedes it rather than editing it {/if} {/if} + {#if continuity} + · {continuity.note} + {/if}

{/if} - {#if warning && basis} + {#if warning}

- {softPolicyNote(versionLabel(basis.unit, basis.version))} + {softPolicyNote(versionLabel(warning.unit, warning.version))}

{/if} @@ -201,9 +216,13 @@ {busy ? 'Writing…' : captured ? 'Request the capture' : heading} - {#if draft.basedOn} + {#if draft.basedOn?.length} + - + {#each draft.basedOn as reference (reference.uri)} + + {/each} {/if} diff --git a/packages/ui/src/lib/components/UnitDetail.svelte b/packages/ui/src/lib/components/UnitDetail.svelte index 58da0ad..0af7932 100644 --- a/packages/ui/src/lib/components/UnitDetail.svelte +++ b/packages/ui/src/lib/components/UnitDetail.svelte @@ -9,10 +9,12 @@ buildableTypes, captureDraft, captureTypes, + freshChainNote, retractArgs, retractableRequest, softPolicyNote, successorAction, + successorBasis, unapproved, } from '$lib/requests.js' import { @@ -105,7 +107,10 @@ // What the successor button says: a project document that has drifted has earned "Request an // update" rather than a version number, and one already asked for says so instead of offering a // second. Both come off the registry via `drift`, never off the type's name. - const successor = $derived(successorAction(space.index, unit, version, drift)) + const successor = $derived(successorAction(space.index, unit, version, drift, project?.gitUrl)) + // Which pull request each button lands in. Flat hint text and a `title`, never colour: this is a + // fact about what pressing the button does, not a verdict on anything (§ the rationed verdict). + const newChain = $derived(buildable.length > 0 ? freshChainNote(version) : undefined) // Capture (§8): a goal artifact distilled into the project's standing memory. The only request // whose target is not the view it is opened from — it is written against the project, so the @@ -143,6 +148,9 @@ asking = false } + // A successor carries the whole basis: the version being read, plus that version's own other-type + // bases, so a v2 arrives with the plan its findings judge it against and not only the findings. + // A different type starts a new chain, and takes only the version it was asked off. const compose = (type: string): void => { if (!version) return openDraft({ @@ -150,7 +158,10 @@ target: view.target.uri, scope, type, - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: + type === unit.type + ? successorBasis(space.index, unit, version) + : [{ uri: version.artifact.uri, cid: version.artifact.cid }], }) } @@ -417,13 +428,15 @@ class:btn-p={successor.primary && !successor.pending} type="button" disabled={successor.pending !== undefined} - title={successor.pending ? 'Asking again would write a second competing successor' : undefined} + title={successor.pending + ? 'Asking again would write a second competing successor' + : successor.continuity?.note} onclick={() => compose(unit.type)} > {successor.pending ?? successor.label} {#each buildable as type (type.name)} - {/each} @@ -449,6 +462,12 @@ {/if} + {#if canSucceed && (successor.continuity || newChain)} + +

{successor.continuity?.note ?? ''} {newChain ?? ''}

+ {/if} diff --git a/packages/ui/src/lib/requests.test.ts b/packages/ui/src/lib/requests.test.ts index 8cdcd22..9bb2d47 100644 --- a/packages/ui/src/lib/requests.test.ts +++ b/packages/ui/src/lib/requests.test.ts @@ -2,17 +2,21 @@ import { FIXTURE_DIDS } from '@radial/core/fixture' import { describe, expect, it } from 'vitest' import { closeDraft, draftFor, openDraft } from './compose.svelte.js' import { buildFixtureSpace } from './fixture.js' +import type { UnitVersion } from '@radial/core' import { buildableTypes, capableAgents, captureBasis, captureDraft, captureTypes, + freshChainNote, + prContinuity, requestArgs, requestableTypes, retractArgs, retractableRequest, successorAction, + successorBasis, successorLabel, unapprovedBasis, type RequestDraft, @@ -43,6 +47,15 @@ const project = () => { return found } +/** The same version with a different branch link — the states the fixture has no record for. */ +const withBranch = (version: UnitVersion, branch: string): UnitVersion => ({ + ...version, + artifact: { + ...version.artifact, + value: { ...version.artifact.value, links: { ...version.artifact.value.links, branch } }, + }, +}) + describe('the menu is the registry', () => { it('offers a view exactly the types of its own scope', () => { expect(requestableTypes(space.index, 'goal').map((type) => type.name)).toEqual([ @@ -159,6 +172,223 @@ describe('the successor button on a system row', () => { }) }) +describe('which pull request the next version lands in', () => { + // A version chain is a branch is a pull request. The reader could previously only find out which + // one a successor landed in by waiting for the link to appear on the row, so every assertion here + // is about saying it on the button instead — and about not saying more than the browser knows. + const disclosure = () => unit('Surface the records the fold ignored', 'implementation') + const version = () => { + const found = disclosure().current + if (!found) throw new Error('fixture version missing') + return found + } + + it('says a successor continues the open pull request the version links, hedged', () => { + const continuity = prContinuity(version()) + expect(continuity?.continues).toBe(true) + expect(continuity?.number).toBe('263') + // The hedge is load bearing: a merge annotation is visible from here, a pull request somebody + // closed by hand is not. + expect(continuity?.note).toBe( + 'Continues PR #263 if it is still open — a closed or merged pull request gets a fresh branch and a new one.', + ) + }) + + it('says a merged pull request gets a fresh one instead', () => { + const merged = unit('Possession-based containment for turn containers', 'implementation').current + if (!merged) throw new Error('fixture version missing') + expect(merged.merged).toBe(true) + expect(prContinuity(merged)).toEqual({ + continues: false, + number: '241', + note: 'PR #241 is merged, so the next version starts a fresh branch and a new pull request.', + }) + }) + + it('refuses to promise a continuation the daemon would decline for an unreserved branch', () => { + // The daemon only checks out and pushes branches in its own `radial/impl-` namespace; a link + // naming anything else gets a fresh branch, and saying otherwise here would be a lie the reader + // discovers a quarter of an hour later. + const byHand = withBranch(version(), 'main') + expect(prContinuity(byHand)?.continues).toBe(false) + expect(prContinuity(byHand)?.note).toBe( + 'PR #263 is not on a branch Radial reserved, so the next version starts a fresh branch and a new pull request.', + ) + }) + + it('refuses to promise a continuation of a pull request outside the project\'s own repository', () => { + // The daemon's third guard: a predecessor whose PR sits on a fork gets a fresh branch. The + // hedge below ("if it is still open") does not cover that reason, so saying it would send the + // reader looking for a closed pull request that is in fact open somewhere else. + const fork = version() + const forked: UnitVersion = { + ...fork, + artifact: { + ...fork.artifact, + value: { + ...fork.artifact.value, + links: { ...fork.artifact.value.links, pr: 'https://github.com/someone/radial-ng/pull/263' }, + }, + }, + } + expect(prContinuity(forked, project().gitUrl)?.continues).toBe(false) + expect(prContinuity(forked, project().gitUrl)?.note).toBe( + 'PR #263 is not in disnet/radial-ng, so the next version starts a fresh branch and a new pull request.', + ) + // Same repository, spelled differently, is still the same repository. + expect(prContinuity(version(), 'https://github.com/DISNET/radial-ng.git')?.continues).toBe(true) + // Nothing to compare against says nothing: a caller without the project, or a remote this does + // not understand, gets the answer the other two guards give. + expect(prContinuity(forked)?.continues).toBe(true) + expect(prContinuity(forked, 'https://git.example.test/disnet/radial-ng.git')?.continues).toBe(true) + }) + + it('says nothing at all about a version that links no pull request', () => { + // A plan has no branch and no PR. Inventing "this opens a new pull request" for one would be + // the UI talking about code that does not exist. + expect(prContinuity(unit('Surface the records the fold ignored', 'plan').current)).toBeUndefined() + }) + + it('says a different type starts its own chain, and only where there is one to be separate from', () => { + expect(freshChainNote(version())).toBe('A different type starts its own chain, separate from PR #263.') + expect(freshChainNote(unit('Surface the records the fold ignored', 'plan').current)).toBeUndefined() + const merged = unit('Possession-based containment for turn containers', 'implementation').current + expect(freshChainNote(merged)).toBeUndefined() + }) + + it('does not promise a branch or a pull request for types that produce neither', () => { + // This sentence is hung on EVERY other-type button — `plan`, `security-review`, whatever an + // admin adds — and a plan turn writes `links: {}`. Only `prContinuity`, computed for a version + // that actually links one, is allowed to talk about branches and pull requests. + const note = freshChainNote(version()) ?? '' + expect(note).not.toMatch(/branch/) + expect(note).not.toMatch(/new pull request/) + }) + + it('rides on the successor action itself, so the button and the card cannot disagree', () => { + expect(successorAction(space.index, disclosure(), version(), 0).continuity?.number).toBe('263') + expect( + successorAction(space.index, disclosure(), version(), 0, project().gitUrl).continuity?.continues, + ).toBe(true) + const plan = unit('Review queue in the web UI', 'plan') + expect(successorAction(space.index, plan, plan.current, 0).continuity).toBeUndefined() + }) +}) + +describe('the successor after a verdict asking for changes', () => { + // Emphasis, never a gate: the reader has been told what is wrong and the successor is the answer, + // so it becomes the surface's one primary button. Every sibling button stays live (design §7). + const plan = () => unit('Review queue in the web UI', 'plan') + + it('is the primary button while a request_changes verdict pins the version being read', () => { + const v1 = plan().versions[0] + if (!v1) throw new Error('fixture version missing') + expect(v1.verdict).toBe('request_changes') + expect(successorAction(space.index, plan(), v1, 0).primary).toBe(true) + }) + + it('is not, on a version nobody asked for changes on', () => { + expect(plan().current?.verdict).toBe('approve') + expect(successorAction(space.index, plan(), plan().current, 0).primary).toBe(false) + }) + + it('stops being primary once a successor is already on the way', () => { + // The `pending` case takes the button out of service anyway; emphasising a disabled button + // would be the surface pointing at something it will not let the reader press. + const v1 = plan().versions[0] + const pending = unit('Review queue in the web UI', 'implementation').openRequest + if (!v1 || !pending) throw new Error('fixture record missing') + expect(successorAction(space.index, { ...plan(), openRequest: pending }, v1, 0).primary).toBe(false) + }) +}) + +describe('the whole basis a successor carries', () => { + // A v2 asked with only its predecessor arrives holding the findings against it and none of the + // plan those findings judge it against: the bundle walks `prev` within one type and never crosses + // to the plan the chain was built from. + const disclosure = () => unit('Surface the records the fold ignored', 'implementation') + + it('names the version being read first, then the other-type bases it inherits', () => { + const version = disclosure().current + if (!version) throw new Error('fixture version missing') + const planV1 = unit('Surface the records the fold ignored', 'plan').versions[0] + if (!planV1) throw new Error('fixture version missing') + + expect(successorBasis(space.index, disclosure(), version)).toEqual([ + { uri: version.artifact.uri, cid: version.artifact.cid }, + { uri: planV1.artifact.uri, cid: planV1.artifact.cid }, + ]) + expect( + requestArgs( + { + kind: 'request', + target: disclosure().target, + scope: 'goal', + type: 'implementation', + basedOn: successorBasis(space.index, disclosure(), version), + }, + { brief: '', assignee: '' }, + ).filter((argument, position, all) => all[position - 1] === '--based-on' || argument === '--based-on'), + ).toEqual([ + '--based-on', + `${version.artifact.uri}#${version.artifact.cid}`, + '--based-on', + `${planV1.artifact.uri}#${planV1.artifact.cid}`, + ]) + }) + + it('never inherits a second basis of its own type, which the daemon would reject the request for', () => { + // Two same-type bases under one goal is the ambiguity that gets a request rejected with a + // message rather than dispatched, so a button must not be able to write one. + const version = disclosure().current + if (!version) throw new Error('fixture version missing') + const impostor = { + ...version, + request: { + ...version.request, + value: { ...version.request?.value, basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }] }, + }, + } as typeof version + expect(successorBasis(space.index, disclosure(), impostor)).toEqual([ + { uri: version.artifact.uri, cid: version.artifact.cid }, + ]) + }) + + it('drops a basis that does not resolve in the reader’s own index', () => { + // An unresolved basedOn parks the request until an ingestion that may never come, so a ref this + // tab cannot see is one it declines to pass on. + const version = disclosure().current + if (!version) throw new Error('fixture version missing') + const ghost = { + ...version, + request: { + ...version.request, + value: { ...version.request?.value, basedOn: [{ uri: 'at://did:plc:nobody/x/y', cid: 'cid-ghost' }] }, + }, + } as typeof version + expect(successorBasis(space.index, disclosure(), ghost)).toEqual([ + { uri: version.artifact.uri, cid: version.artifact.cid }, + ]) + }) + + it('warns on an unapproved INHERITED basis, which is the one a successor now carries', () => { + const version = disclosure().current + const planV1 = unit('Surface the records the fold ignored', 'plan').versions[0] + if (!version || !planV1) throw new Error('fixture version missing') + expect(planV1.verdict).toBeUndefined() + const warned = unapprovedBasis(space.index, { + kind: 'request', + target: disclosure().target, + scope: 'goal', + type: 'implementation', + basedOn: successorBasis(space.index, disclosure(), version), + }) + // Not the predecessor — asking for a v2 of an unapproved v1 is precisely why a v2 is asked for. + expect(warned?.unit.type).toBe('plan') + expect(warned?.version.version).toBe(1) + }) +}) + describe('capture: the request whose target is not the view it was opened from (§8)', () => { const goalUri = () => goal('Review queue in the web UI').target.uri const source = () => { @@ -182,7 +412,7 @@ describe('capture: the request whose target is not the view it was opened from ( target: project().target.uri, scope: 'project', type: 'adr', - basedOn: source(), + basedOn: [source()], // The one thing that is not written down: where the card was composed. from: goalUri(), }) @@ -215,7 +445,7 @@ describe('capture: the request whose target is not the view it was opened from ( target: goalUri(), scope: 'goal', type: 'implementation', - basedOn: source(), + basedOn: [source()], }), ).toBeUndefined() }) @@ -230,7 +460,7 @@ describe('capture: the request whose target is not the view it was opened from ( target: project().target.uri, scope: 'project', type: 'adr', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], }), ).toBeUndefined() }) @@ -291,7 +521,7 @@ describe('soft policy: annotate, never gate (design §7)', () => { target: goal('Review queue in the web UI').target.uri, scope: 'goal', type: 'implementation', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], } expect(unapprovedBasis(space.index, draft)?.version.version).toBe(1) }) @@ -306,7 +536,7 @@ describe('soft policy: annotate, never gate (design §7)', () => { target: goal('Review queue in the web UI').target.uri, scope: 'goal', type: 'plan', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], }), ).toBeUndefined() }) @@ -322,7 +552,7 @@ describe('soft policy: annotate, never gate (design §7)', () => { target: approved.target, scope: 'goal', type: 'implementation', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], }), ).toBeUndefined() }) @@ -393,7 +623,7 @@ describe('what a card produces', () => { target, scope: 'goal', type: 'implementation', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], }, { brief: ' build it ', assignee: FIXTURE_DIDS.builder }, ), diff --git a/packages/ui/src/lib/requests.ts b/packages/ui/src/lib/requests.ts index 4113fcc..de0fecd 100644 --- a/packages/ui/src/lib/requests.ts +++ b/packages/ui/src/lib/requests.ts @@ -22,15 +22,21 @@ import type { } from '@radial/core' import { artifactTypes, durabilityOf } from '@radial/core' import type { Actor, Directory } from './directory.js' -import { findVersion, isGoalView, typeLabel } from './units.js' +import { findVersion, isGoalView, pullRequest, typeLabel } from './units.js' /** * A request that has been composed but not written. * * `target` is the goal or project the request hangs off — what `--goal` / `--project` names — and * `scope` says which, because a goal-scoped type may only be asked for against a goal. `basedOn` is - * a strongref rather than a URI: it pins the exact version the human was reading, so a v3 landing - * mid-compose cannot silently become what the turn is briefed with. + * a list of strongrefs rather than URIs: each pins the exact version the human was reading, so a v3 + * landing mid-compose cannot silently become what the turn is briefed with. + * + * It is a LIST because a successor carries more than one basis. Asking for implementation v2 off v1 + * names v1 — and v1 alone would hand the turn its predecessor's findings without the plan those + * findings are findings against, because the bundle's ancestry walk follows `prev` within one type + * and never crosses to the plan the chain was built from. So the successor inherits its + * predecessor's other-type bases too (`successorBasis`), and the CLI takes one `--based-on` each. */ export interface RequestDraft { kind: 'request' @@ -38,7 +44,7 @@ export interface RequestDraft { scope: 'goal' | 'project' /** A registry type name. */ type: string - basedOn?: StrongRef + basedOn?: StrongRef[] /** * The view the card is composed in, when that is not the view it will land in. Only a capture * needs it: distilling a goal artifact writes a request against the *project*, and the card stays @@ -93,17 +99,127 @@ export function successorLabel(index: MaterializedIndex, unit: UnitView, basis?: return `Request ${typeLabel(unit.type)} v${next}` } +/** The branch namespace a daemon reserves for its own implementation turns, and the only one it + * will continue rather than replace. Mirrored from `dispatch.ts` rather than inferred: a browser + * that guessed differently would tell the reader the opposite of what the turn does. */ +const RESERVED_BRANCH_PREFIX = 'radial/impl-' + +/** + * `owner/repo`, lowercased, from either a GitHub remote (`https://github.com/o/r.git`, + * `ssh://git@github.com/o/r`) or a pull request URL (`https://github.com/o/r/pull/7`). Undefined + * for anything that is not a github.com URL with both segments — the one caller treats that as "no + * opinion" rather than as a mismatch. + */ +function repoSlug(url: string): string | undefined { + let parsed: URL + try { + parsed = new URL(url) + } catch { + return undefined + } + if (parsed.hostname.toLowerCase() !== 'github.com') return undefined + const [owner, repo] = parsed.pathname.replace(/^\//, '').split('/') + if (!owner || !repo) return undefined + return `${owner}/${repo.replace(/\.git$/, '')}`.toLowerCase() +} + +/** + * Which pull request the next version will land in — the one thing about asking for a v2 that the + * reader could not previously tell until the link appeared on the row a quarter of an hour later. + * + * It is derived entirely from the version being read, so it costs no network: a version chain is a + * branch is a pull request, and a successor continues it unless it is finished with. The hedge in + * `note` is not padding — the browser can see a merge annotation but not a pull request somebody + * closed by hand, so "if it is still open" is the honest limit of what this knows. + */ +export interface PrContinuity { + /** True when the successor is expected to push to the same branch and update the same PR. */ + continues: boolean + /** The number a human calls the PR by (`7`), when the URL ends in one. */ + number?: string + /** The one sentence the compose card and the button's title both say. */ + note: string +} + +/** + * What the successor will do about this version's pull request, or nothing where the question does + * not arise — a plan links no PR, and inventing "this opens a new pull request" for one would be + * the UI talking about code that does not exist. + */ +export function prContinuity( + version: UnitVersion | undefined, + /** The project's remote, when the caller has it. The daemon's third guard is that the pull + * request lives in the project's OWN repository, so without this the answer is one guard short. */ + gitUrl?: string, +): PrContinuity | undefined { + const pr = pullRequest(version) + if (!pr) return undefined + const named = pr.number ? `PR #${pr.number}` : 'its pull request' + const number = pr.number ? { number: pr.number } : {} + const fresh = (because: string): PrContinuity => ({ + continues: false, + ...number, + note: `${named} ${because}, so the next version starts a fresh branch and a new pull request.`, + }) + if (version?.merged) return fresh('is merged') + // The same namespace rule the daemon applies before it will check out and push a predecessor's + // branch. A link naming some other branch — `main`, or a branch a human pushed by hand — is one + // it refuses to continue, and saying otherwise here would promise what the turn will not do. + if (!version?.artifact.value.links?.branch?.startsWith(RESERVED_BRANCH_PREFIX)) { + return fresh('is not on a branch Radial reserved') + } + // And the same repository rule. A pull request on a fork is one the daemon declines to continue, + // and the hedge below ("if it is still open") would not cover that reason — the reader would be + // told the wrong thing for a reason they could not guess. Only asserted where both URLs parse as + // GitHub repositories: anywhere else this knows nothing and says nothing, which is what the + // daemon does too (its parse throws and it falls through to a fresh branch — but a browser that + // cannot parse a URL has not learned that the PR is elsewhere). + const project = gitUrl ? repoSlug(gitUrl) : undefined + const host = repoSlug(pr.url) + if (project && host && project !== host) { + return fresh(`is not in ${project}`) + } + return { + continues: true, + ...number, + note: `Continues ${named} if it is still open — a closed or merged pull request gets a fresh branch and a new one.`, + } +} + +/** + * The converse, for the buttons that ask for a DIFFERENT type off this version: they start their + * own chain, separate from this one. Only worth a sentence where the reader is looking at something + * that has a pull request to be separate from. + * + * It says chain, not branch: it is hung on EVERY other-type button, and most types produce neither. + * A plan turn gets a read-only checkout and submits `links: {}` — promising it "a fresh branch and a + * new pull request" would be the UI describing forge activity that never happens. What is true of + * all of them is that they do not continue this chain, and `prContinuity` — computed only for a + * version that actually links a pull request — is where the branch-and-PR sentence belongs. + */ +export function freshChainNote(version: UnitVersion | undefined): string | undefined { + const pr = pullRequest(version) + if (!pr || version?.merged) return undefined + return pr.number + ? `A different type starts its own chain, separate from PR #${pr.number}.` + : 'A different type starts its own chain, separate from this one.' +} + /** The successor button, once the row it sits on is taken into account. */ export interface SuccessorAction { label: string /** * True where asking for the next version is what the page is for — a project-scoped record, whose - * current version rides in every turn's bundle. Inside a goal nothing is ranked: what comes next - * after a plan is a judgement the human makes, not one the registry order makes for them. + * current version rides in every turn's bundle; or an implementation-shaped version a reviewer has + * asked for changes on, where revising is plainly what the reader came to do. Otherwise nothing + * inside a goal is ranked: what comes next after a plan is a judgement the human makes, not one + * the registry order makes for them. */ primary: boolean /** Set when one is already on the way — what the button says instead of offering a second. */ pending?: string + /** Which pull request the next version lands in, when the version being read links one. */ + continuity?: PrContinuity } /** @@ -115,6 +231,12 @@ export interface SuccessorAction { * Nothing here reads a type name: `drift` is `driftOf()`, which gates on the registry's scope and * durability, and so does everything below it. * + * A verdict of `request_changes` earns emphasis on the same terms and for the same reason: the + * reader has been told what is wrong and the successor is the answer, so it becomes the surface's + * one primary button. Emphasis only — every sibling button stays live, because reviews annotate and + * they do not gate (design §7). Once a successor is on the way the emphasis is moot, and the + * `pending` case below takes the button out of service anyway. + * * A successor already requested disables it rather than hiding it. Writing a second one is a write * that works and produces two competing v4s, which is worse than a button that explains itself. */ @@ -123,11 +245,17 @@ export function successorAction( unit: UnitView, basis: UnitVersion | undefined, drift: number, + /** The project's remote, for the same-repository guard in `prContinuity`. */ + gitUrl?: string, ): SuccessorAction { const living = durabilityOf(index, unit.type) === 'living' + const continuity = prContinuity(basis, gitUrl) const action: SuccessorAction = { label: drift > 0 && living ? 'Request an update' : successorLabel(index, unit, basis), - primary: (artifactTypes(index).get(unit.type)?.scope ?? 'goal') === 'project', + primary: + (artifactTypes(index).get(unit.type)?.scope ?? 'goal') === 'project' || + (basis?.verdict === 'request_changes' && !unit.openRequest), + ...(continuity ? { continuity } : {}), } if (unit.versions.length > 0 && unit.openRequest) { action.pending = living @@ -137,6 +265,39 @@ export function successorAction( return action } +/** + * The whole basis a successor request should carry: the version being read, and the OTHER-type + * bases of the request that version answered. + * + * The first ref is what makes it a successor — the daemon resolves the predecessor from exactly one + * same-type basedOn under the same target, stamps `prev` from it, and continues its branch. The + * inherited refs are what stops a v2 arriving with its predecessor's review findings and none of + * the plan those findings judge it against: the bundle walks `prev` within one type, so nothing + * else would carry the plan forward. + * + * Same-type refs are deliberately dropped. A second same-type basis is exactly the ambiguity the + * daemon rejects the request for, and inheriting one would turn a working button into a rejection. + * So is a ref that does not resolve in the reader's own index: an unresolvable basis parks the + * request until an ingestion that may never come. + */ +export function successorBasis( + index: MaterializedIndex, + unit: UnitView, + version: UnitVersion, +): StrongRef[] { + const self: StrongRef = { uri: version.artifact.uri, cid: version.artifact.cid } + const seen = new Set([locator(self)]) + const refs = [self] + for (const reference of version.request?.value.basedOn ?? []) { + if (seen.has(locator(reference))) continue + const named = findVersion(index, reference) + if (!named || named.unit.type === unit.type) continue + seen.add(locator(reference)) + refs.push({ uri: reference.uri, cid: reference.cid }) + } + return refs +} + // ── capture (design §8) ───────────────────────────────────────────────────────────────────────── // Distilling a goal artifact into the project's standing memory is an ordinary project-scoped // request whose `basedOn` names the goal artifact. There is no capture record, no capture flag and @@ -161,7 +322,9 @@ export function captureDraft( type: string, basedOn: StrongRef, ): RequestDraft { - return { kind: 'request', target: projectUri, scope: 'project', type, basedOn, from } + // One source, in the list every draft carries: a capture distils exactly the artifact its button + // was pressed under, and a second basis would be a second thing to distil. + return { kind: 'request', target: projectUri, scope: 'project', type, basedOn: [basedOn], from } } export interface CaptureBasis { @@ -182,10 +345,14 @@ export function captureBasis( index: MaterializedIndex, draft: RequestDraft, ): CaptureBasis | undefined { - if (draft.scope !== 'project' || !draft.basedOn) return undefined - const basis = findVersion(index, draft.basedOn) - if (!basis || !isGoalView(basis.target)) return undefined - return { unit: basis.unit, version: basis.version, goal: basis.target } + if (draft.scope !== 'project') return undefined + for (const reference of draft.basedOn ?? []) { + const basis = findVersion(index, reference) + if (basis && isGoalView(basis.target)) { + return { unit: basis.unit, version: basis.version, goal: basis.target } + } + } + return undefined } /** @@ -218,16 +385,20 @@ export const unapproved = (version: UnitVersion | undefined): boolean => * * The rule is general where the CLI's is `implementation`-shaped: building something NEW on a * version nobody approved is what deserves a note. Asking for another version of the same type is - * not — an unapproved v1 is precisely why a v2 gets asked for. + * not — an unapproved v1 is precisely why a v2 gets asked for. That the rule keys on the basis of a + * DIFFERENT type is what makes it survive a successor carrying several: a v2 inherits the plan it + * was built from, and an unapproved plan is exactly the one worth a note. */ export function unapprovedBasis( index: MaterializedIndex, draft: RequestDraft, ): { unit: UnitView; version: UnitVersion } | undefined { - if (!draft.basedOn) return undefined - const named = findVersion(index, draft.basedOn) - if (!named || named.unit.type === draft.type) return undefined - return unapproved(named.version) ? named : undefined + for (const reference of draft.basedOn ?? []) { + const named = findVersion(index, reference) + if (!named || named.unit.type === draft.type) continue + if (unapproved(named.version)) return named + } + return undefined } /** The one sentence both the drawer and the card say, so they cannot come to disagree. */ @@ -271,7 +442,9 @@ export function requestArgs( '--type', draft.type, ] - if (draft.basedOn) args.push('--based-on', locator(draft.basedOn)) + // One flag per basis, in draft order: `request create` already loops over repeated `--based-on`, + // and the order is what the drawer's provenance line reads back. + for (const reference of draft.basedOn ?? []) args.push('--based-on', locator(reference)) // A DID, never a handle: the picker already knows which identity it offered, and a handle would // be re-resolved at write time against a directory this write does not need to ask. if (fields.assignee) args.push('--assignee', fields.assignee) diff --git a/packages/ui/src/lib/write.test.ts b/packages/ui/src/lib/write.test.ts index b52084b..296071c 100644 --- a/packages/ui/src/lib/write.test.ts +++ b/packages/ui/src/lib/write.test.ts @@ -246,7 +246,7 @@ describe('requesting an artifact from the browser', () => { target: goal.target.uri, scope: 'goal', type: 'security-review', - basedOn: { uri: version.artifact.uri, cid: version.artifact.cid }, + basedOn: [{ uri: version.artifact.uri, cid: version.artifact.cid }], }, { brief: 'Look at the session boundary.', assignee: FIXTURE_DIDS.reviewer }, ), @@ -435,7 +435,7 @@ describe('judging a version from the browser', () => { target: ignored().target.uri, scope: 'goal', type: 'plan', - basedOn: { uri: v1.artifact.uri, cid: v1.artifact.cid }, + basedOn: [{ uri: v1.artifact.uri, cid: v1.artifact.cid }], }, { brief: 'Address the findings on v1.', assignee: FIXTURE_DIDS.planner }, ), diff --git a/readme.md b/readme.md index 7889a98..54c7f53 100644 --- a/readme.md +++ b/readme.md @@ -177,6 +177,31 @@ through the sidecar. An implementation turn receives a writable checkout, pushes a `radial/impl-*` branch, opens or updates a PR, and submits its branch, commit, and PR link; it never merges. +**One PR per version chain.** Asking for another version of an implementation — +which is how a review's findings get resolved — continues the same branch and +updates the same pull request rather than opening a second one. The daemon +selects the branch, and it reuses the predecessor's only when every guard holds: +the request is based on exactly one implementation of this goal, that +implementation links a branch in the daemon's own `radial/impl-` namespace and a +PR in this project's repository, and the PR is still live work. With +`run.forge` configured that last part is confirmed against GitHub (open, with +head ref, head repository and base ref all matching). Without it, the daemon +falls back to the local index plus one `git ls-remote`: no trusted `merge` +annotation on the predecessor, and the branch still on the remote. Read that +fallback honestly — merge annotations are written by the merge poller, which +only runs when a forge is configured, so with no forge there is never one to +find and the `ls-remote` is the guard doing the work. It keeps the sharp case +out: a branch deleted after its PR merged would otherwise be cloned on every +retry until the request gave up. What is left is a merged-but-undeleted branch +reading like an open one, and a PR closed without merging being invisible — the +turn recovers from the latter by opening a PR when the branch has none, and both +are why `run.forge` is in the `radiald init` scaffold. Anything else gets a fresh +`radial/impl-` branch and a new PR. A continued branch is cloned with +history and with its base fetched alongside it, because a `--depth 1` checkout +cannot merge anything. +`sqlite3 .radial-state/ledger.db "SELECT branch FROM turns WHERE request_uri = …"` +says which branch a turn actually took. + ```sh PLAN_REQUEST=$(radial request create --profile human --goal "$GOAL" \ --type plan --assignee agent.example \ -- 2.51.2