From 26ca636ccc8b7a92e5b808190fee541b995cf39d Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 21 Sep 2026 17:46:10 -0700 Subject: [PATCH 1/2] fix(flows): push the work without workflow edits when GitHub refuses them The run's GitHub token is minted with contents and pull_requests write only, so a push that edits .github/workflows/ is refused and the run lost its agent's finished work (cloud run 065fd98f, cloud-e2e-sandbox#40). Every push in the generated flow now goes through FLOW_PUSH_COMMAND. A push that succeeds is unchanged; on GitHub's workflow refusal only, the unpushed commits are rebuilt without their workflow edits (same messages and authors), pushed again, and the withheld patch goes into the pull-request body (or a comment for a revision push). Any other failure behaves as before. Every agent is also told to avoid workflow edits unless needed. Co-Authored-By: Claude Opus 5 --- web/lib/flow-workflows.ts | 106 +++++++++- web/lib/test/flow-local.test.ts | 14 +- web/lib/test/flow-onboarding.test.ts | 25 ++- web/lib/test/flow-push-guard.test.ts | 280 +++++++++++++++++++++++++++ 4 files changed, 406 insertions(+), 19 deletions(-) create mode 100644 web/lib/test/flow-push-guard.test.ts diff --git a/web/lib/flow-workflows.ts b/web/lib/flow-workflows.ts index aa9171b..65d6808 100644 --- a/web/lib/flow-workflows.ts +++ b/web/lib/flow-workflows.ts @@ -325,6 +325,99 @@ export const FLOW_DROP_WORKING_FILES_COMMAND = [ export const FLOW_OPEN_CHANGE_COMMAND = 'open_change() { if command -v relayflow-open-change >/dev/null 2>&1; then relayflow-open-change "$@"; else gh pr create "$@"; fi; }; open_change'; +/** + * Told to every agent. The run's token may not be allowed to push changes + * under `.github/workflows/`; FLOW_PUSH_COMMAND withholds them if so. + */ +export const WORKFLOW_FILES_HINT = 'Changes under .github/workflows/ may not be pushable by this run\'s token, so do not edit workflow files unless the task requires it; if you do, keep those edits in separate commits.'; + +/** + * GitHub's refusal of a push that edits `.github/workflows/` from a token + * without the `workflows` permission. Nothing else triggers the fallback in + * FLOW_PUSH_COMMAND. + */ +const WORKFLOW_PUSH_REFUSAL = 'refusing to allow a GitHub App to create or update workflow|without .workflows. permission'; + +/** Removes credentials from any URL git prints (`https://user:token@host`). */ +const SCRUB_URL_CREDENTIALS = "sed -e 's#://[^/@[:space:]]*@#://#g'"; + +/** + * Pushes the branch, and if GitHub refuses it only because it edits + * `.github/workflows/`, pushes the work without those edits instead of losing + * it. The caller prefixes `base=` and appends the `git push` arguments. + * + * The run's GitHub token is minted with `contents` and `pull_requests` write + * only, so any pushed commit that touches a workflow file is refused. + * AgentWorkforce/cloud-e2e-sandbox run 065fd98f lost an agent's finished work + * ($6.80, 3 commits) that way: it had edited ci.yml only to add a path filter, + * and the one push attempt failed with `refusing to allow a GitHub App to + * create or update workflow .github/workflows/ci.yml without workflows + * permission`. + * + * A push that succeeds is unchanged, so a run whose token has the permission + * pushes workflow edits as before. Any other failure is returned as it was. + * On that one refusal, every unpushed commit since the base is rebuilt with + * the same message, author and dates, with `.github/workflows/` held at its + * parent's state (a commit left empty is dropped). The edits are removed from + * each commit rather than reverted by a commit on top, because GitHub checks + * every pushed commit, not only the branch tip. The edits are saved to + * `.relayflow/workflow-changes.patch`, the original commits stay at + * `refs/relayflow/withheld-workflows`, and the patch (bounded, so the pull + * request body stays under GitHub's limit) is appended to + * `.relayflow/pr-body.md`; with `comment=yes`, for a pull request already + * open, it is posted as a comment instead. If the second push fails, HEAD is + * restored and the step fails with both errors. Git's output is printed with + * credentials removed from any URL. + */ +export const FLOW_PUSH_COMMAND = 'relayflow_push() { ' + [ + 'wf=.github/workflows', + 'err=$(mktemp "${TMPDIR:-/tmp}/relayflow-push.XXXXXX") || { git push "$@"; return; }', + 'git push "$@" 2>"$err"; status=$?', + `${SCRUB_URL_CREDENTIALS} "$err" >&2`, + 'if [ "$status" -eq 0 ]; then rm -f "$err"; return 0; fi', + `if ! grep -Eq ${shq(WORKFLOW_PUSH_REFUSAL)} "$err"; then rm -f "$err"; return "$status"; fi`, + 'mb=; if [ -n "${base:-}" ] && git rev-parse --verify --quiet "$base^{commit}" >/dev/null 2>&1; then mb=$(git merge-base "$base" HEAD 2>/dev/null); fi', + 'if [ -z "$mb" ]; then echo "relayflow push-guard: GitHub refused the workflow edits, and the base commit is unknown, so nothing was withheld." >&2; rm -f "$err"; return "$status"; fi', + 'orig=$(git rev-parse HEAD)', + 'if [ -z "$(git rev-list --full-history "$orig" "^$mb" --not --remotes -- "$wf")" ]; then echo "relayflow push-guard: no unpushed commit edits $wf, so there is nothing to withhold." >&2; rm -f "$err"; return "$status"; fi', + 'tmp=$(mktemp -d "${TMPDIR:-/tmp}/relayflow-withhold.XXXXXX") || { rm -f "$err"; return "$status"; }', + ': > "$tmp/map"; ok=yes', + 'relayflow_new() { n=$(grep "^$1 " "$tmp/map" | cut -d" " -f2); if [ -n "$n" ]; then echo "$n"; else echo "$1"; fi; }', + 'for c in $(git rev-list --reverse --topo-order "$orig" "^$mb" --not --remotes); do ' + + 'parents=$(git rev-list --parents -n 1 "$c" | cut -s -d" " -f2-); p1=; np=; for p in $parents; do q=$(relayflow_new "$p"); if [ -z "$p1" ]; then p1=$q; fi; np="$np -p $q"; done; ' + + 'if GIT_INDEX_FILE="$tmp/index" git read-tree "$c" && { GIT_INDEX_FILE="$tmp/index" git ls-files -z -- "$wf" | GIT_INDEX_FILE="$tmp/index" git update-index -z --force-remove --stdin; } && { [ -z "$p1" ] || git ls-tree -r --full-tree "$p1" -- "$wf" | GIT_INDEX_FILE="$tmp/index" git update-index --index-info; } && tree=$(GIT_INDEX_FILE="$tmp/index" git write-tree); then :; else ok=no; break; fi; ' + + 'case "$parents" in (*" "*) single=no ;; (*) single=yes ;; esac; ' + + 'if [ "$single" = yes ] && [ -n "$p1" ] && [ "$tree" = "$(git rev-parse "$p1^{tree}")" ]; then echo "$c $p1" >> "$tmp/map"; continue; fi; ' + + 'git cat-file commit "$c" | sed "1,/^\\$/d" > "$tmp/msg"; ' + + 'new=$(GIT_AUTHOR_NAME="$(git show -s --format=%an "$c")" GIT_AUTHOR_EMAIL="$(git show -s --format=%ae "$c")" GIT_AUTHOR_DATE="$(git show -s --date=raw --format=%ad "$c")" GIT_COMMITTER_NAME="$(git show -s --format=%cn "$c")" GIT_COMMITTER_EMAIL="$(git show -s --format=%ce "$c")" GIT_COMMITTER_DATE="$(git show -s --date=raw --format=%cd "$c")" git commit-tree "$tree" $np -F "$tmp/msg") || { ok=no; break; }; ' + + 'echo "$c $new" >> "$tmp/map"; done', + 'new=$(relayflow_new "$orig")', + 'if [ "$ok" != yes ] || [ "$new" = "$orig" ]; then echo "relayflow push-guard: could not withhold the workflow edits." >&2; rm -rf "$tmp" "$err"; return "$status"; fi', + 'git update-ref -m "relayflow: withhold workflow edits" HEAD "$new" "$orig"', + 'git push "$@" 2>"$tmp/push"; again=$?', + `${SCRUB_URL_CREDENTIALS} "$tmp/push" >&2`, + `if [ "$again" -ne 0 ]; then git update-ref -m "relayflow: restore after a failed push" HEAD "$orig" "$new"; echo "relayflow push-guard: the push failed again after withholding the workflow edits. The original error was:" >&2; ${SCRUB_URL_CREDENTIALS} "$err" >&2; rm -rf "$tmp" "$err"; return "$again"; fi`, + 'git update-ref refs/relayflow/withheld-workflows "$orig"', + 'git diff --name-only --no-renames "$new" "$orig" -- "$wf" | while IFS= read -r p; do if git cat-file -e "$new:$p" 2>/dev/null; then git checkout -q "$new" -- "$p"; else git rm -q -f --ignore-unmatch -- "$p" >/dev/null 2>&1; rm -f -- "$p"; fi; done', + 'mkdir -p .relayflow; patch=.relayflow/workflow-changes.patch; section=.relayflow/workflow-changes.md', + 'git diff --full-index "$mb" "$orig" -- "$wf" > "$patch"', + 'n=$(git diff --name-only "$mb" "$orig" -- "$wf" | wc -l | tr -d " ")', + 'if [ -s "$patch" ]; then ' + + 'used=0; if [ "${comment:-}" != yes ] && [ -f .relayflow/pr-body.md ]; then used=$(wc -c < .relayflow/pr-body.md | tr -d " "); fi; ' + + 'limit=${RELAYFLOW_WITHHELD_PATCH_LIMIT:-61440}; room=$((61000 - used)); if [ "$room" -lt "$limit" ]; then limit=$room; fi; size=$(wc -c < "$patch" | tr -d " "); ' + + `{ printf '\\n## Workflow changes not applied\\n\\n%s\\n\\n' "The GitHub App token this run pushes with lacks the \\\`workflows\\\` permission, so GitHub refused the commits that change \\\`.github/workflows/\\\`. The rest of the work is pushed; these edits were taken out of its commits. Apply them manually:"; ` + + `git diff --name-status "$mb" "$orig" -- "$wf" | awk -F '\\t' '{ printf "- %s \\140%s\\140\\n", substr($1, 1, 1), $NF }'; ` + + `if [ "$limit" -le 0 ]; then printf '\\n%s\\n' "_The patch ($size bytes) does not fit in the pull request body. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; ` + + `elif [ "$size" -le "$limit" ]; then printf '\\n\`\`\`\`diff\\n'; cat "$patch"; printf '\`\`\`\`\\n'; ` + + `else printf '\\n\`\`\`\`diff\\n'; head -c "$limit" "$patch" | sed '$d'; printf '\`\`\`\`\\n\\n%s\\n' "_Truncated to $limit of $size bytes. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; fi; } > "$section"; ` + + 'if [ "$size" -gt "$limit" ]; then cat "$patch" >&2; fi; ' + + 'if [ "${comment:-}" = yes ]; then if gh pr comment --body-file "$section" >/dev/null 2>&1; then echo "relayflow push-guard: posted the withheld workflow changes to the pull request." >&2; else echo "relayflow push-guard: could not comment on the pull request; $section holds the withheld workflow changes." >&2; fi; ' + + 'elif [ -f .relayflow/pr-body.md ]; then cat "$section" >> .relayflow/pr-body.md; fi; fi', + 'echo "relayflow push-guard: workflow edits withheld ($n files)"', + 'echo "relayflow push-guard: GitHub refused the edits to $wf, so the branch was pushed without them. The patch is $patch; the original commits are refs/relayflow/withheld-workflows." >&2', + 'rm -rf "$tmp" "$err"', +].join('; ') + '; }; relayflow_push'; + /** * Adds the deterministic provider reference after the generated check report. * The reference is supplied through a shell-quoted variable by the generated @@ -471,7 +564,8 @@ export function workflowCode(workflow: WorkflowId, agents: ReturnType; ...`, so a prefix cannot tell them apart. * `check` may be a function, for a sequence of check results. */ +// Every push goes through the workflow-file guard (run 065fd98f). +const PUSH = 'base=; ' + FLOW_PUSH_COMMAND + ' --set-upstream origin HEAD'; +const REVISION_PUSH = 'base=; comment=yes; ' + FLOW_PUSH_COMMAND; + function answer(command: string, { publish = 'publish', clean = 'yes', check = 'pass' as string | (() => string), baseline = 'pass' } = {}) { if (command === FLOW_CHECK_RUN_COMMAND) return typeof check === 'function' ? check() : check; if (command.endsWith(FLOW_BASE_CHECK_COMMAND)) return baseline; @@ -277,7 +281,7 @@ describe('local flow starter kit', () => { }, localRunInput()); } finally { console.error = original; } expect(commands.some(command => command.startsWith(FLOW_OPEN_CHANGE_COMMAND))).toBe(false); - expect(commands.some(command => command.startsWith('git push'))).toBe(false); + expect(commands.some(command => command.includes(FLOW_PUSH_COMMAND))).toBe(false); expect(finish).toBe('needs_human'); expect(messages.join('\n')).toContain('no commits'); }); @@ -316,7 +320,7 @@ describe('local flow starter kit', () => { }, localRunInput(selected)); const create = commands.find(command => command.startsWith(FLOW_OPEN_CHANGE_COMMAND)) ?? ''; expect(create).toContain('--draft'); - expect(commands).toContain('git push --set-upstream origin HEAD'); + expect(commands).toContain(PUSH); expect(finish).toBe('step_failed'); } }); @@ -340,9 +344,9 @@ describe('local flow starter kit', () => { expect(fixer).toBeGreaterThan(0); expect(calls[fixer + 1]).toBe(FLOW_CHECK_RUN_COMMAND); // Pushed either way: the revision is work, and work is never thrown away. - expect(calls.indexOf('git push')).toBeGreaterThan(fixer); + expect(calls.indexOf(REVISION_PUSH)).toBeGreaterThan(fixer); if (fail) { - expect(calls.indexOf(FLOW_CHECK_BLOCKED_COMMAND)).toBeGreaterThan(calls.indexOf('git push')); + expect(calls.indexOf(FLOW_CHECK_BLOCKED_COMMAND)).toBeGreaterThan(calls.indexOf(REVISION_PUSH)); expect(finish).toBe('step_failed'); } else { expect(calls).not.toContain(FLOW_CHECK_BLOCKED_COMMAND); diff --git a/web/lib/test/flow-onboarding.test.ts b/web/lib/test/flow-onboarding.test.ts index 6dcdfb7..f3669f3 100644 --- a/web/lib/test/flow-onboarding.test.ts +++ b/web/lib/test/flow-onboarding.test.ts @@ -3,6 +3,11 @@ import ts from 'typescript'; import { FLOW_BASE_CHECK_COMMAND, FLOW_CHECK_BLOCKED_COMMAND, FLOW_CHECK_RUN_COMMAND, FLOW_DROP_WORKING_FILES_COMMAND, FLOW_OPEN_CHANGE_COMMAND, FLOW_PREPARE_CHANGE_METADATA_COMMAND, FLOW_PUBLISH_CHECK_COMMAND, FLOW_REVIEW_BLOCKED_COMMAND, FLOW_VALIDATE_CHANGE_METADATA_COMMAND } from '../flow-workflows'; import { cloudBlockedReason, cloudConnectionsHref, DEFAULT_FACTORY, factorySource, isMarkdownOnly, MARKDOWN_ONLY_CLOUD_NOTE, readFactoryDraft, canContinue, primaryAgent, onboardingPath, accessibleOnboardingStep, type FactoryDraft } from '../flow-onboarding'; import { localInput } from '../flow-local'; +import { FLOW_PUSH_COMMAND } from '../flow-workflows'; + +// Every push goes through the workflow-file guard (run 065fd98f). +const PUSH = 'base=abc123; ' + FLOW_PUSH_COMMAND + ' --set-upstream origin HEAD'; +const REVISION_PUSH = 'base=abc123; comment=yes; ' + FLOW_PUSH_COMMAND; const matchingIssue = { source: 'github', title: ' Fix login ', body: 'Login fails', labels: ['ready', 'bug'], repository: 'acme/app', identifier: '#507', url: 'https://github.com/acme/app/issues/507' }; @@ -331,7 +336,7 @@ describe('software factory onboarding', () => { // pushed at the base commit is not worth leaving behind either. const { calls, finish, errors } = await runFactory([true, true], true, matchingIssue, completed, 'no-commits'); expect(calls.some(call => call.startsWith(FLOW_OPEN_CHANGE_COMMAND))).toBe(false); - expect(calls.some(call => call.startsWith('git push'))).toBe(false); + expect(calls.some(call => call.includes(FLOW_PUSH_COMMAND))).toBe(false); expect(finish).toBe('needs_human'); // The reason reaches the operator, so "nothing was built" is never silent. expect(errors.join('\n')).toContain('no commits'); @@ -342,7 +347,7 @@ describe('software factory onboarding', () => { // body is what is missing, and `gh pr create --body-file summary.md` would // fail on exactly that. const { calls, finish, errors } = await runFactory([true, true], true, matchingIssue, completed, 'no-summary'); - expect(calls).toContain('git push --set-upstream origin HEAD'); + expect(calls).toContain(PUSH); expect(calls.some(call => call.startsWith(FLOW_OPEN_CHANGE_COMMAND))).toBe(false); expect(finish).toBe('needs_human'); expect(errors.join('\n')).toContain('summary.md'); @@ -353,14 +358,14 @@ describe('software factory onboarding', () => { // anything else, the flow must not push a branch or open a pull request on // the strength of output it did not understand. const { calls, finish } = await runFactory([true, true], true, matchingIssue, completed, 'unexpected output'); - expect(calls.some(call => call.startsWith('git push'))).toBe(false); + expect(calls.some(call => call.includes(FLOW_PUSH_COMMAND))).toBe(false); expect(calls.some(call => call.startsWith(FLOW_OPEN_CHANGE_COMMAND))).toBe(false); expect(finish).toBe('needs_human'); }); it('tests and pushes the branch before opening its pull request', async () => { const { calls } = await runFactory([true]); - const push = calls.indexOf('git push --set-upstream origin HEAD'); + const push = calls.indexOf(PUSH); const create = calls.findIndex(call => call.startsWith(FLOW_OPEN_CHANGE_COMMAND)); expect(push).toBeGreaterThan(calls.indexOf(FLOW_CHECK_RUN_COMMAND)); expect(create).toBeGreaterThan(push); @@ -467,7 +472,7 @@ describe('software factory onboarding', () => { expect(prepare).toContain("reference='Fixes #507'"); expect(validate).toContain('title_length=9'); expect(validate).toContain("identifier='#507'"); - expect(calls.indexOf(validate)).toBeLessThan(calls.indexOf('git push --set-upstream origin HEAD')); + expect(calls.indexOf(validate)).toBeLessThan(calls.indexOf(PUSH)); expect(createCall(calls)).toBe(FLOW_OPEN_CHANGE_COMMAND + " --title 'Fix login' --body-file .relayflow/pr-body.md"); expect(finish).toBe('needs_human'); }); @@ -490,7 +495,7 @@ describe('software factory onboarding', () => { expect(calls.find(call => call.endsWith(FLOW_BASE_CHECK_COMMAND))).toBe('base=abc123; ' + FLOW_BASE_CHECK_COMMAND); expect(reportCall(calls)).toMatch(/^check=fail; baseline=fail; /); expect(createCall(calls)).toContain('--draft'); - expect(calls).toContain('git push --set-upstream origin HEAD'); + expect(calls).toContain(PUSH); expect(calls).toContain('adversary-2:codex'); expect(finish).toBe('needs_human'); expect(errors.join('\n')).toContain('not because of this change'); @@ -501,7 +506,7 @@ describe('software factory onboarding', () => { expect(reportCall(calls)).toMatch(/^check=fail; baseline=pass; /); expect(createCall(calls)).toContain('--draft'); // The work is pushed, never thrown away; the reviews are not worth running. - expect(calls).toContain('git push --set-upstream origin HEAD'); + expect(calls).toContain(PUSH); expect(calls.some(call => call.startsWith('adversary-'))).toBe(false); expect(finish).toBe('step_failed'); expect(errors.join('\n')).toContain('breaks checks that pass on the base commit'); @@ -527,14 +532,14 @@ describe('software factory onboarding', () => { const drops = calls.flatMap((call, index) => call.endsWith(FLOW_DROP_WORKING_FILES_COMMAND) ? [index] : []); expect(drops).toHaveLength(2); expect(drops[0]).toBeLessThan(calls.findIndex(call => call.endsWith(FLOW_PUBLISH_CHECK_COMMAND))); - expect(drops[1]).toBeLessThan(calls.indexOf('git push')); + expect(drops[1]).toBeLessThan(calls.indexOf(REVISION_PUSH)); expect(drops[1]).toBeGreaterThan(calls.indexOf('fixer:claude')); }); it('pushes a review fix that breaks passing checks, then drafts the pull request and stops', async () => { const { calls, finish } = await runFactory([false, true], true, matchingIssue, completed, 'publish', ['pass', 'fail', 'fail', 'fail']); - expect(calls).toContain('git push'); - expect(calls.indexOf(FLOW_CHECK_BLOCKED_COMMAND)).toBeGreaterThan(calls.indexOf('git push')); + expect(calls).toContain(REVISION_PUSH); + expect(calls.indexOf(FLOW_CHECK_BLOCKED_COMMAND)).toBeGreaterThan(calls.indexOf(REVISION_PUSH)); expect(calls.filter(call => call.startsWith('check=')).at(-1)).toMatch(/^check=fail; baseline=revision; /); expect(calls).not.toContain('adversary-2:codex'); expect(finish).toBe('step_failed'); diff --git a/web/lib/test/flow-push-guard.test.ts b/web/lib/test/flow-push-guard.test.ts new file mode 100644 index 0000000..69399e4 --- /dev/null +++ b/web/lib/test/flow-push-guard.test.ts @@ -0,0 +1,280 @@ +import { afterAll, describe, expect, it } from 'vitest'; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { FLOW_PUSH_COMMAND, WORKFLOW_FILES_HINT } from '../flow-workflows'; +import { factorySource, DEFAULT_FACTORY, type FactoryDraft } from '../flow-onboarding'; + +/** + * Cloud run 065fd98f (issue-to-pr, AgentWorkforce/cloud-e2e-sandbox#40) lost + * a finished agent step — 11m24s, $6.80, three commits — at the push: + * + * ! [remote rejected] HEAD -> relayflow/issue-to-pr-065fd98f (refusing to + * allow a GitHub App to create or update workflow `.github/workflows/ci.yml` + * without `workflows` permission) + * + * These cases run the real push command against a real bare remote whose + * pre-receive hook refuses the way GitHub does: any new commit that touches + * `.github/workflows/` is refused, not only a branch tip that differs. + */ +const roots: string[] = []; +afterAll(() => { for (const root of roots) rmSync(root, { recursive: true, force: true }); }); + +const env = { + ...process.env, + GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', + GIT_AUTHOR_NAME: 'Agent', GIT_AUTHOR_EMAIL: 'agent@example.com', GIT_COMMITTER_NAME: 'Agent', GIT_COMMITTER_EMAIL: 'agent@example.com', +}; + +const REFUSAL = 'refusing to allow a GitHub App to create or update workflow'; + +// GitHub checks every commit the push adds. `allow-workflows` is a token with +// the permission; `reject-all` an unrelated failure; `always-refuse` the +// workflow refusal whatever is pushed; `fail-after-refusal` refuses anything +// after one workflow refusal. The refusal carries a credentialed URL so the +// scrubbing can be checked. +const HOOK = `#!/bin/sh +if [ -f reject-all ]; then echo "error: the remote is unavailable (simulated)" >&2; exit 1; fi +if [ -f fail-after-refusal ] && [ -f refused ]; then echo "error: the remote is unavailable (simulated)" >&2; exit 1; fi +if [ -f allow-workflows ]; then exit 0; fi +if [ -f always-refuse ]; then echo "${REFUSAL} \\\`.github/workflows/ci.yml\\\` without \\\`workflows\\\` permission" >&2; exit 1; fi +zero=0000000000000000000000000000000000000000 +while read old new ref; do + [ "$new" = "$zero" ] && continue + for c in $(git rev-list "$new" --not --all); do + f=$(git diff-tree -r -m --root --no-commit-id --name-only "$c" -- .github/workflows | head -n 1) + if [ -n "$f" ]; then + touch refused + echo "To https://x-access-token:ghs_SECRETTOKEN@github.com/acme/app.git" >&2 + echo "${REFUSAL} \\\`$f\\\` without \\\`workflows\\\` permission" >&2 + exit 1 + fi + done +done +exit 0 +`; + +function git(cwd: string, ...args: string[]) { + return execFileSync('git', args, { cwd, encoding: 'utf8', env }); +} + +function write(root: string, files: Record) { + for (const [name, content] of Object.entries(files)) { + const file = path.join(root, name); + if (content === null) { rmSync(file, { force: true }); continue; } + mkdirSync(path.dirname(file), { recursive: true }); + writeFileSync(file, content); + } +} + +function commit(root: string, message: string, files: Record) { + write(root, files); + git(root, 'add', '-A'); + git(root, 'commit', '-q', '-m', message); + return git(root, 'rev-parse', 'HEAD').trim(); +} + +/** A clone of a bare remote at `base`, on the run's branch. */ +function setup(baseFiles: Record, modes: string[] = []) { + const top = mkdtempSync(path.join(tmpdir(), 'flow-push-guard-')); + roots.push(top); + const remote = path.join(top, 'remote.git'); + const root = path.join(top, 'work'); + git(top, 'init', '-q', '--bare', '-b', 'main', remote); + mkdirSync(root); + git(root, 'init', '-q', '-b', 'main'); + const base = commit(root, 'base', baseFiles); + git(root, 'remote', 'add', 'origin', remote); + git(root, 'push', '-q', 'origin', 'main'); + git(root, 'checkout', '-q', '-b', 'relayflow/issue-to-pr-065fd98f'); + writeFileSync(path.join(remote, 'hooks', 'pre-receive'), HOOK); + chmodSync(path.join(remote, 'hooks', 'pre-receive'), 0o755); + for (const mode of modes) writeFileSync(path.join(remote, mode), ''); + return { root, remote, base }; +} + +function push(root: string, base: string, args = ' --set-upstream origin HEAD', extra = '', pathPrefix = '') { + const result = spawnSync('/bin/sh', ['-c', `base=${base}; ${extra}${FLOW_PUSH_COMMAND}${args}`], { + cwd: root, encoding: 'utf8', env: { ...env, ...(pathPrefix ? { PATH: `${pathPrefix}:${process.env.PATH}` } : {}) }, + }); + return { code: result.status, stdout: result.stdout, stderr: result.stderr }; +} + +const BRANCH = 'relayflow/issue-to-pr-065fd98f'; +const remoteHead = (remote: string) => spawnSync('git', ['rev-parse', '--verify', '--quiet', `refs/heads/${BRANCH}`], { cwd: remote, encoding: 'utf8', env }).stdout.trim(); +const show = (cwd: string, spec: string) => git(cwd, 'show', spec); +const tree = (cwd: string, ref: string) => git(cwd, 'ls-tree', '-r', '--name-only', ref).trim().split('\n').sort(); +const read = (root: string, name: string) => existsSync(path.join(root, name)) ? readFileSync(path.join(root, name), 'utf8') : ''; + +const CI = 'name: ci\non:\n pull_request:\n paths: [src/**]\njobs: {}\n'; +const CI_EDITED = 'name: ci\non:\n pull_request:\n paths: [src/**, docs/**]\njobs: {}\n'; +const BODY = 'Consolidates the docs.\n\nFixes #40\n'; + +describe('FLOW_PUSH_COMMAND', () => { + it('pushes a branch without workflow edits exactly as git push does', () => { + const { root, remote, base } = setup({ 'README.md': '#\n' }); + const head = commit(root, 'docs', { 'docs/a.md': 'a\n' }); + const result = push(root, base); + expect(result.code).toBe(0); + expect(result.stdout).not.toContain('push-guard'); + expect(remoteHead(remote)).toBe(head); + expect(git(root, 'rev-parse', '--abbrev-ref', '@{upstream}').trim()).toBe(`origin/${BRANCH}`); + }); + + it('pushes workflow edits unchanged when the token has the workflows permission', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }, ['allow-workflows']); + const head = commit(root, 'wire docs into CI', { '.github/workflows/ci.yml': CI_EDITED, 'docs/a.md': 'a\n' }); + const result = push(root, base); + expect(result.code).toBe(0); + expect(result.stdout).not.toContain('push-guard'); + expect(remoteHead(remote)).toBe(head); + }); + + it('withholds refused workflow edits, pushes the rest, and puts the patch in the pull-request body (run 065fd98f)', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + commit(root, 'docs: consolidate', { 'docs/a.md': 'a\n', 'README.md': '# docs moved\n' }); + commit(root, 'ci: check docs', { '.github/workflows/ci.yml': CI_EDITED, 'scripts/check-docs.sh': 'true\n' }); + const orig = commit(root, 'docs: index', { 'docs/index.md': 'index\n' }); + write(root, { '.relayflow/pr-body.md': BODY }); + + const result = push(root, base); + expect(result.code).toBe(0); + expect(result.stdout).toContain('relayflow push-guard: workflow edits withheld (1 files)\n'); + + // Pushed, and the local branch is what was pushed. + const pushed = remoteHead(remote); + expect(pushed).not.toBe(''); + expect(git(root, 'rev-parse', 'HEAD').trim()).toBe(pushed); + // The workflow file is at the merge-base state in every pushed commit. + expect(show(remote, `${pushed}:.github/workflows/ci.yml`)).toBe(CI); + expect(git(remote, 'rev-list', '--count', `${base}..${pushed}`, '--', '.github/workflows').trim()).toBe('0'); + // Everything else the agent did is there, commit by commit, with the + // same messages and author. + expect(tree(remote, pushed)).toEqual(['.github/workflows/ci.yml', 'README.md', 'docs/a.md', 'docs/index.md', 'scripts/check-docs.sh']); + expect(git(remote, 'log', '--format=%s|%an', `${base}..${pushed}`).trim().split('\n')).toEqual(['docs: index|Agent', 'ci: check docs|Agent', 'docs: consolidate|Agent']); + // The original commits are kept locally, and the working tree matches HEAD. + expect(git(root, 'rev-parse', 'refs/relayflow/withheld-workflows').trim()).toBe(orig); + expect(read(root, '.github/workflows/ci.yml')).toBe(CI); + expect(git(root, 'status', '--porcelain', '--untracked-files=no').trim()).toBe(''); + + // The patch reaches the pull-request body, and applies to the pushed branch. + const body = read(root, '.relayflow/pr-body.md'); + expect(body.startsWith(BODY)).toBe(true); + expect(body).toContain('## Workflow changes not applied'); + expect(body).toContain('lacks the `workflows` permission'); + expect(body).toContain('- M `.github/workflows/ci.yml`'); + expect(body).toContain('````diff\ndiff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml'); + expect(body).toContain('+ paths: [src/**, docs/**]'); + const patch = read(root, '.relayflow/workflow-changes.patch'); + expect(body).toContain(patch); + expect(spawnSync('git', ['apply', '--check', '.relayflow/workflow-changes.patch'], { cwd: root, env }).status).toBe(0); + // No credential from a remote URL reaches the step output. + expect(result.stderr).toContain(REFUSAL); + expect(result.stderr).not.toContain('ghs_SECRETTOKEN'); + }); + + it('handles an added and a deleted workflow file, and drops a commit that only touched workflows', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI, '.github/workflows/old.yml': 'name: old\n' }); + commit(root, 'feature', { 'src/x.ts': 'x\n' }); + commit(root, 'ci: replace old workflow', { '.github/workflows/new.yml': 'name: new\n', '.github/workflows/old.yml': null }); + const result = push(root, base); + expect(result.code).toBe(0); + expect(result.stdout).toContain('relayflow push-guard: workflow edits withheld (2 files)'); + const pushed = remoteHead(remote); + expect(tree(remote, pushed)).toEqual(['.github/workflows/ci.yml', '.github/workflows/old.yml', 'README.md', 'src/x.ts']); + expect(git(remote, 'log', '--format=%s', `${base}..${pushed}`).trim()).toBe('feature'); + expect(existsSync(path.join(root, '.github/workflows/new.yml'))).toBe(false); + expect(read(root, '.github/workflows/old.yml')).toBe('name: old\n'); + const patch = read(root, '.relayflow/workflow-changes.patch'); + expect(patch).toContain('new file mode'); + expect(patch).toContain('deleted file mode'); + // There was no pull-request body yet, so none is invented. + expect(existsSync(path.join(root, '.relayflow/pr-body.md'))).toBe(false); + expect(read(root, '.relayflow/workflow-changes.md')).toContain('- A `.github/workflows/new.yml`'); + expect(read(root, '.relayflow/workflow-changes.md')).toContain('- D `.github/workflows/old.yml`'); + }); + + it('fails as before on any other push failure', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }, ['reject-all']); + const head = commit(root, 'ci', { '.github/workflows/ci.yml': CI_EDITED }); + const result = push(root, base); + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('the remote is unavailable'); + expect(result.stdout).not.toContain('push-guard'); + expect(git(root, 'rev-parse', 'HEAD').trim()).toBe(head); + expect(remoteHead(remote)).toBe(''); + }); + + it('falls back to nothing when the refused branch has no workflow edits of its own', () => { + const { root, remote, base } = setup({ 'README.md': '#\n' }, ['always-refuse']); + const head = commit(root, 'docs', { 'docs/a.md': 'a\n' }); + const result = push(root, base); + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('nothing to withhold'); + expect(result.stdout).not.toContain('withheld'); + expect(git(root, 'rev-parse', 'HEAD').trim()).toBe(head); + expect(remoteHead(remote)).toBe(''); + expect(existsSync(path.join(root, '.relayflow'))).toBe(false); + }); + + it('fails with the original error, and restores HEAD, when the second push fails too', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }, ['fail-after-refusal']); + const head = commit(root, 'ci', { '.github/workflows/ci.yml': CI_EDITED, 'docs/a.md': 'a\n' }); + const result = push(root, base); + expect(result.code).not.toBe(0); + expect(result.stderr).toContain('The original error was:'); + expect(result.stderr.split(REFUSAL).length).toBeGreaterThan(2); + expect(result.stderr).not.toContain('ghs_SECRETTOKEN'); + expect(git(root, 'rev-parse', 'HEAD').trim()).toBe(head); + expect(read(root, '.github/workflows/ci.yml')).toBe(CI_EDITED); + expect(remoteHead(remote)).toBe(''); + }); + + it('keeps pushed commits and comments on the open pull request for a revision push', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + const first = commit(root, 'feature', { 'src/x.ts': 'x\n' }); + expect(push(root, base).code).toBe(0); + commit(root, 'review fixes', { 'src/x.ts': 'y\n', '.github/workflows/ci.yml': CI_EDITED }); + // A stand-in gh records the comment the guard posts. + const bin = mkdtempSync(path.join(tmpdir(), 'flow-push-guard-bin-')); + roots.push(bin); + writeFileSync(path.join(bin, 'gh'), '#!/bin/sh\nprintf "%s " "$@" > "$(dirname "$0")/gh.args"\ncat "$4" > "$(dirname "$0")/gh.body"\n'); + chmodSync(path.join(bin, 'gh'), 0o755); + const result = push(root, base, '', 'comment=yes; ', bin); + expect(result.code).toBe(0); + expect(result.stdout).toContain('workflow edits withheld (1 files)'); + const pushed = remoteHead(remote); + // A fast-forward: the commit already on the remote was not rewritten. + expect(git(remote, 'rev-parse', `${pushed}~1`).trim()).toBe(first); + expect(show(remote, `${pushed}:src/x.ts`)).toBe('y\n'); + expect(show(remote, `${pushed}:.github/workflows/ci.yml`)).toBe(CI); + expect(read(bin, 'gh.args')).toBe('pr comment --body-file .relayflow/workflow-changes.md '); + expect(read(bin, 'gh.body')).toContain('+ paths: [src/**, docs/**]'); + }); + + it('bounds the patch in the pull-request body and says where the rest is', () => { + const { root, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + commit(root, 'ci', { '.github/workflows/ci.yml': CI + '# padding\n'.repeat(4000), 'docs/a.md': 'a\n' }); + write(root, { '.relayflow/pr-body.md': BODY }); + const result = push(root, base, ' --set-upstream origin HEAD', 'RELAYFLOW_WITHHELD_PATCH_LIMIT=2000; export RELAYFLOW_WITHHELD_PATCH_LIMIT; '); + expect(result.code).toBe(0); + const body = read(root, '.relayflow/pr-body.md'); + expect(body).toMatch(/_Truncated to 2000 of \d+ bytes\. The full patch is \.relayflow\/workflow-changes\.patch/); + expect(body.length).toBeLessThan(BODY.length + 3000); + // Every fence opened is closed. + expect(body.split('````').length).toBe(3); + }); +}); + +describe('generated flows', () => { + const draft: FactoryDraft = { ...DEFAULT_FACTORY, version: 4, sources: ['github'], sourceSettings: { github: { repository: 'acme/app', labels: '' } }, agents: ['claude', 'codex'], task: 'Add a test', step: 3 } as FactoryDraft; + it.each(['traditional', 'prototype', 'simple'] as const)('%s pushes only through the guard and tells every agent about workflow files', (workflow) => { + for (const target of ['cloud', 'local'] as const) { + const source = factorySource({ ...draft, workflow }, target); + expect(source).toContain(JSON.stringify(WORKFLOW_FILES_HINT)); + expect(source).toContain(JSON.stringify(FLOW_PUSH_COMMAND)); + expect(source).not.toMatch(/f\.run\("git push/); + } + }); +}); From 1aac80aea0b606c31d96338e16449b01b79027bf Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 21 Sep 2026 22:36:57 -0700 Subject: [PATCH 2/2] fix(flows): harden the workflow push guard per review Review on #114 (Devin, Codex, CodeRabbit): - workflow-only changes: when withholding leaves nothing new to push, never publish an unchanged branch (gh pr create would fail and the patch would be lost). A revision posts the patch to the open pull request; a first push fails loudly with the patch in its output. - uncommitted edits: workflow paths with staged, unstaged or untracked changes relative to the original tip are left untouched; only committed edits are withheld. - mktemp failure: refuse instead of falling back to a push whose output could not be scrubbed of a credentialed remote URL. - a second refusal keeps the earlier withheld commits reachable under refs/relayflow/withheld-workflows-history/. - the reviewer section is measured whole (heading, a file list capped at 50, fences, notes) before the patch gets the remaining room under GitHub's 65,536-character limit. Co-Authored-By: Claude Opus 5 --- web/lib/flow-workflows.ts | 47 ++++++++++---- web/lib/test/flow-push-guard.test.ts | 93 ++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/web/lib/flow-workflows.ts b/web/lib/flow-workflows.ts index 65d6808..af1b898 100644 --- a/web/lib/flow-workflows.ts +++ b/web/lib/flow-workflows.ts @@ -371,7 +371,9 @@ const SCRUB_URL_CREDENTIALS = "sed -e 's#://[^/@[:space:]]*@#://#g'"; */ export const FLOW_PUSH_COMMAND = 'relayflow_push() { ' + [ 'wf=.github/workflows', - 'err=$(mktemp "${TMPDIR:-/tmp}/relayflow-push.XXXXXX") || { git push "$@"; return; }', + // Without the temp file git's output cannot be scrubbed of a credentialed + // remote URL, so refuse rather than push unscrubbed. + 'err=$(mktemp "${TMPDIR:-/tmp}/relayflow-push.XXXXXX") || { echo "relayflow push-guard: could not create a temporary file, so the push was not attempted (its output could not be scrubbed of credentials)." >&2; return 1; }', 'git push "$@" 2>"$err"; status=$?', `${SCRUB_URL_CREDENTIALS} "$err" >&2`, 'if [ "$status" -eq 0 ]; then rm -f "$err"; return 0; fi', @@ -393,23 +395,44 @@ export const FLOW_PUSH_COMMAND = 'relayflow_push() { ' + [ + 'echo "$c $new" >> "$tmp/map"; done', 'new=$(relayflow_new "$orig")', 'if [ "$ok" != yes ] || [ "$new" = "$orig" ]; then echo "relayflow push-guard: could not withhold the workflow edits." >&2; rm -rf "$tmp" "$err"; return "$status"; fi', - 'git update-ref -m "relayflow: withhold workflow edits" HEAD "$new" "$orig"', - 'git push "$@" 2>"$tmp/push"; again=$?', - `${SCRUB_URL_CREDENTIALS} "$tmp/push" >&2`, - `if [ "$again" -ne 0 ]; then git update-ref -m "relayflow: restore after a failed push" HEAD "$orig" "$new"; echo "relayflow push-guard: the push failed again after withholding the workflow edits. The original error was:" >&2; ${SCRUB_URL_CREDENTIALS} "$err" >&2; rm -rf "$tmp" "$err"; return "$again"; fi`, - 'git update-ref refs/relayflow/withheld-workflows "$orig"', - 'git diff --name-only --no-renames "$new" "$orig" -- "$wf" | while IFS= read -r p; do if git cat-file -e "$new:$p" 2>/dev/null; then git checkout -q "$new" -- "$p"; else git rm -q -f --ignore-unmatch -- "$p" >/dev/null 2>&1; rm -f -- "$p"; fi; done', 'mkdir -p .relayflow; patch=.relayflow/workflow-changes.patch; section=.relayflow/workflow-changes.md', 'git diff --full-index "$mb" "$orig" -- "$wf" > "$patch"', 'n=$(git diff --name-only "$mb" "$orig" -- "$wf" | wc -l | tr -d " ")', - 'if [ -s "$patch" ]; then ' + // Builds the reviewer-facing section. The heading, explanation and a file + // list capped at 50 entries are written and measured first; only the room + // left under GitHub's 65,536-character body/comment limit (kept at 65,000, + // minus what the body already holds and ~400 bytes of fences and notes) + // goes to the patch. + 'relayflow_section() { ' + 'used=0; if [ "${comment:-}" != yes ] && [ -f .relayflow/pr-body.md ]; then used=$(wc -c < .relayflow/pr-body.md | tr -d " "); fi; ' - + 'limit=${RELAYFLOW_WITHHELD_PATCH_LIMIT:-61440}; room=$((61000 - used)); if [ "$room" -lt "$limit" ]; then limit=$room; fi; size=$(wc -c < "$patch" | tr -d " "); ' + `{ printf '\\n## Workflow changes not applied\\n\\n%s\\n\\n' "The GitHub App token this run pushes with lacks the \\\`workflows\\\` permission, so GitHub refused the commits that change \\\`.github/workflows/\\\`. The rest of the work is pushed; these edits were taken out of its commits. Apply them manually:"; ` - + `git diff --name-status "$mb" "$orig" -- "$wf" | awk -F '\\t' '{ printf "- %s \\140%s\\140\\n", substr($1, 1, 1), $NF }'; ` - + `if [ "$limit" -le 0 ]; then printf '\\n%s\\n' "_The patch ($size bytes) does not fit in the pull request body. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; ` + + `git diff --name-status "$mb" "$orig" -- "$wf" | awk -F '\\t' '{ printf "- %s \\140%s\\140\\n", substr($1, 1, 1), $NF }' | head -n 50; ` + + `if [ "$n" -gt 50 ]; then printf -- '- …and %s more\\n' "$((n - 50))"; fi; } > "$tmp/head"; ` + + 'overhead=$(( $(wc -c < "$tmp/head" | tr -d " ") + 400 )); ' + + 'limit=${RELAYFLOW_WITHHELD_PATCH_LIMIT:-61440}; room=$((65000 - used - overhead)); if [ "$room" -lt "$limit" ]; then limit=$room; fi; size=$(wc -c < "$patch" | tr -d " "); ' + + `{ cat "$tmp/head"; if [ "$limit" -le 0 ]; then printf '\\n%s\\n' "_The patch ($size bytes) does not fit in the pull request body. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; ` + `elif [ "$size" -le "$limit" ]; then printf '\\n\`\`\`\`diff\\n'; cat "$patch"; printf '\`\`\`\`\\n'; ` - + `else printf '\\n\`\`\`\`diff\\n'; head -c "$limit" "$patch" | sed '$d'; printf '\`\`\`\`\\n\\n%s\\n' "_Truncated to $limit of $size bytes. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; fi; } > "$section"; ` + + `else printf '\\n\`\`\`\`diff\\n'; head -c "$limit" "$patch" | sed '$d'; printf '\`\`\`\`\\n\\n%s\\n' "_Truncated to $limit of $size bytes. The full patch is .relayflow/workflow-changes.patch in the run workspace, and in this push step's output._"; fi; } > "$section"; }`, + // Workflow paths with uncommitted (staged, unstaged or untracked) edits + // relative to the original tip are never reset below: only committed edits + // are withheld, and an agent's in-progress work stays exactly as it was. + 'dirty=$( { git diff --name-only "$orig" -- "$wf"; git diff --cached --name-only "$orig" -- "$wf"; git ls-files --others --exclude-standard -- "$wf"; } 2>/dev/null | sort -u)', + // Every new change was a workflow edit: pushing would publish a branch with + // no change, and GitHub cannot open a pull request for it. For a revision + // (pull request already open) the patch goes to it as a comment; otherwise + // the step fails with the patch in its output rather than losing it silently. + 'if [ -z "$(git rev-list "$new" --not --remotes 2>/dev/null)" ]; then relayflow_section; cat "$patch" >&2; ' + + 'if [ "${comment:-}" = yes ] && gh pr comment --body-file "$section" >/dev/null 2>&1; then echo "relayflow push-guard: every new change edits $wf, which this token cannot push, so nothing else was pushed; posted the withheld workflow changes to the pull request." >&2; rm -rf "$tmp" "$err"; return 0; fi; ' + + 'echo "relayflow push-guard: every change in this run edits $wf, which this token cannot push, so there is nothing else to publish. The edits are in $patch and printed above." >&2; rm -rf "$tmp" "$err"; return "$status"; fi', + 'git update-ref -m "relayflow: withhold workflow edits" HEAD "$new" "$orig"', + 'git push "$@" 2>"$tmp/push"; again=$?', + `${SCRUB_URL_CREDENTIALS} "$tmp/push" >&2`, + `if [ "$again" -ne 0 ]; then git update-ref -m "relayflow: restore after a failed push" HEAD "$orig" "$new"; echo "relayflow push-guard: the push failed again after withholding the workflow edits. The original error was:" >&2; ${SCRUB_URL_CREDENTIALS} "$err" >&2; rm -rf "$tmp" "$err"; return "$again"; fi`, + 'previous=$(git rev-parse --verify --quiet refs/relayflow/withheld-workflows 2>/dev/null || :)', + 'if [ -n "$previous" ] && [ "$previous" != "$orig" ]; then git update-ref "refs/relayflow/withheld-workflows-history/$previous" "$previous"; fi', + 'git update-ref refs/relayflow/withheld-workflows "$orig"', + 'git diff --name-only --no-renames "$new" "$orig" -- "$wf" | while IFS= read -r p; do if printf "%s\\n" "$dirty" | grep -qxF -- "$p"; then echo "relayflow push-guard: $p has uncommitted edits, so it was left as it is." >&2; continue; fi; if git cat-file -e "$new:$p" 2>/dev/null; then git checkout -q "$new" -- "$p"; else git rm -q -f --ignore-unmatch -- "$p" >/dev/null 2>&1; rm -f -- "$p"; fi; done', + 'if [ -s "$patch" ]; then relayflow_section; ' + 'if [ "$size" -gt "$limit" ]; then cat "$patch" >&2; fi; ' + 'if [ "${comment:-}" = yes ]; then if gh pr comment --body-file "$section" >/dev/null 2>&1; then echo "relayflow push-guard: posted the withheld workflow changes to the pull request." >&2; else echo "relayflow push-guard: could not comment on the pull request; $section holds the withheld workflow changes." >&2; fi; ' + 'elif [ -f .relayflow/pr-body.md ]; then cat "$section" >> .relayflow/pr-body.md; fi; fi', diff --git a/web/lib/test/flow-push-guard.test.ts b/web/lib/test/flow-push-guard.test.ts index 69399e4..99987a5 100644 --- a/web/lib/test/flow-push-guard.test.ts +++ b/web/lib/test/flow-push-guard.test.ts @@ -253,6 +253,99 @@ describe('FLOW_PUSH_COMMAND', () => { expect(read(bin, 'gh.body')).toContain('+ paths: [src/**, docs/**]'); }); + // Review on #114: when every commit only edits workflows, withholding + // leaves nothing to publish. Pushing the base as the branch would make + // `gh pr create` fail with the patch lost, so a first push fails loudly + // with the patch in its output instead. + it('fails loudly with the patch in its output when every change is a workflow edit', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + const orig = commit(root, 'ci: only', { '.github/workflows/ci.yml': CI_EDITED }); + write(root, { '.relayflow/pr-body.md': BODY }); + const result = push(root, base); + expect(result.code).not.toBe(0); + expect(remoteHead(remote)).toBe(''); + expect(git(root, 'rev-parse', 'HEAD').trim()).toBe(orig); + expect(result.stderr).toContain('every change in this run edits .github/workflows'); + expect(result.stderr).toContain('+ paths: [src/**, docs/**]'); + expect(read(root, '.relayflow/workflow-changes.patch')).toContain('+ paths: [src/**, docs/**]'); + expect(result.stderr).not.toContain('ghs_SECRETTOKEN'); + }); + + it('comments the patch on the open pull request when a revision only edits workflows', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + commit(root, 'feature', { 'src/x.ts': 'x\n' }); + expect(push(root, base).code).toBe(0); + const before = remoteHead(remote); + commit(root, 'review: ci only', { '.github/workflows/ci.yml': CI_EDITED }); + const bin = mkdtempSync(path.join(tmpdir(), 'flow-push-guard-bin-')); + roots.push(bin); + writeFileSync(path.join(bin, 'gh'), '#!/bin/sh\nprintf "%s " "$@" > "$(dirname "$0")/gh.args"\ncat "$4" > "$(dirname "$0")/gh.body"\n'); + chmodSync(path.join(bin, 'gh'), 0o755); + const result = push(root, base, '', 'comment=yes; ', bin); + expect(result.code).toBe(0); + expect(remoteHead(remote)).toBe(before); + expect(read(bin, 'gh.body')).toContain('+ paths: [src/**, docs/**]'); + }); + + // Review on #114: without a temp file git's output cannot be scrubbed, so + // the guard must refuse rather than fall back to an unscrubbed push. + it('refuses to push when it cannot create the temp file that scrubs git output', () => { + const { root, remote, base } = setup({ 'README.md': '#\n' }); + commit(root, 'feature', { 'src/x.ts': 'x\n' }); + const result = push(root, base, ' --set-upstream origin HEAD', 'TMPDIR=/nonexistent/relayflow-tmp; export TMPDIR; '); + expect(result.code).toBe(1); + expect(remoteHead(remote)).toBe(''); + expect(result.stderr).toContain('could not create a temporary file, so the push was not attempted'); + }); + + // Review on #114: resetting a workflow path must not destroy an agent's + // staged or unstaged edits to it. + it('keeps uncommitted edits to a withheld workflow file exactly as they were', () => { + const { root, remote, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + commit(root, 'feature and ci', { 'src/x.ts': 'x\n', '.github/workflows/ci.yml': CI_EDITED }); + const correction = CI_EDITED + '# a correction still in progress\n'; + write(root, { '.github/workflows/ci.yml': correction }); + const result = push(root, base); + expect(result.code).toBe(0); + expect(show(remote, `${remoteHead(remote)}:.github/workflows/ci.yml`)).toBe(CI); + expect(read(root, '.github/workflows/ci.yml')).toBe(correction); + expect(result.stderr).toContain('.github/workflows/ci.yml has uncommitted edits, so it was left as it is.'); + }); + + // Review on #114: a second refusal must not orphan the first withheld history. + it('keeps the earlier withheld commits reachable after a second refusal', () => { + const { root, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); + commit(root, 'one', { 'src/x.ts': 'x\n', '.github/workflows/ci.yml': CI_EDITED }); + expect(push(root, base).code).toBe(0); + const first = git(root, 'rev-parse', 'refs/relayflow/withheld-workflows').trim(); + commit(root, 'two', { 'src/y.ts': 'y\n', '.github/workflows/ci.yml': CI_EDITED + '# again\n' }); + expect(push(root, base, '').code).toBe(0); + const second = git(root, 'rev-parse', 'refs/relayflow/withheld-workflows').trim(); + expect(second).not.toBe(first); + expect(git(root, 'rev-parse', `refs/relayflow/withheld-workflows-history/${first}`).trim()).toBe(first); + }); + + // Review on #114: the whole section, not just the patch, must fit GitHub's + // 65,536-character body limit. + it('keeps the pull-request body under GitHub\'s limit even with a long file list', () => { + const files: Record = { 'README.md': '#\n' }; + const edits: Record = { 'src/x.ts': 'x\n' }; + for (let i = 0; i < 120; i += 1) { + const name = `.github/workflows/${'w'.repeat(120)}-${i}.yml`; + files[name] = 'name: w\n'; + edits[name] = 'name: w\n' + '# padding padding padding\n'.repeat(20); + } + const { root, base } = setup(files); + commit(root, 'many workflows', edits); + write(root, { '.relayflow/pr-body.md': BODY + 'x'.repeat(20000) + '\n' }); + const result = push(root, base); + expect(result.code).toBe(0); + const body = read(root, '.relayflow/pr-body.md'); + expect(body.length).toBeLessThanOrEqual(65536); + expect(body).toContain('…and 70 more'); + expect(body.split('````').length % 2).toBe(1); + }, 60_000); + it('bounds the patch in the pull-request body and says where the rest is', () => { const { root, base } = setup({ 'README.md': '#\n', '.github/workflows/ci.yml': CI }); commit(root, 'ci', { '.github/workflows/ci.yml': CI + '# padding\n'.repeat(4000), 'docs/a.md': 'a\n' });