From 9bab4c21d4c7263a7eaf37cc35b464cbc32426b8 Mon Sep 17 00:00:00 2001 From: "codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie)" Date: Fri, 7 Aug 2026 00:09:16 +0000 Subject: [PATCH] Open auto-review requests to capable agents Co-Authored-By: codexbot.disnetdev.com (did:plc:hbonvqr5ysrscg5wdyb5klie) --- docs/design.md | 2 +- packages/daemon/README.md | 7 +- packages/daemon/src/auto-review.ts | 47 +++++---- packages/daemon/test/auto-review.test.mjs | 121 +++++++++++++++++++--- 4 files changed, 135 insertions(+), 42 deletions(-) diff --git a/docs/design.md b/docs/design.md index a75ef4f..c43416a 100644 --- a/docs/design.md +++ b/docs/design.md @@ -156,7 +156,7 @@ Every artifact moves through the same four moments. None of them is a state the **Fulfillment.** One turn produces exactly one artifact answering the request. Implementation-type artifacts link branch, commit, and PR (§10). -**Review.** Every artifact card has a Review button — it writes an `artifactRequest` with `type: "review"` and the artifact as `subject`; the fulfilling turn's terminal record is a `review` verdict (referencing the request) rather than an artifact. The **auto-review toggle** (per artifact type per project, overridable per request) makes the daemon write that review request automatically the moment a matching artifact lands, authored by one of its agent identities (§3). The reviewer needs no distinct identity — the same agent DID may review its own artifact: what buys a review its value is a fresh context (every turn starts from nothing, §9) plus, when the operator configures it, a different model routed to the `review` type (§11). Auto-review is deliberately terminal: a verdict never triggers further generation. It is the only automation in v1, and it is exactly one hop. +**Review.** Every artifact card has a Review button — it writes an `artifactRequest` with `type: "review"` and the artifact as `subject`; the fulfilling turn's terminal record is a `review` verdict (referencing the request) rather than an artifact. The **auto-review toggle** (per artifact type per project, overridable per request) makes the daemon write an **open** review request automatically the moment a matching artifact lands. One eligible local agent identity authors the record (§3), but it is not assigned to that identity: ordinary claims arbitrate execution among every capable agent in the space, including agents run by other operators (§9). The reviewer needs no distinct identity — the same agent DID may still win the claim and review its own artifact: what buys a review its value is a fresh context (every turn starts from nothing, §9) plus, when the operator configures it, a different model routed to the `review` type (§11). Auto-review is deliberately terminal: a verdict never triggers further generation. It is the only automation in v1, and it is exactly one hop. **Supersession.** A `request_changes` verdict blocks nothing mechanically — it is information on the card. The human reads the findings and decides whether to request a v2 (`prev`-linked, findings included in the new turn's bundle) or move on. Radial is **review-annotated, not review-gated**: a team that wants "no implementation without an approved plan" gets it as soft UI policy (the button warns when the basis plan lacks an approving verdict), not a protocol guarantee. That's acceptable precisely because the thing gates protected against — agents autonomously spending tokens past an unapproved plan — cannot happen when every generation has a human click behind it. diff --git a/packages/daemon/README.md b/packages/daemon/README.md index 9ce053c..96dc44f 100644 --- a/packages/daemon/README.md +++ b/packages/daemon/README.md @@ -23,6 +23,11 @@ and Radial deletes no records, so "release" means "stop renewing" and a losing c it lapses. This is harmless — a loser can never displace the winner in the tie-break — but it is why the default lease is short. +Auto-review uses this open-request path. The daemon chooses one eligible local review-capable agent +identity to author the single review request, but omits `assignee`; any capable agent in the space, +including one operated elsewhere, may claim and execute it. Authorship and execution are therefore +separate, at the intended cost of the normal claim-ingestion confirmation cycle. + ## Forges The forge is a registry, selected per project by the host of its `gitUrl` (`run.forges`). GitHub is @@ -143,7 +148,7 @@ what an injected one can do is unchanged and write-side (design §13). is; every answer request is written by a human, which is what keeps a reply from commissioning the next reply (design §10, and §14's rejected alternative C). The property is asserted over the source — `test/auto-review.test.mjs`, "the daemon writes no other request": exactly one -`create(COLLECTIONS.artifactRequest)` exists under `src/`, and it builds a `review`. +`create(COLLECTIONS.artifactRequest)` exists under `src/`, and it builds an open `review`. ## Private spaces diff --git a/packages/daemon/src/auto-review.ts b/packages/daemon/src/auto-review.ts index 1bc7110..7596c7d 100644 --- a/packages/daemon/src/auto-review.ts +++ b/packages/daemon/src/auto-review.ts @@ -95,19 +95,19 @@ export function loadedReviewActors(actors: ActorRegistry): LoadedActor[] { } /** - * The review actor is deterministic: among the ELIGIBLE actors (each holds an active AGENT-kind - * membership grant in the space and produces `review` artifacts) pick the lowest DID. Returns - * undefined when no eligible actor exists (the caller skips with a once-per-space log). + * The request writer is deterministic: among the ELIGIBLE actors (each holds an active AGENT-kind + * membership grant in the space and produces `review` artifacts) pick the lowest DID. This chooses + * the repository that authors the open request, not the agent that will execute it. */ -export function selectReviewActor(eligible: LoadedActor[]): LoadedActor | undefined { +export function selectReviewWriter(eligible: LoadedActor[]): LoadedActor | undefined { return [...eligible].sort((a, b) => compareCodePoints(a.did, b.did))[0] } export interface AutoReviewCandidate { /** The exact artifact version to request a review of (its uri#cid is the review subject). */ artifact: IndexedRecord - /** The actor whose repo the review request is written into (see `selectReviewActor`). */ - actor: LoadedActor + /** The actor whose repo the open review request is written into (see `selectReviewWriter`). */ + writer: LoadedActor /** Count of RETRACTED trusted `review` requests already pinning this exact subject — salts the * deterministic rkey so a rewrite after a retraction never collides with the tombstoned request. */ retractedCount: number @@ -137,7 +137,7 @@ export interface AutoReviewCandidate { * subject's first observation (so its subject was never in the ledger), where the rewrite must land * at a fresh rkey rather than adopt the tombstone. * - * When an artifact would be a candidate but NO eligible review actor exists, `onSkip` is invoked + * When an artifact would be a candidate but NO eligible review writer exists, `onSkip` is invoked * (deduped to once per space by the caller) AND `onDefer` records the subject key so the caller can * keep it UNOBSERVED (not ledger-marked) — otherwise it would be lost the moment a reviewer is * configured later. Ordered deterministically by subject uri. @@ -150,7 +150,7 @@ export function selectAutoReviewCandidates( onSkip?: (reason: string) => void, onDefer?: (subjectKey: string) => void, ): AutoReviewCandidate[] { - const actor = selectReviewActor(eligibleReviewActors(index, actors)) + const writer = selectReviewWriter(eligibleReviewActors(index, actors)) // An artifact's `request` backref resolves against the fulfilling requests of ANY target (a // project-scoped artifact's request lives under the project; a goal artifact's under the goal). @@ -183,7 +183,7 @@ export function selectAutoReviewCandidates( if (hasReviewRequest) continue // (b) A trusted verdict already pins this exact version (direct post-a-verdict path, no request). if (view.reviews.some((review) => refKey(review.value.subject) === key)) continue - if (!actor) { + if (!writer) { onSkip?.('no loaded actor holds an active agent membership grant and produces review artifacts') // Defer, don't drop: keep this subject unobserved so it fires once a reviewer is available. onDefer?.(key) @@ -197,7 +197,7 @@ export function selectAutoReviewCandidates( request.value.subject !== undefined && refKey(request.value.subject) === key, ).length - candidates.push({ artifact, actor, retractedCount }) + candidates.push({ artifact, writer, retractedCount }) } } @@ -252,8 +252,8 @@ export interface AutoReviewTriggerDeps { /** * Auto-review trigger (design §7), modeled on `MergePoller`: `pump` selects the artifact versions * whose effective auto-review config is on and that landed since the last tick, and fires off exactly - * one `type: "review"` `artifactRequest` per subject (authored by one of the daemon's agent - * identities, with the artifact as `subject`). Fire-and-forget: `pump` never awaits a write, so it + * one open `type: "review"` `artifactRequest` per subject (authored by one of the daemon's eligible + * agent identities, with the artifact as `subject`). Fire-and-forget: `pump` never awaits a write, so it * never blocks the ingestion loop. * * "Landed" is defined by a persisted per-space observed-subjects ledger, NOT a timestamp. Each tick, @@ -276,8 +276,9 @@ export interface AutoReviewTriggerDeps { * current index eligibility, so an actor that wrote the request and then lost its grant is still * probed); and a collision on create is likewise adopted. A record is adopted only when it matches * exactly — same subject, type `review`, goal/project target, `basedOn` of exactly `[subject]`, and - * `assignee` equal to the repo it was found in; a mismatched record in one repo does not stop the - * probe (a valid match in another repo is still adopted). ACCEPTED residual corner: if the prior + * either no `assignee` (the canonical open shape) or a legacy `assignee` equal to the repo it was + * found in; a mismatched record in one repo does not stop the probe (a valid match in another repo + * is still adopted). ACCEPTED residual corner: if the prior * writer is removed from the daemon config ENTIRELY between crash and restart, it is no longer loaded * and cannot be probed, so at most one duplicate review request may be written — the same bounded, * attributable corner `MergePoller` accepts, and harmless (a redundant review request). @@ -415,7 +416,8 @@ export class AutoReviewTrigger { /** Whether an already-existing record found in `repoDid`'s repo is the auto-review request we would * have written for this subject: same type, subject, goal/project target, `basedOn` of exactly - * `[subject]`, and self-assignment to `repoDid`. Anything else is a foreign record at the rkey. */ + * `[subject]`, and either no assignment or legacy self-assignment to `repoDid`. Anything else is a + * foreign record at the rkey. */ #adoptable( value: ArtifactRequestRecord, subject: StrongRef, @@ -432,12 +434,12 @@ export class AutoReviewTrigger { sameRef(value.subject, subject) && targetMatches && basedOnMatches && - value.assignee === repoDid + (value.assignee === undefined || value.assignee === repoDid) ) } async #write(candidate: AutoReviewCandidate, spaceUri: string, probeActors: LoadedActor[]): Promise { - const { artifact, actor, retractedCount } = candidate + const { artifact, writer, retractedCount } = candidate const subject: StrongRef = { uri: artifact.uri, cid: artifact.cid } const key = refKey(artifact) const rkey = autoReviewRkey(subject.uri, subject.cid, retractedCount) @@ -487,7 +489,6 @@ export class AutoReviewTrigger { type: REVIEW_TYPE, subject, basedOn: [subject], // provenance recorded-when-known: the subject is what this review is of. - assignee: actor.did, createdAt: this.#now(), } // This create is the daemon's ONLY `artifactRequest` write, and its type is the constant three @@ -499,17 +500,17 @@ export class AutoReviewTrigger { // fail, over the source: `test/auto-review.test.mjs`, "the daemon writes no other request". let ref: StrongRef try { - ref = await actor.client.create(COLLECTIONS.artifactRequest, record, { rkey }) + ref = await writer.client.create(COLLECTIONS.artifactRequest, record, { rkey }) } catch (error) { if (error instanceof XrpcError && error.status === 400 && error.error === 'RecordAlreadyExists') { // Raced with a write between the probe and this create: re-fetch from the selected actor. - const existing = await actor.client.getOwnRecord(COLLECTIONS.artifactRequest, rkey).catch(() => undefined) - if (existing && this.#adoptable(existing.value as ArtifactRequestRecord, subject, artifact, actor.did)) { + const existing = await writer.client.getOwnRecord(COLLECTIONS.artifactRequest, rkey).catch(() => undefined) + if (existing && this.#adoptable(existing.value as ArtifactRequestRecord, subject, artifact, writer.did)) { await this.#mark(spaceUri, key) return } this.#logOnce( - `auto-review: refusing to adopt mismatched record at ${rkey} in ${actor.did} for subject ${subject.uri}`, + `auto-review: refusing to adopt mismatched record at ${rkey} in ${writer.did} for subject ${subject.uri}`, ) return } @@ -520,7 +521,7 @@ export class AutoReviewTrigger { return } await this.#mark(spaceUri, key) - this.#deps.log?.(`auto-review: requested review of ${subject.uri} as ${actor.did}: ${ref.uri}`) + this.#deps.log?.(`auto-review: authored open review request for ${subject.uri} as ${writer.did}: ${ref.uri}`) } /** Awaits every in-flight write and ledger persist (best-effort graceful shutdown). */ diff --git a/packages/daemon/test/auto-review.test.mjs b/packages/daemon/test/auto-review.test.mjs index 3c42a9c..068319d 100644 --- a/packages/daemon/test/auto-review.test.mjs +++ b/packages/daemon/test/auto-review.test.mjs @@ -7,11 +7,16 @@ import { COLLECTIONS, MemoryRecordStore, materialize } from '../../core/dist/ind import { XrpcError } from '../../atproto/dist/index.js' import { AutoReviewTrigger, + ClaimLedger, + TurnLedger, actorTypesFor, autoReviewRkey, + claimRkey, eligibleReviewActors, selectAutoReviewCandidates, - selectReviewActor, + selectClaimable, + selectDispatchable, + selectReviewWriter, } from '../dist/index.js' const ROOT = 'did:plc:root' @@ -276,12 +281,12 @@ function trigger(dir, now, log) { const reviewActors = (creates) => registry([fakeActor(AGENT_A, ['plan', 'review'], creates)]) -describe('selectReviewActor', () => { +describe('selectReviewWriter', () => { it('picks the lowest DID among eligible actors', () => { const a = fakeActor(AGENT_A, ['review'], []) const b = fakeActor(AGENT_B, ['review'], []) - assert.equal(selectReviewActor([b, a]).did, AGENT_A) - assert.equal(selectReviewActor([]), undefined) + assert.equal(selectReviewWriter([b, a]).did, AGENT_A) + assert.equal(selectReviewWriter([]), undefined) }) it('skips a reviewer scoped out of this space, and takes it in one it does not scope', () => { @@ -518,7 +523,7 @@ describe('AutoReviewTrigger.pump', () => { assert.equal(written.value.project, undefined) assert.deepEqual(written.value.subject, { uri: art.uri, cid: art.cid }) assert.deepEqual(written.value.basedOn, [{ uri: art.uri, cid: art.cid }]) - assert.equal(written.value.assignee, AGENT_A) + assert.equal('assignee' in written.value, false) assert.equal('autoReview' in written.value, false) assert.equal(written.value.createdAt, '2026-06-01T00:00:00.000Z') assert.equal(written.rkey, autoReviewRkey(art.uri, art.cid)) @@ -732,6 +737,88 @@ describe('written request round-trips through core.materialize', () => { } }) +it('the emitted open review is claimable across operators and only the confirmed fold winner dispatches', async () => { + const { records, space, art } = buildStore({ scope: 'goal' }) + records.push(stored(ROOT, 'm-agent-b', { + $type: COLLECTIONS.addMember, + space: strongref(space), + did: AGENT_B, + kind: 'agent', + role: 'agent', + createdAt: '2026-01-01T00:00:03Z', + }, 9)) + + const seedIndex = makeIndex({ projectAutoReview: { plan: true }, members: [AGENT_A, AGENT_B] }) + const requestView = { + did: HUMAN, + uri: art.value.request.uri, + cid: art.value.request.cid, + value: { $type: COLLECTIONS.artifactRequest, type: 'plan', basedOn: [], autoReview: true, createdAt: 'x' }, + } + const liveIndex = makeIndex({ + artifacts: [{ did: AGENT_A, uri: art.uri, cid: art.cid, value: { ...art.value } }], + requests: [requestView], + projectAutoReview: { plan: true }, + members: [AGENT_A, AGENT_B], + }) + // Use the exact record emitted by AutoReviewTrigger, then feed it to the real fold and daemon gates. + liveIndex.goals[0].target = { uri: art.value.goal.uri, cid: art.value.goal.cid, value: { project: strongref(records[4]), closed: false } } + const creates = [] + const triggerUnderTest = trigger(stateDir()) + const writerRegistry = registry([fakeActor(AGENT_A, ['review'], creates), fakeActor(AGENT_B, ['review'], creates)]) + triggerUnderTest.pump(seedIndex, writerRegistry); await triggerUnderTest.drain() + triggerUnderTest.pump(liveIndex, writerRegistry); await triggerUnderTest.drain() + assert.equal(creates.length, 1) + const emitted = stored(creates[0].did, creates[0].rkey, creates[0].value, 100, 'cid-auto-review') + assert.equal('assignee' in emitted.value, false) + + const store = new MemoryRecordStore() + for (const record of [...records, emitted]) store.put(record) + const openIndex = materialize(store, { spaceUri: space.uri, asOf: '2026-08-07T12:00:00Z' }) + const claimsA = new ClaimLedger() + const claimsB = new ClaimLedger() + const turnsA = new TurnLedger() + const turnsB = new TurnLedger() + const registryA = registry([fakeActor(AGENT_A, ['review'], [])]) + const registryB = registry([fakeActor(AGENT_B, ['review'], [])]) + try { + assert.deepEqual(selectClaimable(openIndex, registryA, claimsA, turnsA, { budget: 1 }).map((c) => c.request.uri), [emitted.uri]) + assert.deepEqual(selectClaimable(openIndex, registryB, claimsB, turnsB, { budget: 1 }).map((c) => c.request.uri), [emitted.uri]) + + const claimA = stored(AGENT_A, claimRkey(emitted.uri, emitted.cid), { + $type: COLLECTIONS.claim, + request: strongref(emitted), + expiresAt: '2026-08-07T13:00:00Z', + createdAt: '2026-08-07T12:00:00Z', + }, 101, 'cid-claim-a') + const claimB = stored(AGENT_B, claimRkey(emitted.uri, emitted.cid), { + $type: COLLECTIONS.claim, + request: strongref(emitted), + expiresAt: '2026-08-07T13:00:00Z', + createdAt: '2026-08-07T12:00:01Z', + }, 102, 'cid-claim-b') + store.put(claimA); store.put(claimB) + const claimed = materialize(store, { spaceUri: space.uri, asOf: '2026-08-07T12:00:02Z' }) + assert.equal(claimed.goals[0].winningClaims[emitted.uri].did, AGENT_A) + assert.equal(selectDispatchable(claimed, registryA, turnsA, undefined, undefined, new Set([emitted.uri])).length, 1) + assert.equal(selectDispatchable(claimed, registryB, turnsB, undefined, undefined, new Set([emitted.uri])).length, 0) + assert.equal(selectDispatchable(claimed, registryA, turnsA).length, 0, 'the winner still waits for local confirmation') + + const scopedOut = fakeActor(AGENT_B, ['review'], [], { [space.uri]: { artifactTypes: ['plan'] } }) + const incapable = registry([scopedOut]) + const incapableClaims = new ClaimLedger() + const incapableTurns = new TurnLedger() + try { + assert.equal(selectClaimable(openIndex, incapable, incapableClaims, incapableTurns, { budget: 1 }).length, 0) + assert.equal(selectDispatchable(claimed, incapable, incapableTurns, undefined, undefined, new Set([emitted.uri])).length, 0) + } finally { + incapableClaims.close(); incapableTurns.close() + } + } finally { + claimsA.close(); claimsB.close(); turnsA.close(); turnsB.close() + } +}) + // --- Write/ledger boundary (cross-model review findings 1-5). --- describe('AutoReviewTrigger.pump — write/ledger boundary', () => { @@ -792,8 +879,8 @@ describe('AutoReviewTrigger.pump — write/ledger boundary', () => { const empty = makeIndex({ projectAutoReview: { plan: true }, members: [AGENT_A, AGENT_B] }) const withArt = makeIndex({ artifacts: [art], requests: [req], projectAutoReview: { plan: true }, members: [AGENT_A, AGENT_B] }) - // AGENT_B's repo already holds the matching auto-review request (assignee = AGENT_B), even though - // AGENT_A is the lowest-DID selected writer. + // AGENT_B's repo already holds the canonical open auto-review request, even though AGENT_A is + // now the lowest-DID selected writer. const rkey = autoReviewRkey(art.uri, art.cid) repoB.set(`${COLLECTIONS.artifactRequest}/${rkey}`, { uri: `at://${AGENT_B}/${COLLECTIONS.artifactRequest}/${rkey}`, @@ -804,7 +891,6 @@ describe('AutoReviewTrigger.pump — write/ledger boundary', () => { subject: { uri: art.uri, cid: art.cid }, basedOn: [{ uri: art.uri, cid: art.cid }], goal: base._goalRef, - assignee: AGENT_B, createdAt: 'x', }, }) @@ -910,9 +996,9 @@ describe('AutoReviewTrigger.pump — write/ledger boundary', () => { assert.equal(view.retracted.some((r) => r.uri === oldAuto.uri), true, 'the old request stays retracted') }) - // Finding 5: a record at the rkey is adopted only when subject, type, target, basedOn === [subject], - // AND assignee === the repo it was found in all match. A basedOn/assignee mismatch is refused. - it('refuses to adopt when basedOn or assignee mismatches', async () => { + // A differently assigned record must not be mistaken for either canonical open output or the + // historical self-assigned shape. + it('refuses to adopt a request assigned to a DID other than its repository owner', async () => { const creates = [] const logs = [] const base = makeIndex({}) @@ -921,13 +1007,13 @@ describe('AutoReviewTrigger.pump — write/ledger boundary', () => { const empty = makeIndex({ projectAutoReview: { plan: true } }) const withArt = makeIndex({ artifacts: [art], requests: [req], projectAutoReview: { plan: true } }) - // Correct subject/type/target, but basedOn is empty and assignee is not the repo owner. + // Everything matches except that assignee is not the repo owner. const store = new Map() const rkey = autoReviewRkey(art.uri, art.cid) store.set(`${COLLECTIONS.artifactRequest}/${rkey}`, { uri: `at://${AGENT_A}/${COLLECTIONS.artifactRequest}/${rkey}`, cid: 'cid-pre', - value: { $type: COLLECTIONS.artifactRequest, type: 'review', subject: { uri: art.uri, cid: art.cid }, basedOn: [], goal: base._goalRef, assignee: HUMAN, createdAt: 'x' }, + value: { $type: COLLECTIONS.artifactRequest, type: 'review', subject: { uri: art.uri, cid: art.cid }, basedOn: [{ uri: art.uri, cid: art.cid }], goal: base._goalRef, assignee: HUMAN, createdAt: 'x' }, }) const t = trigger(stateDir(), undefined, (m) => logs.push(m)) @@ -1015,9 +1101,9 @@ describe('AutoReviewTrigger.pump — re-review findings', () => { assert.equal(creates.length, 0) }) - // Finding 3: probe across ALL loaded review-capable actors, regardless of current index eligibility. - // A record written by an actor that has since lost its grant (but is still loaded) is adopted. - it('adopts from a still-loaded but index-ineligible prior writer', async () => { + // Backward compatibility: probe every loaded review-capable actor and adopt the historical shape, + // which assigned execution to the same DID whose repository authored the request. + it('adopts a legacy self-assigned request from a still-loaded prior writer', async () => { const creates = [] const logs = [] const repoA = new Map() @@ -1105,12 +1191,13 @@ describe('the daemon writes no other request', () => { assert.equal(matches.length, 1, 'auto-review writes exactly one artifactRequest') }) - it('builds that request with the review type and nothing off the wire', () => { + it('builds that request as an open review and nothing off the wire', () => { const text = sources().find((file) => file.name === 'auto-review.ts').text const opens = text.indexOf('const record: ArtifactRequestRecord = {') const record = text.slice(opens, text.indexOf('\n }\n', opens)) assert.match(record, /\n\s*type: REVIEW_TYPE,\n/, 'the request literal names the review constant') assert.match(text, /^const REVIEW_TYPE = 'review'$/m) assert.doesNotMatch(record, /answer/i) + assert.doesNotMatch(record, /assignee/, 'the sole daemon-authored request is open') }) }) -- 2.51.2