Skip to content
5 changes: 2 additions & 3 deletions ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,11 @@ host_broker:
- per-action total timeout, `SIGTERM` then `SIGKILL`, kill the process group;
- no automatic retry. A timed-out write reports that it may have completed. (Current
executor sends SIGKILL only; its timeout starts after preflight/token mint.)
- [ ] Token lifecycle:
- [x] Token lifecycle:
- mint per action with only that action's permissions;
- never cache;
- `finally`-revoke on success, failure, and timeout;
- if revoke fails, log host-side and report that the token expires on its own. (The host log is
now wired; the caller is not yet told.)
- if revoke fails, log host-side and report that the token expires on its own.
- [ ] Single-source action catalog (table below): one declarative table drives the permission
preflight, token mint, command table and docs; configured actions extend it under the Phase 1
rules. A test rejects drift between catalog, preflight, mint and docs, and rejects any built-in
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export interface ActionRunResult {
readonly error?: string;
/** SHA-256 prefix of the action token, for the audit record only. */
readonly tokenFingerprint?: string;
/** Set when the token could not be revoked and so expires on its own. */
readonly warning?: string;
}

/** One credential, as handed to git by the helper. */
Expand Down Expand Up @@ -279,6 +281,8 @@ export interface WireResponse {
readonly stderr?: string;
readonly exitCode?: number | null;
readonly timedOut?: boolean;
/** A non-fatal action warning, for example a failed token revoke. */
readonly warning?: string;
}

/** Where audit records go. */
Expand Down
2 changes: 2 additions & 0 deletions packages/client/src/commands/gh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,12 @@ export async function runGh(argv: readonly string[]): Promise<number> {
fail((error as Error).message);
}
if (response.ok !== true) {
if (response.warning) process.stderr.write(`warning: ${response.warning}\n`);
fail(`refused [${response.code ?? 'unknown'}]: ${response.reason ?? 'no reason'}`);
}
if (response.stdout) process.stdout.write(response.stdout);
if (response.stderr) process.stderr.write(response.stderr);
if (response.warning) process.stderr.write(`warning: ${response.warning}\n`);
if (response.truncated === true) {
process.stderr.write('warning: the action output was truncated at the broker\x27s limit\n');
}
Expand Down
4 changes: 3 additions & 1 deletion packages/host/src/broker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,8 @@ export function createRequestHandler(
values: actionValues,
});
if (result.error !== undefined) {
return denyAction(CODES.PROVIDER_ERROR, result.error, 'the host could not start gh');
const refusal = denyAction(CODES.PROVIDER_ERROR, result.error, 'the host could not start gh');
return result.warning === undefined ? refusal : { ...refusal, warning: result.warning };
}
audit.record({
event: 'action',
Expand All @@ -319,6 +320,7 @@ export function createRequestHandler(
exitCode: result.code,
truncated: result.truncated,
timedOut: result.timedOut,
...(result.warning === undefined ? {} : { warning: result.warning }),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
};
} catch (error) {
const reason = String((error as Error).message ?? error).slice(0, 300);
Expand Down
15 changes: 12 additions & 3 deletions packages/host/src/providers/github-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
*/
import { userTokenPath } from '../config.ts';
import { createSign } from 'node:crypto';
import { getGhAction, ProviderConfigError, tokenFingerprint } from 'gcb-cli';
import { getGhAction, ProviderConfigError, tokenFingerprint, type ActionRunResult } from 'gcb-cli';
import { runGhAction } from '../gh.ts';
import { existsSync, readFileSync, writeFileSync } from 'node:fs';

Expand Down Expand Up @@ -190,6 +190,9 @@ export function createGithubAppProvider(options: GithubAppProviderOptions): Prov
* a token minted for git should not also be able to open a pull request.
*/
const PULL_REQUEST_PERMISSIONS: Record<string, string> = { pull_requests: 'write', contents: 'read' };
/** The result of an action that never reached the executor. */
const NO_RUN = { stdout: '', stderr: '', code: null, signal: null, truncated: false, timedOut: false };
const REVOKE_WARNING = 'the action token could not be revoked; it expires on its own';

/** owner login (lowercased) -> installation */
const installations = new Map<string, CachedInstallation>();
Expand Down Expand Up @@ -578,8 +581,10 @@ function pullRequestNumber(value: unknown): number | undefined {
if (typeof token !== 'string' || !token) {
throw new ProviderConfigError('GitHub returned no installation token for the action');
}
let result: ActionRunResult;
let warning: string | undefined;
try {
const result = await runGhAction({
result = await runGhAction({
action: request.action,
values: request.values,
repo: request.full,
Expand All @@ -588,7 +593,8 @@ function pullRequestNumber(value: unknown): number | undefined {
ghPath,
env: process.env,
});
return { ...result, tokenFingerprint: tokenFingerprint(token) };
} catch (error) {
result = { ...NO_RUN, error: error instanceof Error ? error.message : String(error) };
} finally {
try {
const response = await fetchImpl(`${api}/installation/token`, {
Expand All @@ -608,11 +614,14 @@ function pullRequestNumber(value: unknown): number | undefined {
// The status is enough; a body that cannot be read is still not a successful revoke.
}
log(`could not revoke the action token: GitHub answered ${response.status}${detail ? ` ${detail}` : ''}`);
warning = REVOKE_WARNING;
}
} catch (error) {
log(`could not revoke the action token: ${String((error as Error).message ?? error)}`);
warning = REVOKE_WARNING;
}
}
return { ...result, tokenFingerprint: tokenFingerprint(token), ...(warning === undefined ? {} : { warning }) };
},

async getCredential(request): Promise<Credential> {
Expand Down
53 changes: 53 additions & 0 deletions test/unit/broker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,59 @@ describe('broker actions', () => {
assert.equal(record?.['token_fingerprint'], 'action-fingerprint');
});

it('passes a warning with a failed start', async () => {
const provider: Provider = {
name: 'static',
getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }),
runAction: async () => ({
stdout: '',
stderr: '',
code: null,
signal: null,
truncated: false,
timedOut: false,
error: 'spawn failed',
warning: 'the action token could not be revoked; it expires on its own',
}),
};
const harness = makeHarness(undefined, provider);
const response = await harness.handle({
op: 'action',
host: HOST,
path: 'acme/widget',
ghAction: 'pr:view',
values: { number: 7 },
});
assert.equal(response.ok, false);
assert.match(String(response.warning), /expires on its own/);
});

it('passes a provider warning to the caller', async () => {
const provider: Provider = {
name: 'static',
getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }),
runAction: async () => ({
stdout: 'ok',
stderr: '',
code: 0,
signal: null,
truncated: false,
timedOut: false,
warning: 'the action token could not be revoked; it expires on its own',
}),
};
const harness = makeHarness(undefined, provider);
const response = await harness.handle({
op: 'action',
host: HOST,
path: 'acme/widget',
ghAction: 'pr:view',
values: { number: 7 },
});
assert.equal(response.ok, true);
assert.match(String(response.warning), /expires on its own/);
});

it('refuses an undeclared flag before the provider', async () => {
let called = false;
const provider: Provider = {
Expand Down
119 changes: 90 additions & 29 deletions test/unit/gh-command.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,23 @@ import assert from 'node:assert/strict';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import { describe, it } from 'node:test';

import { runGh } from '../../packages/client/src/commands/gh.ts';
import { startBroker } from '../../packages/host/src/broker.ts';
import { tryRunAsync, type RunResult } from '../lib/run.ts';
import type { AuditSink, BrokerConfig, Provider, StaticHostConfig } from '../../packages/cli/src/types.ts';

/** Capture stdout for one command. */
async function capture(run: () => Promise<number>): Promise<{ code: number; stdout: string; stderr: string }> {
let stdout = '';
let stderr = '';
const originalOut = process.stdout.write;
const originalErr = process.stderr.write;
process.stdout.write = ((chunk: unknown) => {
stdout += String(chunk);
return true;
}) as typeof process.stdout.write;
process.stderr.write = ((chunk: unknown) => {
stderr += String(chunk);
return true;
}) as typeof process.stderr.write;
try {
return { code: await run(), stdout, stderr };
} finally {
process.stdout.write = originalOut;
process.stderr.write = originalErr;
}
/** Run the client from source, next to this file rather than from the cwd. */
const CLIENT_BIN = fileURLToPath(new URL('../../packages/client/bin/gcb.js', import.meta.url));

/** Run the installed client as a child, so its output never replaces the test reporter. */
function runGhCli(args: readonly string[]): Promise<RunResult> {
return tryRunAsync(
process.execPath,
['--conditions=development', CLIENT_BIN, 'gh', ...args],
{ env: process.env, timeoutMs: 5000 },
);
}

describe('gcb gh', () => {
Expand Down Expand Up @@ -57,10 +48,8 @@ describe('gcb gh', () => {
};
const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) });
try {
const result = await capture(() =>
runGh(['pr', 'create', '--head', 'feat/x', '--title', 'x', '--body', 'b', '--repo', 'acme/widget', '--socket', socketPath]),
);
assert.equal(result.code, 1);
const result = await runGhCli(['pr', 'create', '--head', 'feat/x', '--title', 'x', '--body', 'b', '--repo', 'acme/widget', '--socket', socketPath]);
assert.equal(result.status, 1);
assert.ok(result.stdout.endsWith('partial output\n'));
assert.match(result.stderr, /output was truncated/);
assert.match(result.stderr, /may have completed/);
Expand All @@ -70,6 +59,80 @@ describe('gcb gh', () => {
}
});

it('prints a warning on a refused action', async () => {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-refused-warning-'));
const socketPath = path.join(root, 'broker.sock');
const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] };
const config: BrokerConfig = {
socketPath,
socketMode: 0o600,
auditPath: null,
tokenCacheSkewSeconds: 300,
hosts: { 'github.com': block },
};
const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() };
const provider: Provider = {
name: 'static',
getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }),
runAction: async () => ({
stdout: '',
stderr: '',
code: null,
signal: null,
truncated: false,
timedOut: false,
error: 'spawn failed',
warning: 'the action token could not be revoked; it expires on its own',
}),
};
const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) });
try {
const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]);
assert.equal(result.status, 1);
assert.match(result.stderr, /expires on its own/);
assert.match(result.stderr, /refused/);
} finally {
await broker.close();
fs.rmSync(root, { recursive: true, force: true });
}
});

it('prints a provider warning to stderr', async () => {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-warning-'));
const socketPath = path.join(root, 'broker.sock');
const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] };
const config: BrokerConfig = {
socketPath,
socketMode: 0o600,
auditPath: null,
tokenCacheSkewSeconds: 300,
hosts: { 'github.com': block },
};
const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() };
const provider: Provider = {
name: 'static',
getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }),
runAction: async () => ({
stdout: 'viewed\n',
stderr: '',
code: 0,
signal: null,
truncated: false,
timedOut: false,
warning: 'the action token could not be revoked; it expires on its own',
}),
};
const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) });
try {
const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]);
assert.equal(result.status, 0);
assert.match(result.stderr, /expires on its own/);
} finally {
await broker.close();
fs.rmSync(root, { recursive: true, force: true });
}
});

it('sends a typed action to the broker and prints its stdout', async () => {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-command-'));
const socketPath = path.join(root, 'broker.sock');
Expand All @@ -93,10 +156,8 @@ describe('gcb gh', () => {
};
const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) });
try {
const result = await capture(() =>
runGh(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]),
);
assert.equal(result.code, 0);
const result = await runGhCli(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', socketPath]);
assert.equal(result.status, 0);
assert.ok(result.stdout.endsWith('the viewed PR\n'));
} finally {
await broker.close();
Expand Down
Loading
Loading