Skip to content

Commit c07d013

Browse files
rgarciargarcia
committed
Run updater agents in jobs that reference only model API keys
1 parent d3e16de commit c07d013

3 files changed

Lines changed: 358 additions & 111 deletions

File tree

‎.github/tests/update-coverage-security.test.mjs‎

Lines changed: 143 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,40 @@
11
import assert from 'node:assert/strict';
2-
import { spawnSync } from 'node:child_process';
3-
import { readFileSync } from 'node:fs';
2+
import { execFileSync, spawnSync } from 'node:child_process';
3+
import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
4+
import { tmpdir } from 'node:os';
5+
import { join } from 'node:path';
46
import test from 'node:test';
57

6-
const workflow = readFileSync(new URL('../workflows/update-mcp-coverage.yml', import.meta.url), 'utf8');
7-
const jobs = Object.fromEntries(
8-
workflow
9-
.slice(workflow.indexOf('\njobs:\n'))
8+
const load = (file) => {
9+
const text = readFileSync(new URL(`../workflows/${file}`, import.meta.url), 'utf8');
10+
const blocks = text
11+
.slice(text.indexOf('\njobs:\n'))
1012
.split(/^ (?=[\w-]+:\n)/m)
11-
.slice(1)
12-
.map((job) => [job.slice(0, job.indexOf(':')), job.split(/(?=^ - )/m).slice(1)]),
13-
);
14-
const steps = Object.values(jobs).flat();
15-
const step = (job, name) => {
16-
const value = jobs[job].find((entry) => entry.startsWith(` - name: ${name}\n`));
17-
assert.ok(value, `missing ${job} step: ${name}`);
18-
return value;
13+
.slice(1);
14+
const jobText = Object.fromEntries(blocks.map((job) => [job.slice(0, job.indexOf(':')), job]));
15+
const jobs = Object.fromEntries(
16+
Object.entries(jobText).map(([name, job]) => [name, job.split(/(?=^ - )/m).slice(1)]),
17+
);
18+
const step = (job, name) => {
19+
const value = jobs[job].find((entry) => entry.startsWith(` - name: ${name}\n`));
20+
assert.ok(value, `missing ${job} step: ${name}`);
21+
return value;
22+
};
23+
return { text, jobText, jobs, steps: Object.values(jobs).flat(), step };
1924
};
25+
26+
const updaters = [
27+
{
28+
file: 'update-mcp-coverage.yml',
29+
agentStep: 'Update SDK dependency and MCP coverage',
30+
context: 'mcp-coverage-context',
31+
},
32+
{ file: 'update-docs.yml', agentStep: 'Update docs', context: 'sdk-docs-context' },
33+
];
34+
35+
const mcp = load('update-mcp-coverage.yml');
36+
const { text: workflow, jobs, step } = mcp;
37+
2038
const disallowedPaths = (paths) =>
2139
spawnSync('bash', [new URL('../scripts/mcp-update-disallowed-paths.sh', import.meta.url).pathname], {
2240
input: paths.map((path) => `${path}\0`).join(''),
@@ -25,31 +43,118 @@ const disallowedPaths = (paths) =>
2543
.stdout.split('\0')
2644
.filter(Boolean);
2745

28-
test('actions are pinned and checkouts do not persist credentials', () => {
29-
for (const entry of steps.filter((entry) => entry.includes('uses: '))) {
30-
assert.match(entry, /uses: [\w/-]+@[a-f0-9]{40}(?:\s|$)/);
31-
if (entry.includes('uses: actions/checkout@')) {
32-
assert.match(entry, /persist-credentials: false/);
33-
assert.doesNotMatch(entry, /token:/);
46+
const runScript = (entry) => {
47+
const lines = entry.split('\n');
48+
const start = lines.findIndex((line) => line === ' run: |');
49+
return lines
50+
.slice(start + 1)
51+
.map((line) => line.slice(10))
52+
.join('\n');
53+
};
54+
55+
for (const { file, agentStep, context } of updaters) {
56+
const { jobText, jobs, steps, step } = load(file);
57+
58+
test(`${file}: actions are pinned and checkouts do not persist credentials`, () => {
59+
for (const entry of steps.filter((entry) => entry.includes('uses: '))) {
60+
assert.match(entry, /uses: [\w/-]+@[a-f0-9]{40}(?:\s|$)/);
61+
if (entry.includes('uses: actions/checkout@')) {
62+
assert.match(entry, /persist-credentials: false/);
63+
assert.doesNotMatch(entry, /token:/);
64+
}
3465
}
35-
}
36-
});
66+
});
3767

38-
test('API source access is read-only and revoked before generation', () => {
39-
const readToken = step('generate', 'Generate API read token');
40-
assert.match(readToken, /repositories: kernel\n/);
41-
assert.match(readToken, /permission-contents: read/);
42-
assert.doesNotMatch(readToken, /permission-[\w-]+: write/);
43-
const fetch = step('generate', 'Fetch API spec');
44-
assert.match(fetch, /GH_TOKEN: \$\{\{ steps\.api-read-token\.outputs\.token \}\}/);
45-
assert.match(fetch, /gh api --method DELETE \/installation\/token\s*$/);
46-
assert.match(fetch, /-f until="\$1"/);
47-
assert.doesNotMatch(fetch, /gh repo clone/);
48-
assert.ok(
49-
jobs.generate.indexOf(fetch) <
50-
jobs.generate.indexOf(step('generate', 'Update SDK dependency and MCP coverage')),
51-
);
52-
});
68+
test(`${file}: the job that runs Pi references only the model API keys`, () => {
69+
// A job's secrets stay in runner memory for the whole job, where an agent with
70+
// sudo can read them, so revoking a minted token before Pi starts is not enough.
71+
const agentJobs = Object.keys(jobs).filter((name) =>
72+
jobs[name].some((entry) => entry.includes('pi --print')),
73+
);
74+
assert.deepEqual(agentJobs, ['generate']);
75+
const secrets = [...jobText.generate.matchAll(/\bsecrets\b(?:\.(\w+))?/g)].map((match) => match[1]);
76+
assert.deepEqual([...new Set(secrets)].sort(), ['ANTHROPIC_API_KEY', 'OPENAI_API_KEY']);
77+
assert.doesNotMatch(jobText.generate, /GH_TOKEN|github\.token|create-github-app-token|outputs\.token/);
78+
assert.match(step('generate', agentStep), /pi --print --no-session --approve/);
79+
});
80+
81+
test(`${file}: GitHub reads happen in an earlier job that hands over only data`, () => {
82+
const readToken = step('context', 'Generate API read token');
83+
assert.match(readToken, /repositories: kernel\n/);
84+
assert.match(readToken, /permission-contents: read/);
85+
assert.doesNotMatch(readToken, /permission-[\w-]+: write/);
86+
const fetch = step('context', 'Fetch API spec');
87+
assert.match(fetch, /GH_TOKEN: \$\{\{ steps\.api-read-token\.outputs\.token \}\}/);
88+
assert.match(fetch, /gh api --method DELETE \/installation\/token\s*$/);
89+
assert.match(fetch, /-f until="\$1"/);
90+
assert.doesNotMatch(fetch, /gh repo clone/);
91+
assert.doesNotMatch(jobText.context, /pi --print|secrets\.(ANTHROPIC|OPENAI)_API_KEY/);
92+
assert.match(step('context', 'Upload update context'), new RegExp(`name: ${context}\\n`));
93+
assert.match(step('context', 'Upload update context'), /retention-days: 1/);
94+
assert.match(step('generate', 'Download update context'), new RegExp(`name: ${context}\\n`));
95+
assert.match(jobText.generate, /needs: \[sdk-version, context\]\n\s+if: needs\.context\.outputs\.update == 'true'/);
96+
for (const output of ['main_sha', 'update_sha', 'rebased', 'agent_base_tree']) {
97+
assert.match(jobText.generate, new RegExp(`${output}: \\$\\{\\{ needs\\.context\\.outputs\\.${output} \\}\\}`));
98+
}
99+
const rebuild = step('generate', 'Rebuild update branch');
100+
assert.ok(jobs.generate.indexOf(rebuild) < jobs.generate.indexOf(step('generate', agentStep)));
101+
assert.match(step('generate', 'Snapshot publishable changes'), /AGENT_BASE: \$\{\{ steps\.prepare\.outputs\.agent_base \}\}/);
102+
});
103+
104+
test(`${file}: generate rebuilds exactly the branch the context job prepared`, () => {
105+
const script = runScript(step('generate', 'Rebuild update branch'));
106+
const dir = mkdtempSync(join(tmpdir(), 'rebuild-'));
107+
try {
108+
const git = (...args) => execFileSync('git', args, { cwd: dir, encoding: 'utf8' }).trim();
109+
const commit = (file, content, message) => {
110+
writeFileSync(join(dir, file), content);
111+
git('add', file);
112+
git('-c', 'user.name=t', '-c', 'user.email=t@example.com', 'commit', '-qm', message);
113+
return git('rev-parse', 'HEAD');
114+
};
115+
git('init', '-q', '-b', 'main');
116+
const base = commit('a.mdx', 'a\n', 'base');
117+
git('switch', '-qc', 'update');
118+
const update = commit('b.mdx', 'b\n', 'bot update');
119+
git('switch', '-q', 'main');
120+
const main = commit('c.mdx', 'c\n', 'main moved');
121+
git('switch', '-q', '--detach', base);
122+
// What the context job's prepare step leaves behind: the update branch rebased onto main.
123+
git('switch', '-qc', 'expected', update);
124+
git('-c', 'user.name=t', '-c', 'user.email=t@example.com', 'rebase', '-q', main);
125+
const rebasedTree = git('rev-parse', 'HEAD^{tree}');
126+
127+
const run = (env) => {
128+
git('switch', '-q', '--detach', base);
129+
spawnSync('git', ['branch', '-q', '-D', 'rebuilt'], { cwd: dir });
130+
const output = join(dir, '.git', 'output');
131+
writeFileSync(output, '');
132+
const result = spawnSync('bash', ['-e', '-c', script], {
133+
cwd: dir,
134+
encoding: 'utf8',
135+
env: { ...process.env, UPDATE_BRANCH: 'rebuilt', GITHUB_OUTPUT: output, ...env },
136+
});
137+
return { ...result, output: readFileSync(output, 'utf8') };
138+
};
139+
140+
const rebased = run({ REBASED: 'true', UPDATE_SHA: update, MAIN_SHA: main, AGENT_BASE_TREE: rebasedTree });
141+
assert.equal(rebased.status, 0, rebased.stderr);
142+
assert.equal(git('rev-parse', 'HEAD^{tree}'), rebasedTree);
143+
assert.equal(rebased.output, `agent_base=${git('rev-parse', 'HEAD')}\n`);
144+
145+
const fromMain = run({ REBASED: '', UPDATE_SHA: update, MAIN_SHA: main, AGENT_BASE_TREE: git('rev-parse', `${main}^{tree}`) });
146+
assert.equal(fromMain.status, 0, fromMain.stderr);
147+
assert.equal(git('rev-parse', 'HEAD'), main);
148+
149+
const moved = run({ REBASED: 'true', UPDATE_SHA: update, MAIN_SHA: base, AGENT_BASE_TREE: rebasedTree });
150+
assert.notEqual(moved.status, 0);
151+
assert.match(moved.stdout, /differs from the one the context job prepared/);
152+
assert.equal(moved.output, '');
153+
} finally {
154+
rmSync(dir, { recursive: true, force: true });
155+
}
156+
});
157+
}
53158

54159
test('changes are restricted to the allowlist before validation', () => {
55160
const snapshot = step('generate', 'Snapshot publishable changes');
@@ -61,9 +166,8 @@ test('changes are restricted to the allowlist before validation', () => {
61166
assert.ok(jobs.generate.indexOf(snapshot) < jobs.generate.indexOf(verify));
62167
assert.match(snapshot, /mcp-update-disallowed-paths\.sh/);
63168
assert.match(snapshot, /git reset -q --hard\n\s+git clean -fdq/);
64-
assert.match(snapshot, /AGENT_BASE: \$\{\{ steps\.prepare\.outputs\.agent_base \}\}/);
65169
assert.match(snapshot, /git reset -q --soft "\$AGENT_BASE"\n\s+git add -A/);
66-
const prepare = step('generate', 'Prepare update branch and SDK diff');
170+
const prepare = step('context', 'Prepare update branch and SDK diff');
67171
assert.match(prepare, /main_sha=\$\(git rev-parse origin\/main\)/);
68172
assert.match(prepare, /git rebase origin\/main; then[\s\S]*else\n\s+echo "rebased=true"/);
69173
assert.match(prepare, /agent_base=\$\(git rev-parse HEAD\)[\s\S]*agent_base_tree=/);

0 commit comments

Comments
 (0)