From a5704243e7b803da7da01cafa66a5c684863661c Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 26 Aug 2026 23:24:55 +0200 Subject: [PATCH 1/2] fix(notion): require explicit local user for gh intake --- src/cli/fleet.test.ts | 76 ++++++++++++++++++++++++++++++---- src/github/gh-identity.test.ts | 33 ++++++++------- src/github/gh-identity.ts | 6 ++- src/intake/notion.ts | 38 +++++++++-------- 4 files changed, 112 insertions(+), 41 deletions(-) diff --git a/src/cli/fleet.test.ts b/src/cli/fleet.test.ts index 400681a4..39381e55 100644 --- a/src/cli/fleet.test.ts +++ b/src/cli/fleet.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import type { CloudSession } from '@agent-relay/cloud' import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' +import { chmod, mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' @@ -855,6 +855,68 @@ describe('fleet CLI runtime', () => { } }) + it('blocks default-auto Notion repository intake before invoking local gh or reserving a claim', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-cli-notion-no-gh-')) + 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 explicit local identity', + 'Summary: Default auto must not reach the CLI adapter.', + '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 ghLogPath = join(root, 'gh.log') + const ghPath = join(root, 'gh') + await writeFile(ghPath, [ + '#!/bin/sh', + 'printf \'%s\\n\' "$*" >> "$FACTORY_TEST_GH_LOG"', + 'if [ "$1 $2" = "issue list" ]; then printf \'[]\'; fi', + 'if [ "$1 $2" = "repo view" ]; then printf \'private\'; fi', + ].join('\n')) + await chmod(ghPath, 0o755) + vi.stubEnv('PATH', root) + vi.stubEnv('FACTORY_TEST_GH_LOG', ghLogPath) + const notionClaims = { + get: vi.fn(async () => undefined), + findBySourcePrefix: vi.fn(async () => []), + claim: vi.fn(async () => { throw new Error('claim must not be reserved') }), + dispose: vi.fn(async () => undefined), + } + const output = buffer() + + const code = await runFleetCli(['intake', 'notion', manifestPath], { + fleet: new FakeFleetClient(), + notionClaims, + stdout: output, + stderr: buffer(), + }) + + expect(code).toBe(1) + expect(JSON.parse(output.text())).toMatchObject({ + ok: false, + results: [{ + status: 'blocked', + reason: expect.stringMatching(/GitHub identity "auto".*Notion intake.*local gh.*identity.*"user"/iu), + }], + }) + await expect(readFile(ghLogPath, 'utf8')).rejects.toMatchObject({ code: 'ENOENT' }) + expect(notionClaims.claim).not.toHaveBeenCalled() + } finally { + vi.unstubAllEnvs() + await rm(root, { recursive: true, force: true }) + } + }) + 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 @@ -903,16 +965,16 @@ describe('fleet CLI runtime', () => { 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, { + return { + assertWritable: () => { + if (identity === 'app') throw new Error('GitHub identity "app" refuses injected publisher writes') + }, repositoryVisibility: async () => 'private' as const, missingLabels: async () => [], findBySource: async () => undefined, createIssue: async () => ({ number: 42, url: 'https://github.test/issues/42' }), - }) + updateIssue: async () => undefined, + } } const code = await runFleetCli( diff --git a/src/github/gh-identity.test.ts b/src/github/gh-identity.test.ts index 69295255..3dbc0d56 100644 --- a/src/github/gh-identity.test.ts +++ b/src/github/gh-identity.test.ts @@ -12,10 +12,11 @@ import { FactoryConfigSchema } from '../config/schema' * `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. + * Each case below is a must-fire / must-not-fire pair. Merge keeps its + * historical auto/user policy, while the Notion CLI adapter is a stricter + * explicit-user local-host capability because the production container has + * no `gh` binary. A test that only asserted refusal would pass against a + * change that broke every local run. */ const mergeInput = { repo: 'AgentWorkforce/example', number: 7, expectedHeadSha: 'a'.repeat(40) } @@ -172,20 +173,22 @@ describe('local gh mutations under github.identity', () => { 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('requires exact user identity before the Notion gh adapter can write', () => { + const user = fakeGh() + expect(() => new GhCliIssuePublisher('user', user.gh).assertWritable()).not.toThrow() + + const automatic = fakeGh() + expect(() => new GhCliIssuePublisher('auto', automatic.gh).assertWritable()) + .toThrow(/GitHub identity "auto".*Notion intake.*local gh.*identity.*"user"/iu) + expect(automatic.calls).toEqual([]) }) - 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. + it.each(['app', 'auto'] as const)('blocks Notion intake reads under %s before spawning gh', async (identity) => { const { gh, calls } = fakeGh() - const publisher = new GhCliIssuePublisher('app', gh) + const publisher = new GhCliIssuePublisher(identity, gh) - await publisher.missingLabels('AgentWorkforce/example', ['factory']) - expect(calls.map((args) => args[0])).toEqual(['api']) + await expect(publisher.missingLabels('AgentWorkforce/example', ['factory'])) + .rejects.toThrow(new RegExp(`GitHub identity "${identity}".*Notion intake.*local gh`, 'iu')) + expect(calls).toEqual([]) }) }) diff --git a/src/github/gh-identity.ts b/src/github/gh-identity.ts index 3a41a7ea..cd579ecb 100644 --- a/src/github/gh-identity.ts +++ b/src/github/gh-identity.ts @@ -25,8 +25,10 @@ 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`). + * This helper governs mutations only. Read provenance is not an authorship + * concern, although a capability-specific adapter may still reject reads when + * its runtime has no `gh` binary (for example Notion intake in the cloud + * container). */ export function localGhMutationAllowed(identity: GithubWriteIdentity): boolean { return identity !== 'app' diff --git a/src/intake/notion.ts b/src/intake/notion.ts index d18a3898..cfe4cada 100644 --- a/src/intake/notion.ts +++ b/src/intake/notion.ts @@ -7,7 +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' +import type { GithubWriteIdentity } from '../github/gh-identity' const INTAKE_LOCK_STALE_MS = 60_000 @@ -398,11 +398,11 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { * @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. + * issues are authored by the operator. That is an explicit local-host + * mode, not a production fallback: the caller must select exact `user`. + * `auto` and `app` refuse every operation before spawning `gh`, because + * the production container has no binary and the connected App surface + * exposes neither issue creation nor source-marker reconciliation. * @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 @@ -415,14 +415,11 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { } assertWritable(): void { - assertLocalGhMutationAllowed( - this.#identity, - 'creating or editing Notion intake lifecycle issues', - 'createIssue/updateIssue', - ) + this.#assertExplicitLocalUser('createIssue/updateIssue') } async repositoryVisibility(repo: string): Promise<'public' | 'private' | 'internal'> { + this.#assertExplicitLocalUser('repositoryVisibility') 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)'}`) @@ -431,12 +428,14 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { } async missingLabels(repo: string, labels: readonly string[]): Promise { + this.#assertExplicitLocalUser('missingLabels') 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 { + this.#assertExplicitLocalUser('findBySource') const output = await this.#gh([ 'issue', 'list', '--repo', repo, '--state', 'all', '--limit', '100', '--search', `"factory-source:${sourceKey}" in:body`, @@ -448,7 +447,7 @@ 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') + this.#assertExplicitLocalUser('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 this.#gh(args, input.body)).trim() @@ -458,13 +457,18 @@ export class GhCliIssuePublisher implements GithubIssuePublisher { } async updateIssue(input: { repo: string; number: number; body: string }): Promise { - assertLocalGhMutationAllowed( - this.#identity, - `editing the body of GitHub issue ${input.repo}#${input.number}`, - 'updateIssue', - ) + this.#assertExplicitLocalUser('updateIssue') await this.#gh(['issue', 'edit', String(input.number), '--repo', input.repo, '--body-file', '-'], input.body) } + + #assertExplicitLocalUser(operation: string): void { + if (this.#identity === 'user') return + throw new Error( + `GitHub identity "${this.#identity}" refuses Notion intake ${operation} through local gh. ` + + 'This adapter is local-only and requires explicit github.identity "user"; the production Factory container does not contain gh, ' + + 'and the connected GitHub App surface does not expose issue creation or source-marker reconciliation.', + ) + } } async function publishRepoTask( From 9cf59308266c76d6189faa30a5308a113794c02f Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 26 Aug 2026 23:24:58 +0200 Subject: [PATCH 2/2] docs(notion): describe explicit user CLI gate --- README.md | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 1644f25d..bd9ca1d3 100644 --- a/README.md +++ b/README.md @@ -800,33 +800,29 @@ 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 | +| Write | Built-in CLI identity gate | 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` | +| Guarded squash merge (`mergePolicy: "on-green-with-review"`) | `"app"` declines and logs; nothing is merged | `mergePullRequest` | +| Notion intake issue create | only exact `"user"`; `"auto"` and `"app"` block before any durable claim | `createIssue` | +| Notion intake issue edit | only exact `"user"`; `"auto"` and `"app"` block before any CLI access | `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. +The merge refusal names `"user"` or `"auto"` as its local-user recovery path. +The built-in Notion publisher is stricter: its local-host opt-in is exact +`github.identity: "user"`, because the production Factory container contains +no `gh` binary and the App surface has no issue-create or source-marker query. 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. +`workspaceConfig`/`nodeConfig` contract, where the node half wins. Only exact +`"user"` enables the built-in CLI publisher. An absent contract resolves to +`"auto"` and therefore blocks repository intake explicitly; a contract that +exists but cannot be parsed is an error rather than a silent downgrade. + +The gate covers the built-in publisher's visibility, label, reconciliation, +create, and edit operations. `"auto"` and `"app"` fail before spawning `gh` and +before reserving a durable claim. Project-only intake and injected custom +publishers do not use this adapter and remain available. The standalone babysitter reads complete PR metadata from the authenticated mounted projection and reports `source: 'mount'`; it does not fall back to