diff --git a/README.md b/README.md index 05ce6c9c..b1cbdd4d 100644 --- a/README.md +++ b/README.md @@ -792,8 +792,47 @@ mount acknowledges the provider mutation. Provider-authoritative issue reads remain optional on the writeback interface; when unavailable, their existing call sites keep their conservative fallback behavior. -This identity setting does not change Notion intake's separate GitHub issue -publisher, which still requires local `gh` authentication when enabled. +#### Writes that still shell out to `gh` + +Two Factory GitHub mutations are not represented on the connected App surface +and therefore cannot be performed as the app today. Under `"app"` they refuse +rather than writing as the operator, so an explicit app identity never produces +a human-attributed write: + +| Write | Refuses under `"app"` | Missing connected capability | +|---|---|---| +| Guarded squash merge (`mergePolicy: "on-green-with-review"`) | the merge is declined and logged; nothing is merged | `mergePullRequest` | +| Notion intake issue create | the run is blocked with the reason, before any durable claim is taken | `createIssue` | +| Notion intake issue edit | the run is blocked with the reason, before any durable claim is taken | `updateIssue` | + +Both refusals name the missing capability and the recovery path: set +`github.identity` to `"user"` or `"auto"` to deliberately accept local-user +attribution for that operation. Under `"auto"` and `"user"` both paths behave +exactly as they always have. Neither refusal is reachable in the default cloud +deployment, which runs `mergePolicy: "never"` and does not run Notion intake. + +Notion intake is a separate surface from the Factory lifecycle writeback and +still requires local `gh` authentication when enabled. Its CLI entry point +resolves `github.identity` from the selected contract — including a split +`workspaceConfig`/`nodeConfig` contract, where the node half wins — so `"app"` +refuses while `"user"` and `"auto"` proceed. An absent contract resolves to +`"auto"`, matching the schema's own synthesis of an unset `github` block; a +contract that exists but cannot be parsed is an error rather than a silent +downgrade to the permissive value. + +Only the mutations refuse. Reconciliation of an already-dispatched task +performs no GitHub write, so it continues to work under `"app"`: the refusal is +raised immediately before the issue create or the issue edit, and in the create +path before the durable delivery claim is taken, so a refused run never +consumes the exactly-once claim and can be retried under a permitted +identity. + +Read paths are deliberately unaffected. `gh pr view` carries no authorship, so +merge-gate reads, Notion intake label/visibility lookups, and the standalone +babysitter's PR metadata read (whose `source: 'gh'` provenance marker is +retained for exactly this reason) continue to work under every identity. +Review replies and pushes on a babysat PR are performed by the dispatched agent +under the agent's own credential, not by the Factory process. Authenticated Factory progress reporting is enabled by default for real CLI sessions. Factory sends privacy-bounded lifecycle events, worker ownership, diff --git a/src/cli/fleet.test.ts b/src/cli/fleet.test.ts index 935c58cb..bb9053b4 100644 --- a/src/cli/fleet.test.ts +++ b/src/cli/fleet.test.ts @@ -25,6 +25,7 @@ import { import { MountAuthScopeError, mountAuthRemediation } from '../mount/mount-auth-error' import { DocumentStateStore, FileStateStore } from '../state/file-state-store' import { FakeFleetClient, FakeMountClient, withDeadline } from '../testing' +import { GhCliIssuePublisher } from '../intake/notion' import type { GithubConnectionRead, GithubConnectionWrite, GithubIssueLookup, GithubWriteback, LocalMountOptions, SpawnInput, SpawnResult } from '../ports' import type { HarnessDriverClientLike } from '../fleet/internal-fleet-client' import type { RelayMessaging } from '@agent-relay/sdk' @@ -840,6 +841,113 @@ describe('fleet CLI runtime', () => { } }) + it('honours github.identity "app" for Notion intake instead of writing as the local gh user', async () => { + // The gate in GhCliIssuePublisher is worthless if this call site hardcodes + // an identity: this is the only production caller of runNotionIntake, so + // the refusal must be reachable from the resolved contract. + const root = await mkdtemp(join(tmpdir(), 'factory-cli-notion-identity-')) + try { + const mountedPage = join(root, 'notion', 'pages', '3b36800c-1c90-801d-b1cf-c8f2e1cff7cf') + await mkdir(mountedPage, { recursive: true }) + await writeFile(join(mountedPage, 'content.md'), [ + '# Chief Spec', + 'Status: ready', + 'Title: Verify identity gating', + 'Summary: Prove intake refuses under an app identity.', + 'Recipe: single', + 'Repos: AgentWorkforce/cloud', + ].join('\n')) + const manifestPath = join(root, 'notion.json') + await writeFile(manifestPath, JSON.stringify({ + version: 1, + mountRoot: './notion', + statePath: './state.json', + tasks: [{ page: '3b36800c1c90801db1cfc8f2e1cff7cf' }], + })) + const configPath = join(root, 'factory.config.json') + // Deliberately the SPLIT contract shape. A reader that only looked at a + // flat `github` key would miss this and fall back to `auto`, silently + // permitting the local-user write. + await writeFile(configPath, JSON.stringify({ + workspaceConfig: { repos: { org: 'AgentWorkforce', names: ['cloud'] } }, + nodeConfig: { github: { identity: 'app' } }, + })) + + const durableClaims = new Map() + const notionClaims = { + get: vi.fn(async (sourceKey: string) => durableClaims.get(sourceKey)), + findBySourcePrefix: vi.fn(async () => []), + claim: vi.fn(async (claim: { sourceKey: string; digest: string; claimedAt: string }) => { + durableClaims.set(claim.sourceKey, claim) + return { status: 'claimed' as const, claim } + }), + dispose: vi.fn(async () => undefined), + } + const output = buffer() + // Capture the identity the CLI resolved, and keep the publisher's reads + // hermetic so the refusal is the only thing that can block the task. + const resolved: string[] = [] + const notionGithub = (identity: string) => { + resolved.push(identity) + const publisher = new GhCliIssuePublisher( + identity as 'app' | 'user' | 'auto', + async () => { throw new Error('gh must not be invoked in this test') }, + ) + return Object.assign(publisher, { + repositoryVisibility: async () => 'private' as const, + missingLabels: async () => [], + findBySource: async () => undefined, + createIssue: async () => ({ number: 42, url: 'https://github.test/issues/42' }), + }) + } + + const code = await runFleetCli( + ['intake', 'notion', manifestPath, '--config', configPath], + { fleet: new FakeFleetClient(), notionClaims, notionGithub, stdout: output, stderr: buffer() }, + ) + + // The CLI read the SPLIT contract's node half, not a flat github key. + expect(resolved).toEqual(['app']) + expect(code).toBe(1) + expect(JSON.parse(output.text())).toMatchObject({ + ok: false, + results: [{ + status: 'blocked', + reason: expect.stringContaining('GitHub identity "app"'), + }], + }) + // The refusal must not have consumed the exactly-once claim. + expect(notionClaims.claim).not.toHaveBeenCalled() + expect(durableClaims.size).toBe(0) + + // Split config is a shallow top-level merge. An explicitly present, + // empty node github block replaces the workspace block, so the schema + // default is `auto`; falling back to the workspace identity here would + // disagree with loadFactoryConfig. + await writeFile(configPath, JSON.stringify({ + workspaceConfig: { + repos: { org: 'AgentWorkforce', names: ['cloud'] }, + github: { identity: 'app' }, + }, + nodeConfig: { github: {} }, + })) + const permittedOutput = buffer() + const permittedCode = await runFleetCli( + ['intake', 'notion', manifestPath, '--config', configPath], + { fleet: new FakeFleetClient(), notionClaims, notionGithub, stdout: permittedOutput, stderr: buffer() }, + ) + + expect(resolved).toEqual(['app', 'auto']) + expect(permittedCode).toBe(0) + expect(JSON.parse(permittedOutput.text())).toMatchObject({ + ok: true, + results: [{ status: 'dispatched' }], + }) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + it('returns after exact-path intake while preserving the spawned worker infrastructure', async () => { const root = await mkdtemp(join(tmpdir(), 'factory-cli-notion-dispatch-')) try { diff --git a/src/cli/fleet.ts b/src/cli/fleet.ts index 1e5fd70e..3e4e65c8 100644 --- a/src/cli/fleet.ts +++ b/src/cli/fleet.ts @@ -7,6 +7,8 @@ import { ensureCloudSession, type CloudSession } from '@agent-relay/cloud' import { stringifyLogValue } from '../logging' import { resolveLocalFactoryConfig, type LocalClonePathOptions } from '../config/local-clone-paths' +import { loadFactoryConfig } from '../config/schema' +import type { GithubWriteIdentity } from '../github' import { initializeFactory } from './init' import { diagnoseDeployedFactory, renderDeployedDiagnosis } from './diagnose' import { @@ -104,6 +106,7 @@ import { } from '../version-info' import { GhCliIssuePublisher, + type GithubIssuePublisher, NotionApiFactoryTasksClient, RelayChannelNotionClaimStore, RelayChannelNotionContractPublisher, @@ -179,6 +182,12 @@ export interface FleetCliDeps { notionContracts?: NotionContractPublisher /** Hermetic workspace-global Notion claim store for tests and alternate runtimes. */ notionClaims?: NotionIntakeClaimStore + /** + * Hermetic GitHub issue publisher for intake tests and alternate runtimes. + * Receives the identity resolved from the selected contract, so a test can + * assert the CLI honours `github.identity` without reaching the gh binary. + */ + notionGithub?: (identity: GithubWriteIdentity) => GithubIssuePublisher /** Hermetic Factory Tasks reader for manifest-generation tests and alternate runtimes. */ notionFactoryTasks?: FactoryTasksNotionClient /** Hermetic verification-environment sweep for CLI tests and alternate runtimes. */ @@ -364,7 +373,14 @@ export async function runFleetCli(argv: string[], deps: FleetCliDeps = {}): Prom manifest, dispatch: !globals.dryRun, ...(!globals.dryRun ? { - github: new GhCliIssuePublisher(), + // Notion intake has no app-authored issue-create route, so under an + // explicit `github.identity: "app"` it refuses rather than writing + // as the operator. Hardcoding `'user'` here would make that gate + // unreachable from the only production caller. + github: await (async () => { + const identity = await resolveNotionIntakeIdentity(globals.config) + return deps.notionGithub?.(identity) ?? new GhCliIssuePublisher(identity) + })(), workspace, ...(notionClaims ? { claims: notionClaims } : {}), ...(notionContracts ? { contracts: notionContracts } : {}), @@ -1671,6 +1687,33 @@ function parseFactoryStartFlags(args: Array): { mode: 'live' return { mode } } +/** + * The GitHub write identity Notion intake must honour. + * + * Notion intake does not otherwise require a Factory contract on disk, so an + * absent one is not an error — it is the same as an unset `github` block, + * which the schema synthesises to `auto`. A file that exists but cannot be + * read or parsed IS an error: silently degrading it to `auto` would resolve a + * deliberate `app` selection into the permissive value and reintroduce the + * silent local-user write this gate exists to prevent. + * + * Use the canonical config loader for both contract shapes. In particular, + * split config is a shallow top-level merge: a present `nodeConfig.github` + * replaces `workspaceConfig.github` even when the node block is empty, after + * which the GitHub schema supplies its `auto` default. + */ +async function resolveNotionIntakeIdentity(path?: string): Promise { + const configPath = path ?? resolve(process.cwd(), 'factory.config.json') + let raw: string + try { + raw = await readFile(configPath, 'utf8') + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return 'auto' + throw error + } + return loadFactoryConfig(JSON.parse(raw)).factoryConfig.github.identity +} + async function loadConfig(path?: string, options: LocalClonePathOptions = {}): Promise { const configPath = path ?? resolve(process.cwd(), 'factory.config.json') const raw = JSON.parse(await readFile(configPath, 'utf8')) as unknown diff --git a/src/config/schema.ts b/src/config/schema.ts index 6e3370d9..6ff1e907 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -415,12 +415,22 @@ const previewSchema = z.object({ } }).optional() +/** + * The GitHub write identity, and the single place its values are declared. + * + * Exported because callers outside the loaded config must resolve the same + * value the schema would — notably the Notion intake CLI, which reads a + * contract that may not exist. Absent input becomes `auto`, which is the + * compatibility value, so this default must never be changed casually. + */ +export const githubIdentitySchema = z.enum(['app', 'user', 'auto']).default('auto') + const githubSchema = z.object({ // Controls the credential identity used for GitHub writes. Exact `app` // selects the connected App for both PR publication and issue lifecycle // writes. `auto` preserves compatibility: PRs prefer the App, while issue // lifecycle writes retain the operator's local `gh` authentication. - identity: z.enum(['app', 'user', 'auto']).default('auto'), + identity: githubIdentitySchema, }).default({}) const verificationSchema = z.object({ diff --git a/src/github/gh-identity.test.ts b/src/github/gh-identity.test.ts new file mode 100644 index 00000000..d69987c6 --- /dev/null +++ b/src/github/gh-identity.test.ts @@ -0,0 +1,189 @@ +import { describe, expect, it } from 'vitest' + +import { GhCliGithubMergeGate, type GhRunner } from './merge-gate' +import { localGhMutationAllowed } from './gh-identity' +import { GhCliIssuePublisher } from '../intake/notion' +import { FactoryConfigSchema } from '../config/schema' + +/** + * The defect (#221): Factory writes to GitHub through two identities. The + * lifecycle writeback honours `github.identity`, but the guarded merge and + * Notion intake shell out to `gh` unconditionally, so under an explicit + * `identity: "app"` they still attribute the write to whichever human is + * logged in locally. + * + * Each case below is a must-fire / must-not-fire pair: `app` must refuse + * WITHOUT spawning `gh`, and `auto`/`user` must behave exactly as they do + * today. A test that only asserted the refusal would pass against a change + * that broke every local run. + */ + +const mergeInput = { repo: 'AgentWorkforce/example', number: 7, expectedHeadSha: 'a'.repeat(40) } + +/** + * Every `gh` invocation in this file goes through a fake. Nothing here may + * reach the network: an earlier draft of this test used the real runner and + * created a junk issue and overwrote a merged PR's body on the live repo. + */ +const fakeGh = (): { gh: (args: string[], input?: string) => Promise; calls: string[][] } => { + const calls: string[][] = [] + return { + calls, + gh: async (args) => { + calls.push(args) + return 'https://github.com/AgentWorkforce/example/issues/1' + }, + } +} + +const recordingRunner = (): { runner: GhRunner; calls: string[][] } => { + const calls: string[][] = [] + return { + calls, + runner: async (args) => { + calls.push(args) + return { stdout: '', stderr: '' } + }, + } +} + +const configWith = (identity?: 'app' | 'user' | 'auto') => + FactoryConfigSchema.parse({ + repos: { org: 'AgentWorkforce', names: ['factory'] }, + ...(identity ? { github: { identity } } : {}), + }) + +describe('local gh mutations under github.identity', () => { + it('MUST NOT FIRE: auto and user still squash-merge through the local gh CLI', async () => { + for (const identity of ['auto', 'user'] as const) { + const { runner, calls } = recordingRunner() + const result = await new GhCliGithubMergeGate(runner, identity).merge(mergeInput) + + expect(result.merged, `${identity} must keep merging`).toBe(true) + expect(calls, `${identity} must still invoke gh`).toHaveLength(1) + expect(calls[0]?.slice(0, 2)).toEqual(['pr', 'merge']) + } + }) + + it('MUST NOT FIRE: a gate constructed without an identity keeps the historical behavior', async () => { + const { runner, calls } = recordingRunner() + const result = await new GhCliGithubMergeGate(runner).merge(mergeInput) + + expect(result.merged).toBe(true) + expect(calls).toHaveLength(1) + }) + + it('MUST FIRE: identity "app" refuses the guarded merge and never spawns gh', async () => { + const { runner, calls } = recordingRunner() + const result = await new GhCliGithubMergeGate(runner, 'app').merge(mergeInput) + + expect(result.merged).toBe(false) + // Refused before the process boundary, not after a merge already landed. + expect(calls).toEqual([]) + expect(result.reason).toContain('GitHub identity "app"') + // The refusal must name the missing server-side capability and the + // operator's recovery path, or it is an outage with no exit. + expect(result.reason).toContain('mergePullRequest') + expect(result.reason).toContain('"user" or "auto"') + }) + + it('MUST NOT FIRE: identity "app" leaves the merge-gate READ working', async () => { + // `check` reads `gh pr view`; a read carries no authorship, so gating it + // would break the gate without removing any attribution. + const gate = new GhCliGithubMergeGate(async () => ({ + stdout: JSON.stringify({ + mergeable: 'MERGEABLE', + mergeStateStatus: 'CLEAN', + headRefOid: mergeInput.expectedHeadSha, + reviewDecision: 'APPROVED', + statusCheckRollup: [{ conclusion: 'SUCCESS' }], + }), + }), 'app') + + await expect(gate.check(mergeInput)).resolves.toMatchObject({ verdict: 'READY', ready: true }) + }) + + // The FactoryLoop selector's own must-fire/must-not-fire pair lives in + // `src/orchestrator/factory.test.ts`. It belongs with the loop it selects + // for, and importing the 23k-line orchestrator module from a third test + // file measurably slowed the parallel CI workers enough to time out an + // unrelated 5s MCP test. + + it('MUST NOT FIRE: the selector leaves auto and an absent github key merging', async () => { + // `github.identity` is synthesised to `auto` when the key is absent, so + // "no github block" and "identity: auto" must both stay on the old path. + for (const config of [configWith('auto'), configWith()]) { + expect(config.github.identity).toBe('auto') + expect(localGhMutationAllowed(config.github.identity)).toBe(true) + } + }) + + it('MUST FIRE: identity "app" refuses Notion intake issue create and edit, without invoking gh', async () => { + const { gh, calls } = fakeGh() + const publisher = new GhCliIssuePublisher('app', gh) + + await expect(publisher.createIssue({ + repo: 'AgentWorkforce/example', + title: 'title', + body: 'body', + labels: [], + })).rejects.toThrow(/GitHub identity "app"[\s\S]*createIssue/u) + + await expect(publisher.updateIssue({ + repo: 'AgentWorkforce/example', + number: 7, + body: 'body', + })).rejects.toThrow(/GitHub identity "app"[\s\S]*updateIssue/u) + + // Refused before the process boundary — no write reached GitHub. + expect(calls).toEqual([]) + }) + + it('MUST NOT FIRE: Notion intake under "user" still creates and edits through gh', async () => { + const { gh, calls } = fakeGh() + const publisher = new GhCliIssuePublisher('user', gh) + + await expect(publisher.createIssue({ + repo: 'AgentWorkforce/example', + title: 'title', + body: 'body', + labels: ['factory'], + })).resolves.toMatchObject({ number: 1 }) + + await publisher.updateIssue({ repo: 'AgentWorkforce/example', number: 7, body: 'body' }) + + expect(calls.map((args) => args.slice(0, 2))).toEqual([ + ['issue', 'create'], + ['issue', 'edit'], + ]) + }) + + it('MUST FIRE: the Notion refusal is raised by assertWritable, before any claim', () => { + // The refusal must be reachable WITHOUT calling createIssue, because + // publishRepoTask reserves an exactly-once delivery claim first. A + // refusal that only lived inside createIssue would burn that claim and + // permanently block the operator's retry under a permitted identity. + const { gh, calls } = fakeGh() + + expect(() => new GhCliIssuePublisher('app', gh).assertWritable()) + .toThrow(/GitHub identity "app"/u) + expect(calls).toEqual([]) + }) + + it('MUST NOT FIRE: assertWritable permits user and auto', () => { + for (const identity of ['user', 'auto'] as const) { + const { gh } = fakeGh() + expect(() => new GhCliIssuePublisher(identity, gh).assertWritable()).not.toThrow() + } + }) + + it('MUST NOT FIRE: identity "app" leaves Notion intake READS working', async () => { + // Reads carry no authorship, so gating them would break intake without + // removing any attribution. + const { gh, calls } = fakeGh() + const publisher = new GhCliIssuePublisher('app', gh) + + await publisher.missingLabels('AgentWorkforce/example', ['factory']) + expect(calls.map((args) => args[0])).toEqual(['api']) + }) +}) diff --git a/src/github/gh-identity.ts b/src/github/gh-identity.ts new file mode 100644 index 00000000..3a41a7ea --- /dev/null +++ b/src/github/gh-identity.ts @@ -0,0 +1,57 @@ +/** + * One rule for every GitHub mutation Factory performs through the local `gh` + * CLI. + * + * Factory has two GitHub write identities. Lifecycle writes (PR publication, + * issue comments, status labels, issue closure) are selected by + * `github.identity` and, on exact `app`, are performed server-side by the + * connected workspace GitHub App. Everything else still shells out to `gh`, + * which authenticates as whatever local user happens to be logged in — a + * human account. That produced one product with two audit trails. + * + * The remaining `gh` mutations cannot simply be re-routed: the connected + * `GithubConnectionWrite` surface has no merge operation and no issue-create + * operation, and Factory must never receive or invoke a GitHub credential of + * its own (see `src/mount/github-api-issue-read.ts`). So the honest behavior + * under an explicit `app` identity is to refuse rather than to silently write + * as the operator — a documented limitation instead of an invisible one. + * + * `auto` and `user` keep today's local-`gh` behavior. Only exact `app` + * refuses, and the refusal names the recovery path so the gate does not take + * an honest caller hostage. + */ +export type GithubWriteIdentity = 'app' | 'user' | 'auto' + +/** + * Whether the configured identity permits mutating GitHub through local `gh`. + * + * Reads are always permitted: `gh pr view` leaks no authorship, so read + * provenance is not an identity concern (see `StandalonePullRequest.source`). + */ +export function localGhMutationAllowed(identity: GithubWriteIdentity): boolean { + return identity !== 'app' +} + +/** + * The refusal text for a local-`gh` mutation blocked by `github.identity: "app"`. + * + * @param operation what Factory was about to do, in caller-facing terms. + * @param capability the server-side operation Relayfile Cloud would need to + * expose on the connected App surface for this write to be + * performed as the app instead of refused. + */ +export function localGhMutationRefusal(operation: string, capability: string): string { + return `GitHub identity "app" refuses ${operation} through the local gh CLI, which would attribute the write to the operator's account instead of the workspace GitHub App. ` + + `Performing it as the app requires the connected write capability "${capability}", which the Relayfile GitHub connection does not expose. ` + + 'Set github.identity to "user" or "auto" to deliberately accept local-user attribution for this operation.' +} + +/** Throw the standard refusal when `identity` forbids a local-`gh` mutation. */ +export function assertLocalGhMutationAllowed( + identity: GithubWriteIdentity, + operation: string, + capability: string, +): void { + if (localGhMutationAllowed(identity)) return + throw new Error(localGhMutationRefusal(operation, capability)) +} diff --git a/src/github/index.ts b/src/github/index.ts index 072032ca..0fbdf869 100644 --- a/src/github/index.ts +++ b/src/github/index.ts @@ -4,6 +4,11 @@ export { defaultGhRunner, evaluateGithubMergeGate, } from './merge-gate' +export { + assertLocalGhMutationAllowed, + localGhMutationAllowed, + localGhMutationRefusal, +} from './gh-identity' export { closeProbePr, } from './probe-closer' @@ -37,3 +42,4 @@ export type { StandalonePullRequest, } from './standalone-babysitter' export type { RoutedPrCandidate, RoutedPrDiscoveryReport } from './routed-pr-babysitter' +export type { GithubWriteIdentity } from './gh-identity' diff --git a/src/github/merge-gate.ts b/src/github/merge-gate.ts index 6b11766c..4ca1fda3 100644 --- a/src/github/merge-gate.ts +++ b/src/github/merge-gate.ts @@ -1,6 +1,8 @@ import { execFile } from 'node:child_process' import { promisify } from 'node:util' +import { localGhMutationAllowed, localGhMutationRefusal, type GithubWriteIdentity } from './gh-identity' + const execFileAsync = promisify(execFile) export interface GhRunResult { @@ -49,9 +51,17 @@ export interface GithubMergeGate { export class GhCliGithubMergeGate implements GithubMergeGate { readonly #run: GhRunner - - constructor(run: GhRunner = defaultGhRunner) { + readonly #identity: GithubWriteIdentity + + /** + * @param identity the configured `github.identity`. `check` is a read and + * ignores it; `merge` mutates GitHub and refuses under exact `app` rather + * than squash-merging as the operator's own account. Defaults to `auto` + * so a directly-constructed gate keeps its historical behavior. + */ + constructor(run: GhRunner = defaultGhRunner, identity: GithubWriteIdentity = 'auto') { this.#run = run + this.#identity = identity } async check(input: GithubMergeGateInput): Promise { @@ -78,6 +88,20 @@ export class GhCliGithubMergeGate implements GithubMergeGate { } async merge(input: GithubMergeInput): Promise { + // Fail closed before spawning `gh`. A guarded merge run through the local + // CLI is recorded by GitHub as the operator merging, which is precisely + // the split audit trail `github.identity: "app"` exists to remove. There + // is no app-authored merge to fall through to, so refuse and say why. + if (!localGhMutationAllowed(this.#identity)) { + return { + merged: false, + reason: localGhMutationRefusal( + `the guarded squash merge of ${input.repo}#${input.number}`, + 'mergePullRequest', + ), + } + } + try { const result = await this.#run([ 'pr', @@ -173,8 +197,14 @@ export function evaluateGithubMergeGate( } export const defaultGhRunner: GhRunner = async (args) => { - // TODO(issue-52): retire this compatibility runner when merge-gate reads and - // guarded merges are fully represented by the mounted GitHub connection. + // Compatibility runner. Retire it when merge-gate reads and guarded merges + // are fully represented by the mounted GitHub connection: that needs a + // `mergePullRequest` capability on `GithubConnectionWrite`, fulfilled + // server-side by Relayfile Cloud so Factory still holds no GitHub + // credential. Until then `merge` refuses under `github.identity: "app"` + // rather than merging as the operator (see ./gh-identity). Tracked on + // AgentWorkforce/factory#221; the previous marker cited issue 52, which is + // closed as completed and no longer owns this work. const { stdout, stderr } = await execFileAsync('gh', args, { maxBuffer: 1024 * 1024 }) return { stdout, stderr } } diff --git a/src/github/standalone-babysitter.ts b/src/github/standalone-babysitter.ts index e24ea189..e777d47b 100644 --- a/src/github/standalone-babysitter.ts +++ b/src/github/standalone-babysitter.ts @@ -25,6 +25,23 @@ export interface StandalonePullRequest { crossRepository?: boolean maintainerCanModify?: boolean filesChanged?: string[] + /** + * Where this PR's metadata was read from — deliberately retained. + * + * `gh` here is READ provenance, not write provenance: the only `gh` call + * this module makes is `gh pr view --json` (see `readStandalonePullRequest`), + * and a read carries no authorship, so it cannot split Factory's audit + * trail the way a `gh`-authored comment or merge would. The member is kept + * because the mounted projection can be stale or incomplete, and callers + * need to know whether a field came from the mount, from live GitHub, or + * from both before they act on it. + * + * This module performs no GitHub writes. Factory's identity-bearing GitHub + * mutations live in `src/writeback/github.ts` (lifecycle, identity-selected) + * and `src/github/merge-gate.ts` (guarded merge, refuses under `app`); the + * babysitter's own review replies and pushes are performed by the dispatched + * agent under the agent's credential, not by this process. + */ source: 'mount' | 'gh' | 'mount+gh' } diff --git a/src/intake/notion.test.ts b/src/intake/notion.test.ts index 7c85f143..55951b12 100644 --- a/src/intake/notion.test.ts +++ b/src/intake/notion.test.ts @@ -184,6 +184,72 @@ describe('Notion spec intake', () => { expect(github.createIssue).toHaveBeenCalledTimes(1) }) + it('refuses an app-identity intake WITHOUT consuming the exactly-once delivery claim', async () => { + // A refusal raised from createIssue would land after claimNotionDelivery, + // burning the claim: the operator's retry under a permitted identity would + // then hit `durable Notion claim already exists` forever. The policy check + // must happen before anything durable is reserved. + const { root, manifest } = await fixtureManifest('private mounted body', { + bootstrap: bootstrap({ repo: 'AgentWorkforce/cloud', labels: [] }), + }) + roots.push(root) + + const refusing = fakeGithub({ visibility: 'private' }) + refusing.assertWritable = () => { + throw new Error('GitHub identity "app" refuses creating or editing Notion intake lifecycle issues') + } + + const blocked = await runNotionIntake({ manifest, dispatch: true, claims, github: refusing }) + + expect(blocked.ok).toBe(false) + expect(blocked.results[0]).toMatchObject({ + status: 'blocked', + reason: expect.stringContaining('GitHub identity "app"'), + }) + // Nothing durable and nothing remote was touched. + expect(vi.mocked(claims.claim)).not.toHaveBeenCalled() + expect(durableClaims.size).toBe(0) + expect(refusing.createIssue).not.toHaveBeenCalled() + // Reads carry no authorship and stay available: the refusal is raised at + // the mutation, not at the top of the task. + expect(refusing.repositoryVisibility).toHaveBeenCalled() + + // MUST NOT FIRE: the operator switches to a permitted identity and the + // retry succeeds, proving the aborted run left no wedge behind. + const permitted = fakeGithub({ visibility: 'private' }) + const retried = await runNotionIntake({ manifest, dispatch: true, claims, github: permitted }) + + expect(retried.ok).toBe(true) + expect(permitted.createIssue).toHaveBeenCalledTimes(1) + }) + + it('still reconciles an already-dispatched task under an app identity, because it writes nothing', async () => { + // A blanket refusal at the top of publishRepoTask would break read-only + // reconciliation for every app-configured host. Only mutations refuse. + const { root, manifest } = await fixtureManifest('private mounted body', { + bootstrap: bootstrap({ repo: 'AgentWorkforce/cloud', labels: [] }), + }) + roots.push(root) + + const permitted = fakeGithub({ visibility: 'private' }) + const first = await runNotionIntake({ manifest, dispatch: true, claims, github: permitted }) + expect(first.ok).toBe(true) + const created = await vi.mocked(permitted.createIssue).mock.results[0]!.value as { number: number; url: string } + + const refusing = fakeGithub({ visibility: 'private' }) + refusing.assertWritable = () => { throw new Error('GitHub identity "app" refuses') } + refusing.findBySource = vi.fn(async () => ({ + ...created, + body: vi.mocked(permitted.createIssue).mock.calls[0]![0].body, + })) + + const reconciled = await runNotionIntake({ manifest, dispatch: true, claims, github: refusing }) + + expect(reconciled.ok).toBe(true) + expect(reconciled.results[0]).toMatchObject({ status: 'already-dispatched' }) + expect(refusing.updateIssue).not.toHaveBeenCalled() + }) + it('preserves an explicit Factory title prefix without duplicating it', async () => { const { root, manifest } = await fixtureManifest('private mounted body', { bootstrap: { @@ -319,6 +385,61 @@ describe('Notion spec intake', () => { expect(reconciled.receipts[`notion:${pageId}:repo:agentworkforce/cloud`].delivery.messageIds).toEqual(['message-1']) }) + it('refuses portable issue migration before its claim or contract publication while preserving metadata reconciliation', async () => { + const { root, manifest } = await fixtureManifest('private mounted implementation detail', { + bootstrap: bootstrap({ repo: 'AgentWorkforce/cloud', labels: [] }), + }) + roots.push(root) + const github = fakeGithub({ visibility: 'private' }) + await runNotionIntake({ manifest, dispatch: true, claims, github }) + const originalBody = vi.mocked(github.createIssue).mock.calls[0]![0].body + vi.mocked(github.findBySource).mockResolvedValue({ + number: 42, + url: 'https://github.test/issues/42', + body: originalBody, + }) + manifest.workerMountTransport = { kind: 'relay-channel' } + const contracts: NotionContractPublisher = { + publish: vi.fn(async () => ({ + kind: 'relay-channel', + channel: 'factory-notion-e1cff7cf-aabbccddee', + messageIds: ['message-1'], + encoding: 'base64-chunks-v1', + })), + } + durableClaims.clear() + vi.mocked(claims.claim).mockClear() + github.assertWritable = () => { throw new Error('GitHub identity "app" refuses') } + + const blocked = await runNotionIntake({ manifest, dispatch: true, claims, github, contracts }) + + expect(blocked.results[0]).toMatchObject({ + status: 'blocked', + reason: expect.stringContaining('GitHub identity "app"'), + }) + expect(claims.claim).not.toHaveBeenCalled() + expect(durableClaims.size).toBe(0) + expect(contracts.publish).not.toHaveBeenCalled() + expect(github.updateIssue).not.toHaveBeenCalled() + + delete github.assertWritable + const migrated = await runNotionIntake({ manifest, dispatch: true, claims, github, contracts }) + expect(migrated.results[0]).toMatchObject({ status: 'already-dispatched' }) + const migratedBody = vi.mocked(github.updateIssue).mock.calls[0]![0].body + vi.mocked(github.findBySource).mockResolvedValue({ + number: 42, + url: 'https://github.test/issues/42', + body: migratedBody, + }) + github.assertWritable = () => { throw new Error('GitHub identity "app" refuses') } + vi.mocked(github.updateIssue).mockClear() + + const reconciled = await runNotionIntake({ manifest, dispatch: true, claims, github, contracts }) + + expect(reconciled.results[0]).toMatchObject({ status: 'already-dispatched' }) + expect(github.updateIssue).not.toHaveBeenCalled() + }) + it('refuses to overwrite a manually edited lifecycle issue during portable mount migration', async () => { const { root, manifest } = await fixtureManifest('private mounted implementation detail', { bootstrap: bootstrap({ repo: 'AgentWorkforce/cloud', labels: [] }), diff --git a/src/intake/notion.ts b/src/intake/notion.ts index 0c3d38ef..d18a3898 100644 --- a/src/intake/notion.ts +++ b/src/intake/notion.ts @@ -7,6 +7,7 @@ import lockfile from 'proper-lockfile' import { z } from 'zod' import { dispatchNotionPageIdentity } from '../dispatch/work-unit-identity' +import { assertLocalGhMutationAllowed, type GithubWriteIdentity } from '../github/gh-identity' const INTAKE_LOCK_STALE_MS = 60_000 @@ -91,7 +92,19 @@ export interface ExistingGithubIssue { body: string } +/** Invokes the `gh` CLI with shell-free arguments and optional stdin. */ +export type GhCommandRunner = (args: string[], input?: string) => Promise + export interface GithubIssuePublisher { + /** + * Refuse now if this publisher may not perform GitHub mutations at all. + * + * Called before any durable claim is reserved. An identity refusal raised + * from `createIssue` would arrive after `claimNotionDelivery` has already + * consumed the exactly-once claim, so the operator's retry under a + * permitted identity would then be blocked forever by its own aborted run. + */ + assertWritable?(): void repositoryVisibility(repo: string): Promise<'public' | 'private' | 'internal'> missingLabels(repo: string, labels: readonly string[]): Promise findBySource(repo: string, sourceKey: string): Promise @@ -378,8 +391,39 @@ export function parseChiefSpecHeader(content: string): { /** GitHub CLI publisher with shell-free arguments, bounded output, and source-marker reconciliation. */ export class GhCliIssuePublisher implements GithubIssuePublisher { + readonly #identity: GithubWriteIdentity + readonly #gh: GhCommandRunner + + /** + * @param identity the GitHub write identity this publisher may use. Notion + * intake is a separate surface from the Factory lifecycle writeback and + * still creates and edits issues through the local `gh` CLI, so its + * issues are authored by the operator. That is a documented exception + * (see README), not a silent fallback: the caller must state the identity + * it is choosing, and exact `app` refuses rather than mislabelling the + * write, because the connected App surface exposes no issue-create + * operation to route it through. + * @param gh the `gh` invoker. Injectable because every method here mutates + * or reads real GitHub: without a seam the only way to exercise this + * class is against the live API, which during development of #221 + * created a junk issue and overwrote a merged PR's body. Tests must pass + * a fake; production takes the default. + */ + constructor(identity: GithubWriteIdentity, gh: GhCommandRunner = runGh) { + this.#identity = identity + this.#gh = gh + } + + assertWritable(): void { + assertLocalGhMutationAllowed( + this.#identity, + 'creating or editing Notion intake lifecycle issues', + 'createIssue/updateIssue', + ) + } + async repositoryVisibility(repo: string): Promise<'public' | 'private' | 'internal'> { - const output = (await runGh(['repo', 'view', repo, '--json', 'visibility', '--jq', '.visibility'])).trim().toLowerCase() + const output = (await this.#gh(['repo', 'view', repo, '--json', 'visibility', '--jq', '.visibility'])).trim().toLowerCase() if (output !== 'public' && output !== 'private' && output !== 'internal') { throw new Error(`GitHub returned unknown visibility for ${repo}: ${output || '(empty)'}`) } @@ -387,13 +431,13 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { } async missingLabels(repo: string, labels: readonly string[]): Promise { - const output = await runGh(['api', '--paginate', `repos/${repo}/labels?per_page=100`, '--jq', '.[].name']) + const output = await this.#gh(['api', '--paginate', `repos/${repo}/labels?per_page=100`, '--jq', '.[].name']) const available = new Set(output.split('\n').map((label) => label.trim()).filter(Boolean)) return labels.filter((label) => !available.has(label)) } async findBySource(repo: string, sourceKey: string): Promise { - const output = await runGh([ + const output = await this.#gh([ 'issue', 'list', '--repo', repo, '--state', 'all', '--limit', '100', '--search', `"factory-source:${sourceKey}" in:body`, '--json', 'number,url,body', @@ -404,16 +448,22 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { } async createIssue(input: { repo: string; title: string; body: string; labels: readonly string[] }): Promise<{ number: number; url: string }> { + assertLocalGhMutationAllowed(this.#identity, `creating a GitHub issue in ${input.repo}`, 'createIssue') const args = ['issue', 'create', '--repo', input.repo, '--title', input.title, '--body-file', '-'] for (const label of input.labels) args.push('--label', label) - const url = (await runGh(args, input.body)).trim() + const url = (await this.#gh(args, input.body)).trim() const number = Number(/\/issues\/(\d+)\/?$/u.exec(url)?.[1]) if (!Number.isInteger(number) || number <= 0) throw new Error(`GitHub issue create returned an unexpected URL: ${url}`) return { number, url } } async updateIssue(input: { repo: string; number: number; body: string }): Promise { - await runGh(['issue', 'edit', String(input.number), '--repo', input.repo, '--body-file', '-'], input.body) + assertLocalGhMutationAllowed( + this.#identity, + `editing the body of GitHub issue ${input.repo}#${input.number}`, + 'updateIssue', + ) + await this.#gh(['issue', 'edit', String(input.number), '--repo', input.repo, '--body-file', '-'], input.body) } } @@ -453,24 +503,22 @@ async function publishRepoTask( if (currentDigest !== task.digest) { return { ...base, status: 'blocked', issue: existing, reason: 'mounted spec changed after the lifecycle issue was created' } } - await ensureNotionWorkUnitClaim(task, input) let claim = await observeNotionDeliveryClaim(task, input) - if (!claim) { - if (!receipt) { - return { - ...base, - status: 'blocked', - issue: existing, - reason: 'lifecycle issue marker has neither a durable shared claim nor a local migration receipt', - } + if (!claim && !receipt) { + return { + ...base, + status: 'blocked', + issue: existing, + reason: 'lifecycle issue marker has neither a durable shared claim nor a local migration receipt', } - claim = (await claimNotionDelivery(task, input)).claim } const bodyDelivery = contractDeliveryFromBody(existing.body) if (input.manifest.workerMountTransport.kind === 'local') { if (receipt?.delivery || bodyDelivery) { return { ...base, status: 'blocked', issue: existing, reason: 'portable Notion delivery cannot be downgraded to a local worker mount' } } + await ensureNotionWorkUnitClaim(task, input) + claim ??= (await claimNotionDelivery(task, input)).claim state.receipts[task.sourceKey] = receipt ?? { kind: 'github', digest: task.digest, @@ -496,6 +544,18 @@ async function publishRepoTask( if (receipt?.delivery && bodyDelivery && !sameContractDelivery(receipt.delivery, bodyDelivery)) { return { ...base, status: 'blocked', issue: existing, reason: 'lifecycle issue portable delivery does not match its local receipt cache' } } + if (!bodyDelivery) { + // A portable migration must edit the issue. Refuse before either claim + // can be reserved and before the Relay contract publisher emits any + // messages. Existing body metadata takes the read-only path below. + try { + input.github.assertWritable?.() + } catch (error) { + return { ...base, status: 'blocked', issue: existing, reason: error instanceof Error ? error.message : String(error) } + } + } + await ensureNotionWorkUnitClaim(task, input) + claim ??= (await claimNotionDelivery(task, input)).claim const delivery = await prepareContractDelivery(task, input, receipt?.delivery ?? bodyDelivery) if (delivery && !bodyDelivery) { await assertMountedTaskUnchanged(task) @@ -527,6 +587,16 @@ async function publishRepoTask( if (missing.length > 0) { return { ...base, status: 'blocked', reason: `missing required GitHub labels: ${missing.join(', ')}` } } + // A create is now certain, so refuse here if this publisher may not write. + // Deliberately after the read-only checks above -- reconciliation and + // already-dispatched tasks need no mutation and must keep working under an + // app identity -- and deliberately before the first durable claim, so a + // policy refusal never consumes the exactly-once claim. + try { + input.github.assertWritable?.() + } catch (error) { + return { ...base, status: 'blocked', reason: error instanceof Error ? error.message : String(error) } + } await ensureNotionWorkUnitClaim(task, input) const delivery = await prepareContractDelivery(task, input) const claim = await claimNotionDelivery(task, input) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 4ce3cb42..7d212917 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -29,7 +29,8 @@ import { type TriageEngine, type WorkflowRunnerInput, } from '../index' -import { LatePlacementReleasedError, changeEventPath } from './factory' +import { LatePlacementReleasedError, changeEventPath, defaultMergeGate } from './factory' +import type { GhRunner } from '../github' import { RelaySpawnAckTimeoutError } from '../fleet/relay-fleet-client' import { RelayfileOperationTimeoutError } from '../mount/relayfile-operation-timeout' import type { AgentWorktree, AgentWorktreeCleanupInspection, AgentWorktreeManager, AgentWorktreeRepository, ChangeEvent, EventPage, GithubConnectionRead, GithubConnectionWrite, GithubIssueStatus, GithubIssueCloseWriteResult, GithubPublishPullRequestInput, GithubStatusClaimReceipt, GithubStatusWriteResult, GithubWriteback, LinearWriteback, PreviewReference, PreviewStartInput, ProviderSyncStatus, RosterEntry, SlackWriteback, SpawnInput, SpawnResult } from '../ports' @@ -32254,3 +32255,50 @@ describe('probe PR resolution from the pull index', () => { expect(walkedReads).toHaveLength(61) }) }) + +describe('merge gate identity selection', () => { + // The guard in `src/github/gh-identity.ts` is only worth anything if the + // FactoryLoop actually hands it the configured identity. Without this pair + // it would be a gate nobody invokes: `GhCliGithubMergeGate` refusing + // correctly while the loop kept constructing it with the default. + const mergeInput = { repo: 'AgentWorkforce/example', number: 7, expectedHeadSha: 'a'.repeat(40) } + // Every gh invocation is faked. Nothing here may reach the real binary: the + // operation on the other side is an irreversible squash merge. + const fakeGh = (): { run: GhRunner; calls: string[][] } => { + const calls: string[][] = [] + return { calls, run: async (args) => { calls.push(args); return { stdout: '', stderr: '' } } } + } + const configFor = (identity?: 'app' | 'user' | 'auto') => + FactoryConfigSchema.parse({ + repos: { org: 'AgentWorkforce', names: ['factory'] }, + ...(identity ? { github: { identity } } : {}), + }) + + it('MUST FIRE: identity "app" yields a merge gate that refuses without invoking gh', async () => { + const { run, calls } = fakeGh() + const result = await defaultMergeGate(configFor('app'), run).merge(mergeInput) + + expect(result.merged).toBe(false) + expect(calls).toEqual([]) + expect(result.reason).toContain('GitHub identity "app"') + expect(result.reason).toContain('mergePullRequest') + }) + + it('MUST NOT FIRE: identity "user" yields a gate that still performs the merge', async () => { + const { run, calls } = fakeGh() + const result = await defaultMergeGate(configFor('user'), run).merge(mergeInput) + + expect(result.merged).toBe(true) + expect(calls[0]?.slice(0, 2)).toEqual(['pr', 'merge']) + }) + + it('MUST NOT FIRE: an absent github key is synthesised to auto and still merges', async () => { + const config = configFor() + expect(config.github.identity).toBe('auto') + + const { run, calls } = fakeGh() + const result = await defaultMergeGate(config, run).merge(mergeInput) + expect(result.merged).toBe(true) + expect(calls).toHaveLength(1) + }) +}) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index afc7a357..17f943fa 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -1222,7 +1222,7 @@ export class FactoryLoop implements Factory { this.#githubWriteback = ports.githubWriteback ?? defaultGithubWriteback(config, ports.mount) this.#slack = config.slack ? MountSlackWriteback(ports.mount, config.slack) : ports.slack this.#github = ports.github ?? MountGithubRead(ports.mount) - this.#mergeGate = ports.mergeGate ?? new GithubMergeGate() + this.#mergeGate = ports.mergeGate ?? defaultMergeGate(config) this.#verificationGate = ports.verificationGate ?? (config.verification.enabled ? new VerificationPipeline({ descriptorPath: config.verification.descriptorPath, @@ -19470,6 +19470,21 @@ export class FactoryLoop implements Factory { } } +/** + * The guarded merge is a GitHub mutation, so it answers to the same identity + * policy as the lifecycle writeback. Under exact `github.identity: "app"` the + * gate refuses the merge instead of squash-merging as the operator's local + * `gh` user; `auto` and `user` keep today's behavior. + * + * Exported so the selection itself is testable rather than buried in the + * constructor — the same shape as `defaultGithubWriteback` below. `run` is a + * test seam only: production passes nothing and gets `defaultGhRunner`. No + * test may reach the real `gh` binary here, because the operation it would + * perform is an irreversible merge. + */ +export const defaultMergeGate = (config: FactoryConfig, run?: GhRunner): GithubMergeGatePort => + new GithubMergeGate(run, config.github.identity) + const defaultGithubWriteback = (config: FactoryConfig, mount: MountClient): GithubWriteback => { if (config.github.identity !== 'app') { return new GhCliGithubWriteback()