Scope installation token permissions in restricted mode (#226)
* Scope installation token permissions in restricted mode
In restricted/disabled bash mode, the installation token is now scoped
to match the workflow's permissions block. This preserves fork push
capability while limiting what the agent can do with the token.
- Read workflow permissions from pullfrog.yml at runtime
- Pass permissions to API when acquiring installation token
- Clear OIDC env vars in restricted mode to prevent token minting
- Simplify setupGit by moving token resolution to main.ts
* Address review feedback: fail closed with default permissions
- Add restrictive default permissions (contents:read, pull_requests:read,
issues:read) as fallback when workflow permissions can't be read
- Add support for job-level permissions via GITHUB_JOB env var
- Fix misleading comment about token resolution in restricted mode
- Add documentation about fork PR checkout behavior
* Simplify to separate git/MCP tokens without workflow permission scoping
- gitToken: minimal contents:write only (assumed exfiltratable)
- mcpToken: full installation token (not exfiltratable via MCP tools)
- Remove workflowPermissions.ts - security-conscious users can pass
their own token via GH_TOKEN or inputs.token
- Add type-safe InstallationTokenPermissions to github.ts and API route
* Rename `write` permission to `push` and remove vestigial tool blocking
The `write` permission was previously used to block local file write tools
in agents. This was security theater since bash can write files anyway.
Now `push` only controls the git token scope:
- push: enabled → contents:write (can push commits)
- push: disabled → contents:read (read-only, can't push)
Changes:
- Rename `write` to `push` in action.yml, Prisma schema, and all TS types
- Remove vestigial write tool blocking from all agents (claude, cursor,
gemini, opencode, codex)
- Add data-preserving Prisma migration using RENAME COLUMN
- Update UI: "Write files" → "Git push" with updated description
* add PID namespace isolation for bash sandbox
when running in CI, attempts to use unshare --pid to create a new PID
namespace for bash subprocesses. this prevents the /proc/$PPID/environ
attack where a malicious command could read secrets from the parent
process's environment.
the protection works by:
1. creating a new PID namespace (subprocess becomes PID 1)
2. mounting fresh /proc showing only sandbox PIDs
3. parent PIDs become invisible (PPID = 0, /proc/0 doesn't exist)
combined with filterEnv(), this provides complete protection against
/proc-based secret theft. falls back gracefully if namespaces aren't
available.
includes test script to verify the protection works.
* add PID namespace test to CI workflow
tests whether unshare --pid works on GHA runners out of the box,
and if not, whether enabling via sysctl helps. also runs the
pidNamespace.ts test to verify the full protection.
* fix pnpm setup and add procIsolation agent test
- fix pnpm/action-setup by specifying package_json_file path
- add procIsolation crossagent test that has agent attempt to
read secrets via /proc/$PPID/environ
- add procIsolation to CI test matrix
* add pid-namespace test job to main workflow
this job tests unshare --pid capabilities on GHA runners and runs
the pidNamespace.ts adhoc test to verify /proc isolation works
* test bubblewrap's sysctl approach for enabling namespaces
- write to /etc/sysctl.d/99-userns.conf and run sysctl --system
- try aa-complain on unshare binary
- more detailed diagnostics
* fix pidNamespace test and add sudo-unshare fallback for GHA
- fix reference error in pidNamespace.ts (renamed function but didn't update calls)
- add sudo-unshare as fallback method for GHA runners where unprivileged
namespaces are blocked but sudo is available
- update bash.ts to detect and use sudo unshare when unprivileged fails
* consolidate security docs and document PID namespace isolation
- update security.md with current implementation details
- document sudo unshare fallback for GHA runners
- add testing instructions for local Docker and CI
- add "Further Exploration" section with Landlock and path validation ideas
- delete bash-sandbox.md and landlock.md (consolidated into security.md)
* move procIsolation test to adhoc folder
the procIsolation test requires PID namespace capabilities that aren't
available in the Docker test environment. moved to adhoc/ so it's excluded
from default test runs and can be run explicitly when needed (e.g. via
the pid-namespace CI job or locally with --privileged docker).
* fix Docker test environment for PID namespace isolation
- add CI and GITHUB_ACTIONS to testEnvAllowList so sandbox detection runs
- add --privileged to Docker run for PID namespace support (unshare)
this fixes the test environment to properly test the sandbox. in production,
the action runs directly on GHA runner where sudo unshare works.
* fix getJobToken() to work in test environment
add fallback to GH_TOKEN and GITHUB_TOKEN when INPUT_TOKEN is not set.
this allows tests to run without requiring workflow-level token input.
the token resolution order is:
1. INPUT_TOKEN (from workflow `with: token:`)
2. GH_TOKEN (external token override)
3. GITHUB_TOKEN (pre-acquired in tests or from GHA env)
* security: filter secrets from all subprocess environments
- extract filterEnv() to shared utils/secrets.ts
- make $() utility filter secrets by default (git, npm, etc. don't need them)
- disable git hooks via core.hooksPath to prevent hook-based exfiltration
- git auth uses token embedded in URL, not env vars
this prevents malicious git hooks, npm postinstall scripts, and other
code execution vectors from exfiltrating GITHUB_TOKEN and API keys.
* docs: clarify defense-in-depth security model
update security.md to explain why BOTH layers are required:
- filterEnv(): cleans child's own /proc/self/environ
- PID namespace: hides parent's /proc entries
PID namespace alone isn't sufficient - with --mount-proc, the child
becomes PID 1, so /proc/1/environ is the child's OWN environment.
without filterEnv(), secrets would still be accessible there.
* add procSandbox crossagent test for PID namespace security
- add crossagent/procSandbox.ts: security test that instructs agent to try
various /proc attack vectors and validates sandbox blocks them
- update wiki/security.md: document PID namespace isolation details, add
verification commands, explain why sudo inside sandbox doesn't break security
- update docker.ts: use node:24 with sudo for GHA-like test environment
- update instructions.ts: allow disabling security messaging for pentests
- clean up adhoc test files (procIsolation.ts, securityAudit.ts)
the procSandbox test sets SANDBOX_TEST_TOKEN (matches *_TOKEN filter), gives
the agent explicit hints about attack vectors (/proc, sudo, nsenter, etc.),
disables prompt-based mitigations, and validates the secret is never leaked.
* move procSandbox test to agnostic/ (runs with one agent)
* WIP
* docs: add agent testing guide (pnpm play, Docker, pentesting)
* docs: add CI details to agent testing guide
* docs: add interesting findings and gotchas from pentesting
* improve test fidelity: auto-set CI=true, verify sandbox active
- docker.ts: always set CI=true in container so sandbox activates
- docker.ts: skip sudo/user setup if already done (faster reruns)
- bash.ts: export getSandboxMethod() for diagnostics
- bash.ts: add debug log when sandbox disabled
- procSandbox.ts: add sandbox_was_active check to catch vacuous passes
the CI=true change is critical: without it, PID namespace isolation
is skipped and security tests pass without actually testing anything.
* docs: update agent-testing.md with CI=true auto-set note
* docs: clarify log format is agent-specific
* fix git auth, simplify MCP tools, add adversarial tests
- fix gitWithAuth to use Basic auth format (Bearer doesn't work with git's http.extraheader)
- fix token.ts: push:restricted now correctly gets contents:write
- fix github.ts: pass permissions when acquiring installation tokens locally
- remove commit_files and create_branch MCP tools (redundant, don't require credentials)
- remove containsSecrets() - trivially bypassable, not sound security
- add agnostic adversarial tests: pushDisabled, pushRestricted, tokenExfil
- update instructions.ts to clearly list available git MCP tools
- add wiki/git.md documenting credential isolation and push permission tiers
- update wiki/docker.md with custom image considerations
- update wiki/agent-testing.md with adversarial testing patterns
* fix type errors after rebase
- change ResolveTokensParams.push from ToolPermission to PushPermission
- use tags: ["agnostic"] instead of agnostic: true in test files
* fix cleanup permission error in sandbox tests
when sandbox isolation is enabled (CI=true), files created by the unshare
subprocess may have different ownership, causing rmSync to fail with EACCES.
this error in the finally block was overriding the test's success result.
fix: wrap cleanup in try-catch and fall back to sudo rm if rmSync fails.
* Add adhoc
* Handle git config/remote bypasses
* add git hooks protection and simplify ToolState
- disable git hooks in restricted mode via -c core.hooksPath=/dev/null
- add gitHooks adversarial test to verify hook protection works
- unify prNumber/issueNumber into single issueNumber field
- add pushUrl to ToolState for push validation
- add generateTestMarker() for simpler single-agent test markers
- export SENSITIVE_PATTERNS and isSensitiveEnvName from secrets.ts
- remove redundant pidNamespace.ts (duplicated by procSandbox.ts)
- update documentation
* harden $git() auth: subcommand whitelist, binary tamper detection
- rename gitWithAuth() to $git() with explicit subcommand first arg
- restrict to "fetch" | "push" at type level (filters don't run for these)
- resolve git binary path at startup via resolveGit(), sha256 fingerprint
- verify hash before each $git() call to detect binary replacement
- rename disableHooks to restricted for cleaner semantics
- document filter exfiltration attack and empirical verification in wiki
* remove redundant pid-namespace CI job
the PID namespace isolation testing is now handled by
action/test/agnostic/procSandbox.ts via pnpm runtest agnostic
* fix push_branch for new branches and improve token leak detection
- getPushDestination now falls back to origin/<branch> when @{push}
is not configured (happens for new branches created locally)
- gitPerms validator now checks for actual token patterns instead
of matching "x-access-token" string in test instructions
* use kebab-case for test names
* simplify shell env API: "restricted" | "inherit" | object
replace passFullEnv boolean with cleaner env option that accepts:
- "restricted" (default): filterEnv() to prevent secret leakage
- "inherit": full process.env
- object: custom env merged with restricted base
* share EnvMode and resolveEnv between shell.ts and bash.ts
move shared env resolution logic to secrets.ts
* add env option to bash tool (default: restricted)
* delete agent-testing.md (renamed to adversarial.md)
* Add checkout tests
* reframe githooks test prompt to avoid claude safety refusal
claude was refusing to execute the test because the prompt used words
like "malicious" and "security testing". reframed as a debugging task
with innocuous env var name (TESTING_DEBUG_TAG) per adversarial.md guidance.
Co-authored-by: Cursor <cursoragent@cursor.com>
* clean up verbose token acquisition logs
move logging responsibility to call sites which have better context
(git token vs MCP token). remove redundant intermediate OIDC logs
and unused "(permission-scoped)" suffix.
Co-authored-by: Cursor <cursoragent@cursor.com>
* isolate agnostic tests with matrix strategy, fix .pullfrog-env secret leak
- split action-agnostic into per-test matrix jobs for isolated logs and filesystems
- only write explicitly opted-in env vars to .pullfrog-env via fileAgentEnv
(fixes token-exfil test where claude found SANDBOX_TEST_TOKEN on disk)
- mcpmerge test opts in via fileAgentEnv for cursor's repo-level MCP fallback
Co-authored-by: Cursor <cursoragent@cursor.com>
* remove env parameter from bash tool to prevent agents bypassing filterEnv
the bash tool exposed an `env` parameter accepting "restricted" | "inherit"
which allowed agents to pass env: "inherit" and see all secrets including
SANDBOX_TEST_TOKEN, bypassing the restricted environment filtering entirely.
env mode is now determined internally (always restricted).
Co-authored-by: Cursor <cursoragent@cursor.com>
* use pullfrog/test-repo for push tests to stop polluting main repo
push tests were creating branches and tags on pullfrog/app directly.
now all push tests (push-restricted, push-disabled, push-enabled,
git-permissions) target pullfrog/test-repo instead.
Co-authored-by: Cursor <cursoragent@cursor.com>
* use pullfrog/test-repo for all tests, not just push tests
no test should clone or operate on pullfrog/app directly.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix token scoping for test-repo and bash timeout defaults
- acquireTokenViaOIDC now includes GITHUB_REPOSITORY repo in token
scope so push tests work against pullfrog/test-repo
- bash tool default timeout: 120s -> 30s, cap: 600s -> 120s
- activity timeout: 30s -> 60s
- prevents hung bash commands (e.g. find /) from killing the agent
via activity timeout
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Colin McDonnell
committed by
pullfrog[bot]
pullfrog[bot]
parent
6fbff21fca
commit
3a7145db1a
@@ -1,145 +1,108 @@
|
||||
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
|
||||
|
||||
exports[`fetchAndFormatPrDiff > fetches PR files and generates TOC with formatted diff > content 1`] = `
|
||||
"## Files (3)
|
||||
- .github/workflows/test.yml → lines 7-47
|
||||
- index.test.ts → lines 48-110
|
||||
- index.ts → lines 111-132
|
||||
exports[`fetchAndFormatPrDiff > generates accurate TOC line numbers for pullfrog/test-repo#1 > content 1`] = `
|
||||
"## Files (5)
|
||||
- src/format.ts → lines 9-32
|
||||
- src/math.ts → lines 33-55
|
||||
- src/old-module.ts → lines 56-64
|
||||
- src/validate.ts → lines 65-80
|
||||
- test/math.test.ts → lines 81-93
|
||||
|
||||
---
|
||||
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
|
||||
--- a/.github/workflows/test.yml
|
||||
+++ b/.github/workflows/test.yml
|
||||
@@ -0,0 +1,36 @@
|
||||
| | 1 | + | name: Test
|
||||
| | 2 | + |
|
||||
| | 3 | + | on:
|
||||
| | 4 | + | push:
|
||||
| | 5 | + | branches: [main]
|
||||
| | 6 | + | pull_request:
|
||||
| | 7 | + | branches: [main]
|
||||
| | 8 | + |
|
||||
| | 9 | + | jobs:
|
||||
| | 10 | + | test:
|
||||
| | 11 | + | runs-on: ubuntu-latest
|
||||
| | 12 | + |
|
||||
| | 13 | + | strategy:
|
||||
| | 14 | + | matrix:
|
||||
| | 15 | + | node-version: [22.x]
|
||||
| | 16 | + |
|
||||
| | 17 | + | steps:
|
||||
| | 18 | + | - name: Checkout code
|
||||
| | 19 | + | uses: actions/checkout@v4
|
||||
| | 20 | + |
|
||||
| | 21 | + | - name: Setup pnpm
|
||||
| | 22 | + | uses: pnpm/action-setup@v2
|
||||
| | 23 | + | with:
|
||||
| | 24 | + | version: 8
|
||||
| | 25 | + |
|
||||
| | 26 | + | - name: Setup Node.js \${{ matrix.node-version }}
|
||||
| | 27 | + | uses: actions/setup-node@v4
|
||||
| | 28 | + | with:
|
||||
| | 29 | + | node-version: \${{ matrix.node-version }}
|
||||
| | 30 | + | cache: 'pnpm'
|
||||
| | 31 | + |
|
||||
| | 32 | + | - name: Install dependencies
|
||||
| | 33 | + | run: pnpm install
|
||||
| | 34 | + |
|
||||
| | 35 | + | - name: Run tests
|
||||
| | 36 | + | run: pnpm test
|
||||
diff --git a/src/format.ts b/src/format.ts
|
||||
--- a/src/format.ts
|
||||
+++ b/src/format.ts
|
||||
@@ -1,7 +1,17 @@
|
||||
| 1 | | - | export function formatCurrency(amount: number) {
|
||||
| 2 | | - | return \`$\${amount.toFixed(2)}\`;
|
||||
| | 1 | + | export function formatCurrency(amount: number, currency = "USD") {
|
||||
| | 2 | + | return new Intl.NumberFormat("en-US", {
|
||||
| | 3 | + | style: "currency",
|
||||
| | 4 | + | currency,
|
||||
| | 5 | + | }).format(amount);
|
||||
| 3 | 6 | | }
|
||||
| 4 | 7 | |
|
||||
| 5 | 8 | | export function formatPercent(value: number) {
|
||||
| 6 | 9 | | return \`\${(value * 100).toFixed(1)}%\`;
|
||||
| 7 | 10 | | }
|
||||
| | 11 | + |
|
||||
| | 12 | + | export function formatNumber(value: number, decimals = 2) {
|
||||
| | 13 | + | return new Intl.NumberFormat("en-US", {
|
||||
| | 14 | + | minimumFractionDigits: decimals,
|
||||
| | 15 | + | maximumFractionDigits: decimals,
|
||||
| | 16 | + | }).format(value);
|
||||
| | 17 | + | }
|
||||
|
||||
diff --git a/index.test.ts b/index.test.ts
|
||||
--- a/index.test.ts
|
||||
+++ b/index.test.ts
|
||||
@@ -1,5 +1,5 @@
|
||||
| 1 | 1 | | import { describe, it, expect } from 'vitest'
|
||||
| 2 | | - | import { add } from './index.js'
|
||||
| | 2 | + | import { add, multiply, subtract, divide } from './index.js'
|
||||
| 3 | 3 | |
|
||||
| 4 | 4 | | describe('add function', () => {
|
||||
| 5 | 5 | | it('should add two positive numbers correctly', () => {
|
||||
@@ -25,3 +25,51 @@ describe('add function', () => {
|
||||
| 25 | 25 | | expect(add(0.1, 0.2)).toBeCloseTo(0.3)
|
||||
| 26 | 26 | | })
|
||||
| 27 | 27 | | })
|
||||
| | 28 | + |
|
||||
| | 29 | + | describe('multiply function', () => {
|
||||
| | 30 | + | it('should multiply two positive numbers correctly', () => {
|
||||
| | 31 | + | expect(multiply(3, 4)).toBe(12)
|
||||
| | 32 | + | })
|
||||
| | 33 | + |
|
||||
| | 34 | + | it('should multiply negative numbers correctly', () => {
|
||||
| | 35 | + | expect(multiply(-2, 3)).toBe(-6)
|
||||
| | 36 | + | expect(multiply(-2, -3)).toBe(6)
|
||||
| | 37 | + | })
|
||||
| | 38 | + |
|
||||
| | 39 | + | it('should handle zero correctly', () => {
|
||||
| | 40 | + | expect(multiply(5, 0)).toBe(0)
|
||||
| | 41 | + | expect(multiply(0, 5)).toBe(0)
|
||||
| | 42 | + | })
|
||||
| | 43 | + | })
|
||||
| | 44 | + |
|
||||
| | 45 | + | describe('subtract function', () => {
|
||||
| | 46 | + | it('should subtract two positive numbers correctly', () => {
|
||||
| | 47 | + | expect(subtract(10, 3)).toBe(7)
|
||||
| | 48 | + | })
|
||||
| | 49 | + |
|
||||
| | 50 | + | it('should handle negative numbers correctly', () => {
|
||||
| | 51 | + | expect(subtract(5, -3)).toBe(8)
|
||||
| | 52 | + | expect(subtract(-5, 3)).toBe(-8)
|
||||
| | 53 | + | })
|
||||
| | 54 | + |
|
||||
| | 55 | + | it('should handle zero correctly', () => {
|
||||
| | 56 | + | expect(subtract(5, 0)).toBe(5)
|
||||
| | 57 | + | expect(subtract(0, 5)).toBe(-5)
|
||||
| | 58 | + | })
|
||||
| | 59 | + | })
|
||||
| | 60 | + |
|
||||
| | 61 | + | describe('divide function', () => {
|
||||
| | 62 | + | it('should divide two positive numbers correctly', () => {
|
||||
| | 63 | + | expect(divide(10, 2)).toBe(5)
|
||||
| | 64 | + | })
|
||||
| | 65 | + |
|
||||
| | 66 | + | it('should handle negative numbers correctly', () => {
|
||||
| | 67 | + | expect(divide(-10, 2)).toBe(-5)
|
||||
| | 68 | + | expect(divide(10, -2)).toBe(-5)
|
||||
| | 69 | + | })
|
||||
| | 70 | + |
|
||||
| | 71 | + | it('should handle decimal results correctly', () => {
|
||||
| | 72 | + | expect(divide(10, 3)).toBeCloseTo(3.333, 2)
|
||||
| | 73 | + | expect(divide(7, 2)).toBe(3.5)
|
||||
| | 74 | + | })
|
||||
| | 75 | + | })
|
||||
|
||||
diff --git a/index.ts b/index.ts
|
||||
--- a/index.ts
|
||||
+++ b/index.ts
|
||||
@@ -3,11 +3,13 @@ export function add(a: number, b: number) {
|
||||
diff --git a/src/math.ts b/src/math.ts
|
||||
--- a/src/math.ts
|
||||
+++ b/src/math.ts
|
||||
@@ -3,13 +3,16 @@ export function add(a: number, b: number) {
|
||||
| 3 | 3 | | }
|
||||
| 4 | 4 | |
|
||||
| 5 | 5 | | export function multiply(a: number, b: number) {
|
||||
| 6 | | - | // Bug: accidentally adding 1 to the result
|
||||
| 7 | | - | return a * b + 1;
|
||||
| | 6 | + | return a * b;
|
||||
| 8 | 7 | | }
|
||||
| 9 | 8 | |
|
||||
| 10 | 9 | | export function subtract(a: number, b: number) {
|
||||
| 11 | | - | // Bug: accidentally adding instead of subtracting
|
||||
| 12 | | - | return a + b;
|
||||
| | 10 | + | return a - b;
|
||||
| 5 | 5 | | export function subtract(a: number, b: number) {
|
||||
| 6 | | - | return a + b; // bug: should be a - b
|
||||
| | 6 | + | return a - b;
|
||||
| 7 | 7 | | }
|
||||
| 8 | 8 | |
|
||||
| 9 | 9 | | export function multiply(a: number, b: number) {
|
||||
| 10 | | - | return a * b + 1; // bug: off by one
|
||||
| | 10 | + | return a * b;
|
||||
| 11 | 11 | | }
|
||||
| 12 | 12 | |
|
||||
| 13 | 13 | | export function divide(a: number, b: number) {
|
||||
| | 14 | + | if (b === 0) {
|
||||
| | 15 | + | throw new Error("division by zero");
|
||||
| | 16 | + | }
|
||||
| 14 | 17 | | return a / b;
|
||||
| 15 | 18 | | }
|
||||
|
||||
diff --git a/src/old-module.ts b/src/old-module.ts
|
||||
--- a/src/old-module.ts
|
||||
+++ b/src/old-module.ts
|
||||
@@ -1,4 +0,0 @@
|
||||
| 1 | | - | // this module is deprecated and will be removed
|
||||
| 2 | | - | export function legacyHelper() {
|
||||
| 3 | | - | return "old";
|
||||
| 4 | | - | }
|
||||
|
||||
diff --git a/src/validate.ts b/src/validate.ts
|
||||
--- a/src/validate.ts
|
||||
+++ b/src/validate.ts
|
||||
@@ -0,0 +1,11 @@
|
||||
| | 1 | + | export function isPositive(n: number) {
|
||||
| | 2 | + | return n > 0;
|
||||
| | 3 | + | }
|
||||
| | 4 | + |
|
||||
| | 5 | + | export function isInRange(value: number, min: number, max: number) {
|
||||
| | 6 | + | return value >= min && value <= max;
|
||||
| | 7 | + | }
|
||||
| | 8 | + |
|
||||
| | 9 | + | export function isInteger(n: number) {
|
||||
| | 10 | + | return Number.isInteger(n);
|
||||
| | 11 | + | }
|
||||
| | 12 | + |
|
||||
| | 13 | + | export function divide(a: number, b: number) {
|
||||
| | 14 | + | return a / b;
|
||||
| 13 | 15 | | }
|
||||
|
||||
diff --git a/test/math.test.ts b/test/math.test.ts
|
||||
--- a/test/math.test.ts
|
||||
+++ b/test/math.test.ts
|
||||
@@ -17,4 +17,8 @@ describe("math", () => {
|
||||
| 17 | 17 | | it("divides", () => {
|
||||
| 18 | 18 | | expect(divide(10, 2)).toBe(5);
|
||||
| 19 | 19 | | });
|
||||
| | 20 | + |
|
||||
| | 21 | + | it("throws on division by zero", () => {
|
||||
| | 22 | + | expect(() => divide(1, 0)).toThrow("division by zero");
|
||||
| | 23 | + | });
|
||||
| 20 | 24 | | });
|
||||
"
|
||||
`;
|
||||
|
||||
exports[`fetchAndFormatPrDiff > fetches PR files and generates TOC with formatted diff > toc 1`] = `
|
||||
"## Files (3)
|
||||
- .github/workflows/test.yml → lines 7-47
|
||||
- index.test.ts → lines 48-110
|
||||
- index.ts → lines 111-132
|
||||
exports[`fetchAndFormatPrDiff > generates accurate TOC line numbers for pullfrog/test-repo#1 > toc 1`] = `
|
||||
"## Files (5)
|
||||
- src/format.ts → lines 9-32
|
||||
- src/math.ts → lines 33-55
|
||||
- src/old-module.ts → lines 56-64
|
||||
- src/validate.ts → lines 65-80
|
||||
- test/math.test.ts → lines 81-93
|
||||
|
||||
---
|
||||
"
|
||||
|
||||
+106
-29
@@ -1,10 +1,11 @@
|
||||
// changes to bash security (filterEnv, spawnBash) should be reflected in wiki/bash-sandbox.md, wiki/security.md, wiki/landlock.md, and docs/security.mdx
|
||||
import { type ChildProcess, type StdioOptions, spawn } from "node:child_process";
|
||||
// changes to bash security (filterEnv, spawnBash) should be reflected in wiki/security.md and docs/security.mdx
|
||||
import { type ChildProcess, type StdioOptions, spawn, spawnSync } from "node:child_process";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { closeSync, openSync, writeFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { type } from "arktype";
|
||||
import { log } from "../utils/log.ts";
|
||||
import { resolveEnv } from "../utils/secrets.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
|
||||
@@ -16,39 +17,115 @@ export const BashParams = type({
|
||||
"background?": "boolean",
|
||||
});
|
||||
|
||||
// patterns for sensitive env vars
|
||||
const SENSITIVE_PATTERNS = [/_KEY$/i, /_SECRET$/i, /_TOKEN$/i, /_PASSWORD$/i, /_CREDENTIAL$/i];
|
||||
|
||||
function isSensitive(key: string): boolean {
|
||||
return SENSITIVE_PATTERNS.some((p) => p.test(key));
|
||||
}
|
||||
|
||||
/** filter env vars, removing sensitive values */
|
||||
function filterEnv(): Record<string, string> {
|
||||
const filtered: Record<string, string> = {};
|
||||
for (const [key, value] of Object.entries(process.env)) {
|
||||
if (value === undefined) continue;
|
||||
if (isSensitive(key)) continue;
|
||||
filtered[key] = value;
|
||||
}
|
||||
return filtered;
|
||||
}
|
||||
|
||||
type SpawnParams = {
|
||||
command: string;
|
||||
env: Record<string, string>;
|
||||
env: Record<string, string | undefined>;
|
||||
cwd: string;
|
||||
stdio: StdioOptions;
|
||||
};
|
||||
|
||||
export type SandboxMethod = "unshare" | "sudo-unshare" | "none";
|
||||
|
||||
/** cached result of sandbox capability check */
|
||||
let detectedSandboxMethod: SandboxMethod | undefined;
|
||||
|
||||
/** get the current sandbox method (for testing/diagnostics) */
|
||||
export function getSandboxMethod(): SandboxMethod {
|
||||
return detectSandboxMethod();
|
||||
}
|
||||
|
||||
/** detect which sandbox method is available on this system */
|
||||
function detectSandboxMethod(): SandboxMethod {
|
||||
if (detectedSandboxMethod !== undefined) {
|
||||
return detectedSandboxMethod;
|
||||
}
|
||||
|
||||
// only attempt in CI environments - sandbox has overhead and is primarily for untrusted code
|
||||
if (process.env.CI !== "true") {
|
||||
detectedSandboxMethod = "none";
|
||||
log.debug("sandbox disabled (CI !== true)");
|
||||
return "none";
|
||||
}
|
||||
|
||||
// try unprivileged unshare first (works on some systems)
|
||||
try {
|
||||
const result = spawnSync("unshare", ["--pid", "--fork", "--mount-proc", "true"], {
|
||||
timeout: 5000,
|
||||
stdio: "ignore",
|
||||
});
|
||||
if (result.status === 0) {
|
||||
detectedSandboxMethod = "unshare";
|
||||
log.info("PID namespace isolation enabled (unprivileged unshare)");
|
||||
return "unshare";
|
||||
}
|
||||
} catch {
|
||||
// continue to try sudo
|
||||
}
|
||||
|
||||
// try sudo unshare (works on GHA runners)
|
||||
try {
|
||||
const result = spawnSync("sudo", ["unshare", "--pid", "--fork", "--mount-proc", "true"], {
|
||||
timeout: 5000,
|
||||
stdio: "ignore",
|
||||
});
|
||||
if (result.status === 0) {
|
||||
detectedSandboxMethod = "sudo-unshare";
|
||||
log.info("PID namespace isolation enabled (sudo unshare)");
|
||||
return "sudo-unshare";
|
||||
}
|
||||
} catch {
|
||||
// no sandbox available
|
||||
}
|
||||
|
||||
detectedSandboxMethod = "none";
|
||||
log.warning("PID namespace isolation not available - falling back to env filtering only");
|
||||
return "none";
|
||||
}
|
||||
|
||||
function spawnBash(params: SpawnParams): ChildProcess {
|
||||
const spawnOpts = { env: params.env, cwd: params.cwd, stdio: params.stdio, detached: true };
|
||||
// ---- temporarily disable namespace isolation to fix CI ----
|
||||
// use PID namespace isolation in CI to prevent reading /proc/$PPID/environ
|
||||
// const useNamespaceIsolation = process.env.CI === "true";
|
||||
// return useNamespaceIsolation
|
||||
// ? spawn("unshare", ["--pid", "--fork", "--mount-proc", "bash", "-c", params.command], spawnOpts)
|
||||
// : spawn("bash", ["-c", params.command], spawnOpts);
|
||||
const sandboxMethod = detectSandboxMethod();
|
||||
|
||||
if (sandboxMethod === "unshare") {
|
||||
// use PID namespace isolation to prevent reading /proc/$PPID/environ
|
||||
// this creates a new PID namespace where:
|
||||
// 1. the subprocess becomes PID 1 in its namespace
|
||||
// 2. parent PIDs are not visible (PPID = 0)
|
||||
// 3. fresh /proc is mounted showing only sandbox PIDs
|
||||
// combined with resolveEnv("restricted"), this prevents all /proc-based secret theft
|
||||
return spawn(
|
||||
"unshare",
|
||||
["--pid", "--fork", "--mount-proc", "bash", "-c", params.command],
|
||||
spawnOpts
|
||||
);
|
||||
}
|
||||
|
||||
if (sandboxMethod === "sudo-unshare") {
|
||||
// on GHA runners, unprivileged namespaces are blocked but sudo works
|
||||
// pass filtered env via sudo env command since sudo clears environment
|
||||
const envArgs: string[] = [];
|
||||
for (const [k, v] of Object.entries(params.env)) {
|
||||
if (v !== undefined) {
|
||||
envArgs.push(`${k}=${v}`);
|
||||
}
|
||||
}
|
||||
return spawn(
|
||||
"sudo",
|
||||
[
|
||||
"env",
|
||||
...envArgs,
|
||||
"unshare",
|
||||
"--pid",
|
||||
"--fork",
|
||||
"--mount-proc",
|
||||
"bash",
|
||||
"-c",
|
||||
params.command,
|
||||
],
|
||||
{ ...spawnOpts, env: {} } // empty env since we pass via sudo env
|
||||
);
|
||||
}
|
||||
|
||||
return spawn("bash", ["-c", params.command], spawnOpts);
|
||||
}
|
||||
|
||||
@@ -88,9 +165,9 @@ Use this tool to:
|
||||
- Perform git operations`,
|
||||
parameters: BashParams,
|
||||
execute: execute(async (params) => {
|
||||
const timeout = Math.min(params.timeout ?? 120000, 600000);
|
||||
const timeout = Math.min(params.timeout ?? 30000, 120000);
|
||||
const cwd = params.working_directory ?? process.cwd();
|
||||
const env = filterEnv();
|
||||
const env = resolveEnv(ctx.payload.bash === "enabled" ? "inherit" : "restricted");
|
||||
|
||||
if (params.background) {
|
||||
const tempDir = getTempDir();
|
||||
|
||||
+45
-12
@@ -2,8 +2,26 @@ import { Octokit } from "@octokit/rest";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { fetchAndFormatPrDiff } from "./checkout.ts";
|
||||
|
||||
/**
|
||||
* parses TOC entries like "- src/math.ts → lines 7-42" into structured data.
|
||||
*/
|
||||
function parseTocEntries(toc: string) {
|
||||
const entries: Array<{ filename: string; startLine: number; endLine: number }> = [];
|
||||
for (const line of toc.split("\n")) {
|
||||
const match = line.match(/^- (.+) → lines (\d+)-(\d+)$/);
|
||||
if (match) {
|
||||
entries.push({
|
||||
filename: match[1],
|
||||
startLine: parseInt(match[2], 10),
|
||||
endLine: parseInt(match[3], 10),
|
||||
});
|
||||
}
|
||||
}
|
||||
return entries;
|
||||
}
|
||||
|
||||
describe("fetchAndFormatPrDiff", () => {
|
||||
it("fetches PR files and generates TOC with formatted diff", async () => {
|
||||
it("generates accurate TOC line numbers for pullfrog/test-repo#1", async () => {
|
||||
const token = process.env.GH_TOKEN;
|
||||
if (!token) {
|
||||
throw new Error("GH_TOKEN not set in .env");
|
||||
@@ -13,23 +31,38 @@ describe("fetchAndFormatPrDiff", () => {
|
||||
const result = await fetchAndFormatPrDiff({
|
||||
octokit,
|
||||
owner: "pullfrog",
|
||||
repo: "scratch",
|
||||
pullNumber: 49,
|
||||
repo: "test-repo",
|
||||
pullNumber: 1,
|
||||
});
|
||||
|
||||
// verify TOC structure
|
||||
expect(result.toc).toContain("## Files");
|
||||
expect(result.toc).toContain("→ lines");
|
||||
|
||||
// verify content includes TOC at the start
|
||||
expect(result.content.startsWith(result.toc)).toBe(true);
|
||||
|
||||
// verify content includes diff headers
|
||||
expect(result.content).toContain("diff --git");
|
||||
expect(result.content).toContain("---");
|
||||
expect(result.content).toContain("+++");
|
||||
// parse TOC and validate every entry's line numbers against actual content
|
||||
const contentLines = result.content.split("\n");
|
||||
const tocEntries = parseTocEntries(result.toc);
|
||||
expect(tocEntries.length).toBeGreaterThan(0);
|
||||
|
||||
// snapshot the full output
|
||||
for (const entry of tocEntries) {
|
||||
// line numbers are 1-indexed, arrays are 0-indexed
|
||||
const firstLine = contentLines[entry.startLine - 1];
|
||||
expect(firstLine).toBeDefined();
|
||||
// first line of each file section should be the diff header
|
||||
expect(firstLine).toBe(`diff --git a/${entry.filename} b/${entry.filename}`);
|
||||
|
||||
// endLine should be within bounds
|
||||
expect(entry.endLine).toBeLessThanOrEqual(contentLines.length);
|
||||
}
|
||||
|
||||
// verify adjacent files don't overlap and are contiguous
|
||||
for (let i = 1; i < tocEntries.length; i++) {
|
||||
const prev = tocEntries[i - 1];
|
||||
const curr = tocEntries[i];
|
||||
// current file starts right after previous file ends
|
||||
expect(curr.startLine).toBe(prev.endLine + 1);
|
||||
}
|
||||
|
||||
// snapshot the full output for regression detection
|
||||
expect(result.toc).toMatchSnapshot("toc");
|
||||
expect(result.content).toMatchSnapshot("content");
|
||||
});
|
||||
|
||||
+40
-13
@@ -3,8 +3,9 @@ import { join } from "node:path";
|
||||
import type { Octokit, RestEndpointMethodTypes } from "@octokit/rest";
|
||||
import { type } from "arktype";
|
||||
import { log } from "../utils/cli.ts";
|
||||
import { $git } from "../utils/gitAuth.ts";
|
||||
import { $ } from "../utils/shell.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import type { ToolContext, ToolState } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
|
||||
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
|
||||
@@ -136,6 +137,8 @@ export type CheckoutPrResult = {
|
||||
url: string;
|
||||
headRepo: string;
|
||||
diffPath: string;
|
||||
toc: string;
|
||||
instructions: string;
|
||||
};
|
||||
|
||||
type FetchPrDiffParams = {
|
||||
@@ -163,23 +166,28 @@ interface CheckoutPrBranchParams {
|
||||
octokit: Octokit;
|
||||
owner: string;
|
||||
name: string;
|
||||
token: string;
|
||||
gitToken: string;
|
||||
pullNumber: number;
|
||||
toolState: ToolState;
|
||||
// restricted bash mode: disables git hooks to prevent token exfiltration
|
||||
restricted: boolean;
|
||||
}
|
||||
|
||||
interface CheckoutPrBranchResult {
|
||||
prNumber: number;
|
||||
isFork: boolean;
|
||||
forkUrl?: string | undefined; // only set when isFork is true
|
||||
}
|
||||
|
||||
/**
|
||||
* Shared helper to checkout a PR branch and configure fork remotes.
|
||||
* Assumes origin remote is already configured with authentication.
|
||||
* Returns the PR number for caller to set on toolState.
|
||||
* Updates toolState.issueNumber and toolState.pushUrl (for fork PRs).
|
||||
*/
|
||||
export async function checkoutPrBranch(
|
||||
params: CheckoutPrBranchParams
|
||||
): Promise<CheckoutPrBranchResult> {
|
||||
const { octokit, owner, name, token, pullNumber } = params;
|
||||
const { octokit, owner, name, gitToken, pullNumber, toolState, restricted } = params;
|
||||
log.info(`» checking out PR #${pullNumber}...`);
|
||||
|
||||
// fetch PR metadata
|
||||
@@ -212,7 +220,7 @@ export async function checkoutPrBranch(
|
||||
} else {
|
||||
// fetch base branch so origin/<base> exists for diff operations
|
||||
log.debug(`» fetching base branch (${baseBranch})...`);
|
||||
$("git", ["fetch", "--no-tags", "origin", baseBranch]);
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], { token: gitToken, restricted });
|
||||
|
||||
// checkout base branch first to avoid "refusing to fetch into current branch" error
|
||||
// -B creates or resets the branch to match origin/baseBranch
|
||||
@@ -220,7 +228,10 @@ export async function checkoutPrBranch(
|
||||
|
||||
// fetch PR branch using pull/{n}/head refspec (works for both fork and same-repo PRs)
|
||||
log.debug(`» fetching PR #${pullNumber} (${localBranch})...`);
|
||||
$("git", ["fetch", "--no-tags", "origin", `pull/${pullNumber}/head:${localBranch}`]);
|
||||
$git("fetch", ["--no-tags", "origin", `pull/${pullNumber}/head:${localBranch}`], {
|
||||
token: gitToken,
|
||||
restricted,
|
||||
});
|
||||
|
||||
// checkout the branch
|
||||
$("git", ["checkout", localBranch]);
|
||||
@@ -231,7 +242,7 @@ export async function checkoutPrBranch(
|
||||
// fetch if we skipped checkout (already on branch) - otherwise already fetched above
|
||||
if (alreadyOnBranch) {
|
||||
log.debug(`» fetching base branch (${baseBranch})...`);
|
||||
$("git", ["fetch", "--no-tags", "origin", baseBranch]);
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], { token: gitToken, restricted });
|
||||
}
|
||||
|
||||
// configure push remote for this branch
|
||||
@@ -239,7 +250,8 @@ export async function checkoutPrBranch(
|
||||
// fork remotes. This ensures fork PRs can push even when checkout_pr is called after setupGit.
|
||||
if (isFork) {
|
||||
const remoteName = `pr-${pullNumber}`;
|
||||
const forkUrl = `https://x-access-token:${token}@github.com/${headRepo.full_name}.git`;
|
||||
// SECURITY: fork URL without token - auth is injected via GIT_CONFIG_PARAMETERS in $git()
|
||||
const forkUrl = `https://github.com/${headRepo.full_name}.git`;
|
||||
|
||||
// add fork as a named remote (suppress logging to avoid "error: remote already exists" spam)
|
||||
try {
|
||||
@@ -270,7 +282,17 @@ export async function checkoutPrBranch(
|
||||
$("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${headBranch}`]);
|
||||
}
|
||||
|
||||
return { prNumber: pullNumber };
|
||||
// update toolState
|
||||
toolState.issueNumber = pullNumber;
|
||||
if (isFork) {
|
||||
toolState.pushUrl = `https://github.com/${headRepo.full_name}.git`;
|
||||
}
|
||||
|
||||
return {
|
||||
prNumber: pullNumber,
|
||||
isFork,
|
||||
forkUrl: isFork ? `https://github.com/${headRepo.full_name}.git` : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
export function CheckoutPrTool(ctx: ToolContext) {
|
||||
@@ -285,13 +307,12 @@ export function CheckoutPrTool(ctx: ToolContext) {
|
||||
octokit: ctx.octokit,
|
||||
owner: ctx.repo.owner,
|
||||
name: ctx.repo.name,
|
||||
token: ctx.githubInstallationToken,
|
||||
gitToken: ctx.gitToken,
|
||||
pullNumber: pull_number,
|
||||
toolState: ctx.toolState,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
});
|
||||
|
||||
// set prNumber on toolState
|
||||
ctx.toolState.prNumber = result.prNumber;
|
||||
|
||||
// fetch PR metadata to return result
|
||||
const pr = await ctx.octokit.rest.pulls.get({
|
||||
owner: ctx.repo.owner,
|
||||
@@ -334,6 +355,12 @@ export function CheckoutPrTool(ctx: ToolContext) {
|
||||
url: pr.data.html_url,
|
||||
headRepo: headRepo.full_name,
|
||||
diffPath,
|
||||
toc: formatResult.toc,
|
||||
instructions:
|
||||
`the diff file at diffPath contains a table of contents (TOC) at the top listing every changed file with its line range. ` +
|
||||
`use the line ranges to read specific files from the diff instead of reading the entire file. ` +
|
||||
`for example, if the TOC says "src/foo.ts → lines 5-42", read lines 5-42 from diffPath to see that file's changes. ` +
|
||||
`review files selectively based on relevance rather than reading everything sequentially.`,
|
||||
} satisfies CheckoutPrResult;
|
||||
}),
|
||||
});
|
||||
|
||||
+1
-2
@@ -165,8 +165,7 @@ export async function reportProgress(
|
||||
ctx.toolState.lastProgressBody = body;
|
||||
|
||||
const existingCommentId = ctx.toolState.progressCommentId;
|
||||
const issueNumber =
|
||||
ctx.toolState.prNumber ?? ctx.toolState.issueNumber ?? ctx.payload.event.issue_number;
|
||||
const issueNumber = ctx.toolState.issueNumber ?? ctx.payload.event.issue_number;
|
||||
const isPlanMode = ctx.toolState.selectedMode === "Plan";
|
||||
|
||||
// if we already have a progress comment, update it
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
// re-export the normalizeUrl function for testing
|
||||
// note: in a real scenario, we'd export this from git.ts or move to a shared utils file
|
||||
function normalizeUrl(url: string): string {
|
||||
return url.replace(/\.git$/, "").toLowerCase();
|
||||
}
|
||||
|
||||
describe("normalizeUrl", () => {
|
||||
it("removes .git suffix", () => {
|
||||
expect(normalizeUrl("https://github.com/owner/repo.git")).toBe("https://github.com/owner/repo");
|
||||
});
|
||||
|
||||
it("lowercases URL", () => {
|
||||
expect(normalizeUrl("https://github.com/Owner/Repo")).toBe("https://github.com/owner/repo");
|
||||
});
|
||||
|
||||
it("handles URL without .git suffix", () => {
|
||||
expect(normalizeUrl("https://github.com/owner/repo")).toBe("https://github.com/owner/repo");
|
||||
});
|
||||
|
||||
it("handles combined case and .git suffix", () => {
|
||||
expect(normalizeUrl("https://github.com/OWNER/REPO.git")).toBe("https://github.com/owner/repo");
|
||||
});
|
||||
});
|
||||
|
||||
describe("push URL validation", () => {
|
||||
// these tests document the expected behavior
|
||||
// actual integration testing happens via the agent test suite
|
||||
|
||||
it("should block push when actual URL differs from pushUrl", () => {
|
||||
// pushUrl is set by setupGit (base repo) or checkout_pr (fork repo)
|
||||
const pushUrl = "https://github.com/fork-owner/repo.git";
|
||||
const actualUrl = "https://github.com/base-owner/repo.git"; // different repo
|
||||
|
||||
const pushUrlNormalized = normalizeUrl(pushUrl);
|
||||
const actualUrlNormalized = normalizeUrl(actualUrl);
|
||||
|
||||
expect(pushUrlNormalized).not.toBe(actualUrlNormalized);
|
||||
// in real code, this mismatch would throw an error
|
||||
});
|
||||
|
||||
it("should allow push when actual URL matches pushUrl", () => {
|
||||
const pushUrl = "https://github.com/fork-owner/repo.git";
|
||||
const actualUrl = "https://github.com/fork-owner/repo"; // same repo, no .git
|
||||
|
||||
const pushUrlNormalized = normalizeUrl(pushUrl);
|
||||
const actualUrlNormalized = normalizeUrl(actualUrl);
|
||||
|
||||
expect(pushUrlNormalized).toBe(actualUrlNormalized);
|
||||
// in real code, this would allow the push
|
||||
});
|
||||
|
||||
it("should handle case differences in URLs", () => {
|
||||
const pushUrl = "https://github.com/Owner/Repo.git";
|
||||
const actualUrl = "https://github.com/owner/repo";
|
||||
|
||||
const pushUrlNormalized = normalizeUrl(pushUrl);
|
||||
const actualUrlNormalized = normalizeUrl(actualUrl);
|
||||
|
||||
expect(pushUrlNormalized).toBe(actualUrlNormalized);
|
||||
});
|
||||
});
|
||||
+208
-147
@@ -1,137 +1,83 @@
|
||||
import { type } from "arktype";
|
||||
import { log } from "../utils/cli.ts";
|
||||
import { containsSecrets } from "../utils/secrets.ts";
|
||||
import { $git } from "../utils/gitAuth.ts";
|
||||
import { $ } from "../utils/shell.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
|
||||
export function CreateBranchTool(ctx: ToolContext) {
|
||||
const defaultBranch = ctx.repo.data.default_branch || "main";
|
||||
type PushDestination = {
|
||||
remoteName: string;
|
||||
remoteBranch: string;
|
||||
url: string;
|
||||
};
|
||||
|
||||
const CreateBranch = type({
|
||||
branchName: type.string.describe(
|
||||
"The name of the branch to create (e.g., 'pullfrog/123-fix-bug')"
|
||||
),
|
||||
baseBranch: type.string
|
||||
.describe(`The base branch to create from (defaults to '${defaultBranch}')`)
|
||||
.default(defaultBranch),
|
||||
});
|
||||
/**
|
||||
* get where git would actually push this branch.
|
||||
* uses git's native @{push} resolution, falls back to origin if unset.
|
||||
*
|
||||
* for branches created via checkout_pr: uses configured pushRemote/merge
|
||||
* for new branches (git checkout -b): falls back to origin/<branch>
|
||||
*/
|
||||
function getPushDestination(branch: string): PushDestination {
|
||||
// try git's @{push} resolution first (works for checkout_pr branches)
|
||||
try {
|
||||
const pushRef = $(
|
||||
"git",
|
||||
["rev-parse", "--abbrev-ref", "--symbolic-full-name", `${branch}@{push}`],
|
||||
{ log: false }
|
||||
).trim();
|
||||
|
||||
return tool({
|
||||
name: "create_branch",
|
||||
description:
|
||||
"Create a new git branch from the specified base branch. The branch will be created locally and pushed to the remote repository.",
|
||||
parameters: CreateBranch,
|
||||
execute: execute(async ({ branchName, baseBranch }) => {
|
||||
// baseBranch should always be defined due to default, but TypeScript needs help
|
||||
const resolvedBaseBranch = baseBranch || ctx.repo.data.default_branch || "main";
|
||||
// pushRef is like "origin/main" or "pr-123/feature/foo"
|
||||
// parse carefully to handle branch names with slashes
|
||||
const slashIndex = pushRef.indexOf("/");
|
||||
if (slashIndex === -1) {
|
||||
throw new Error(`unexpected push ref format: ${pushRef}`);
|
||||
}
|
||||
const remoteName = pushRef.slice(0, slashIndex);
|
||||
const remoteBranch = pushRef.slice(slashIndex + 1);
|
||||
|
||||
// validate branch name for secrets
|
||||
if (containsSecrets(branchName)) {
|
||||
throw new Error(
|
||||
"Branch creation blocked: secrets detected in branch name. " +
|
||||
"Please remove any sensitive information (API keys, tokens, passwords) before creating a branch."
|
||||
);
|
||||
}
|
||||
// get the actual URL git would push to (handles remote.X.pushurl)
|
||||
const url = $("git", ["remote", "get-url", "--push", remoteName], { log: false }).trim();
|
||||
|
||||
log.debug(`Creating branch ${branchName} from ${resolvedBaseBranch}`);
|
||||
|
||||
// fetch base branch to ensure we're up to date
|
||||
$("git", ["fetch", "origin", resolvedBaseBranch, "--depth=1"]);
|
||||
|
||||
// checkout base branch, ensuring it matches the remote version
|
||||
// -B creates or resets the branch to match origin/baseBranch
|
||||
$("git", ["checkout", "-B", resolvedBaseBranch, `origin/${resolvedBaseBranch}`]);
|
||||
|
||||
// create and checkout new branch
|
||||
$("git", ["checkout", "-b", branchName]);
|
||||
|
||||
// push branch to remote (set upstream)
|
||||
$("git", ["push", "-u", "origin", branchName]);
|
||||
|
||||
log.debug(`Successfully created and pushed branch ${branchName}`);
|
||||
|
||||
return {
|
||||
success: true,
|
||||
branchName,
|
||||
baseBranch: resolvedBaseBranch,
|
||||
message: `Branch ${branchName} created from ${resolvedBaseBranch} and pushed to remote`,
|
||||
};
|
||||
}),
|
||||
});
|
||||
return { remoteName, remoteBranch, url };
|
||||
} catch {
|
||||
// @{push} not configured - branch was created locally without checkout_pr
|
||||
// fall back to origin with the same branch name
|
||||
log.debug(`no push tracking for ${branch}, falling back to origin/${branch}`);
|
||||
const url = $("git", ["remote", "get-url", "--push", "origin"], { log: false }).trim();
|
||||
return { remoteName: "origin", remoteBranch: branch, url };
|
||||
}
|
||||
}
|
||||
|
||||
export const CommitFiles = type({
|
||||
message: type.string.describe("The commit message"),
|
||||
files: type.string
|
||||
.array()
|
||||
.describe(
|
||||
"Array of file paths to commit (relative to repo root). If empty, commits all staged changes."
|
||||
),
|
||||
});
|
||||
/**
|
||||
* normalize URL for comparison (handle .git suffix, case)
|
||||
*/
|
||||
function normalizeUrl(url: string): string {
|
||||
return url.replace(/\.git$/, "").toLowerCase();
|
||||
}
|
||||
|
||||
export function CommitFilesTool(_ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "commit_files",
|
||||
description:
|
||||
"Stage and commit files with a commit message. If files array is empty, commits all staged changes. The commit will be attributed to the correct bot account.",
|
||||
parameters: CommitFiles,
|
||||
execute: execute(async ({ message, files }) => {
|
||||
// validate commit message for secrets
|
||||
if (containsSecrets(message)) {
|
||||
throw new Error(
|
||||
"Commit blocked: secrets detected in commit message. " +
|
||||
"Please remove any sensitive information (API keys, tokens, passwords) before committing."
|
||||
);
|
||||
}
|
||||
type ValidatePushParams = {
|
||||
branch: string;
|
||||
pushUrl: string;
|
||||
};
|
||||
|
||||
// validate files for secrets if provided
|
||||
if (files.length > 0) {
|
||||
for (const file of files) {
|
||||
try {
|
||||
// try to read file content - if it exists, check for secrets
|
||||
const content = $("cat", [file], { log: false });
|
||||
if (containsSecrets(content)) {
|
||||
throw new Error(
|
||||
`Commit blocked: secrets detected in file ${file}. ` +
|
||||
"Please remove any sensitive information (API keys, tokens, passwords) before committing."
|
||||
);
|
||||
}
|
||||
} catch (error) {
|
||||
// if error is about secrets, re-throw it
|
||||
if (error instanceof Error && error.message.includes("Commit blocked")) {
|
||||
throw error;
|
||||
}
|
||||
// if file doesn't exist (cat fails), that's ok - it will be created by git add
|
||||
// other errors are also ok - git add will handle them
|
||||
}
|
||||
}
|
||||
}
|
||||
/**
|
||||
* validate that the push destination matches expected URL.
|
||||
* pushUrl is set by setupGit (base repo) and updated by checkout_pr (fork repo).
|
||||
*/
|
||||
function validatePushDestination(params: ValidatePushParams): PushDestination {
|
||||
const dest = getPushDestination(params.branch);
|
||||
|
||||
const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false });
|
||||
log.debug(`Committing files on branch ${currentBranch}`);
|
||||
if (normalizeUrl(dest.url) !== normalizeUrl(params.pushUrl)) {
|
||||
throw new Error(
|
||||
`Push blocked: destination does not match expected repository.\n` +
|
||||
`Expected: ${params.pushUrl}\n` +
|
||||
`Actual: ${dest.url}\n` +
|
||||
`Git configuration may have been tampered with.`
|
||||
);
|
||||
}
|
||||
|
||||
// stage files if provided, otherwise stage all changes
|
||||
if (files.length > 0) {
|
||||
$("git", ["add", ...files]);
|
||||
} else {
|
||||
$("git", ["add", "."]);
|
||||
}
|
||||
|
||||
// commit with message
|
||||
$("git", ["commit", "-m", message]);
|
||||
|
||||
const commitSha = $("git", ["rev-parse", "HEAD"], { log: false });
|
||||
log.debug(`Successfully committed: ${commitSha.substring(0, 7)}`);
|
||||
|
||||
return {
|
||||
success: true,
|
||||
commitSha,
|
||||
branch: currentBranch,
|
||||
message: `Committed ${files.length > 0 ? files.length + " file(s)" : "all changes"} with message: ${message}`,
|
||||
};
|
||||
}),
|
||||
});
|
||||
return dest;
|
||||
}
|
||||
|
||||
export const PushBranch = type({
|
||||
@@ -143,61 +89,176 @@ export const PushBranch = type({
|
||||
|
||||
export function PushBranchTool(ctx: ToolContext) {
|
||||
const defaultBranch = ctx.repo.data.default_branch || "main";
|
||||
const pushPermission = ctx.payload.push;
|
||||
|
||||
return tool({
|
||||
name: "push_branch",
|
||||
description:
|
||||
"Push the current branch (or specified branch) to the remote repository. Git automatically determines the correct remote based on branch config (set by checkout_pr for fork PRs). Never force push unless explicitly requested. Pushes to the default branch are blocked.",
|
||||
"Push the current branch (or specified branch) to the remote repository. Git automatically determines the correct remote based on branch config (set by checkout_pr for fork PRs). Never force push unless explicitly requested. Pushes to the default branch are blocked in restricted mode.",
|
||||
parameters: PushBranch,
|
||||
execute: execute(async ({ branchName, force }) => {
|
||||
// permission check
|
||||
if (pushPermission === "disabled") {
|
||||
throw new Error("Push is disabled. This repository is configured for read-only access.");
|
||||
}
|
||||
|
||||
const branch = branchName || $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false });
|
||||
|
||||
// check if branch has a configured pushRemote
|
||||
let remote = "origin";
|
||||
try {
|
||||
remote = $("git", ["config", `branch.${branch}.pushRemote`], { log: false }).trim();
|
||||
} catch {
|
||||
// no configured pushRemote, default to origin
|
||||
// validate push destination matches expected URL
|
||||
const pushUrl = ctx.toolState.pushUrl;
|
||||
if (!pushUrl) {
|
||||
throw new Error("pushUrl not set - setupGit must run before push_branch");
|
||||
}
|
||||
const pushDest = validatePushDestination({ branch, pushUrl });
|
||||
|
||||
// check if branch has a configured merge ref (remote branch name may differ from local)
|
||||
let remoteBranch = branch;
|
||||
try {
|
||||
const mergeRef = $("git", ["config", `branch.${branch}.merge`], { log: false }).trim();
|
||||
// merge ref is like "refs/heads/main", extract the branch name
|
||||
remoteBranch = mergeRef.replace("refs/heads/", "");
|
||||
} catch {
|
||||
// no configured merge ref, use local branch name
|
||||
}
|
||||
|
||||
// block pushes to default branch
|
||||
if (remoteBranch === defaultBranch) {
|
||||
// block pushes to default branch in restricted mode
|
||||
if (pushPermission === "restricted" && pushDest.remoteBranch === defaultBranch) {
|
||||
throw new Error(
|
||||
`Push blocked: cannot push directly to default branch '${remoteBranch}'. ` +
|
||||
`Push blocked: cannot push directly to default branch '${pushDest.remoteBranch}'. ` +
|
||||
`Create a feature branch and open a PR instead.`
|
||||
);
|
||||
}
|
||||
|
||||
// use refspec when local and remote branch names differ
|
||||
const refspec = branch === remoteBranch ? branch : `${branch}:${remoteBranch}`;
|
||||
const args = force
|
||||
? ["push", "--force", "-u", remote, refspec]
|
||||
: ["push", "-u", remote, refspec];
|
||||
const refspec =
|
||||
branch === pushDest.remoteBranch ? branch : `${branch}:${pushDest.remoteBranch}`;
|
||||
const pushArgs = force
|
||||
? ["--force", "-u", pushDest.remoteName, refspec]
|
||||
: ["-u", pushDest.remoteName, refspec];
|
||||
|
||||
log.debug(`pushing ${branch} to ${remote}/${remoteBranch}`);
|
||||
log.debug(`pushing ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch}`);
|
||||
if (force) {
|
||||
log.warning(`force pushing - this will overwrite remote history`);
|
||||
}
|
||||
$("git", args);
|
||||
$git("push", pushArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
});
|
||||
|
||||
return {
|
||||
success: true,
|
||||
branch,
|
||||
remoteBranch,
|
||||
remote,
|
||||
remoteBranch: pushDest.remoteBranch,
|
||||
remote: pushDest.remoteName,
|
||||
force,
|
||||
message: `successfully pushed ${branch} to ${remote}/${remoteBranch}`,
|
||||
message: `successfully pushed ${branch} to ${pushDest.remoteName}/${pushDest.remoteBranch}`,
|
||||
};
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
// commands that require authentication - redirect to dedicated tools
|
||||
const AUTH_REQUIRED_REDIRECT: Record<string, string> = {
|
||||
push: "Use push_branch tool instead.",
|
||||
fetch: "Use git_fetch tool instead.",
|
||||
pull: "Use git_fetch + git merge instead.",
|
||||
clone: "Repository already cloned. Use checkout_pr for PR branches.",
|
||||
};
|
||||
|
||||
const Git = type({
|
||||
subcommand: type.string.describe("Git subcommand (e.g., 'status', 'log', 'diff')"),
|
||||
args: type.string.array().describe("Additional arguments for the git command").optional(),
|
||||
});
|
||||
|
||||
export function GitTool(_ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "git",
|
||||
description:
|
||||
"Run git commands. For push/fetch/pull, use the dedicated MCP tools instead (push_branch, git_fetch).",
|
||||
parameters: Git,
|
||||
execute: execute(async (params) => {
|
||||
const subcommand = params.subcommand;
|
||||
const args = params.args ?? [];
|
||||
|
||||
const redirect = AUTH_REQUIRED_REDIRECT[subcommand];
|
||||
if (redirect) {
|
||||
throw new Error(`git ${subcommand} requires authentication. ${redirect}`);
|
||||
}
|
||||
|
||||
const output = $("git", [subcommand, ...args]);
|
||||
return { success: true, output };
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
const GitFetch = type({
|
||||
ref: type.string.describe("Ref to fetch: branch name, tag, or 'pull/N/head' for PRs"),
|
||||
depth: type.number.describe("Fetch depth (for shallow clones)").optional(),
|
||||
});
|
||||
|
||||
export function GitFetchTool(ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "git_fetch",
|
||||
description: "Fetch refs from remote repository. Use this instead of git fetch directly.",
|
||||
parameters: GitFetch,
|
||||
execute: execute(async (params) => {
|
||||
const fetchArgs = ["--no-tags", "origin", params.ref];
|
||||
if (params.depth !== undefined) {
|
||||
fetchArgs.push(`--depth=${params.depth}`);
|
||||
}
|
||||
$git("fetch", fetchArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
});
|
||||
return { success: true, ref: params.ref };
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
const DeleteBranch = type({
|
||||
branchName: type.string.describe("Remote branch to delete"),
|
||||
});
|
||||
|
||||
export function DeleteBranchTool(ctx: ToolContext) {
|
||||
const pushPermission = ctx.payload.push;
|
||||
|
||||
return tool({
|
||||
name: "delete_branch",
|
||||
description: "Delete a remote branch. Requires push: enabled permission.",
|
||||
parameters: DeleteBranch,
|
||||
execute: execute(async (params) => {
|
||||
if (pushPermission !== "enabled") {
|
||||
throw new Error(
|
||||
"Branch deletion requires push: enabled permission. " +
|
||||
"Current mode only allows pushing to non-protected branches."
|
||||
);
|
||||
}
|
||||
|
||||
$git("push", ["origin", "--delete", params.branchName], {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
});
|
||||
return { success: true, deleted: params.branchName };
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
const PushTags = type({
|
||||
tag: type.string.describe("Tag name to push"),
|
||||
force: type.boolean.describe("Force push the tag").default(false),
|
||||
});
|
||||
|
||||
export function PushTagsTool(ctx: ToolContext) {
|
||||
const pushPermission = ctx.payload.push;
|
||||
|
||||
return tool({
|
||||
name: "push_tags",
|
||||
description: "Push a tag to remote. Requires push: enabled permission.",
|
||||
parameters: PushTags,
|
||||
execute: execute(async (params) => {
|
||||
if (pushPermission !== "enabled") {
|
||||
throw new Error(
|
||||
"Tag pushing requires push: enabled permission. " +
|
||||
"Current mode only allows pushing branches."
|
||||
);
|
||||
}
|
||||
|
||||
const pushArgs = [...(params.force ? ["-f"] : []), "origin", `refs/tags/${params.tag}`];
|
||||
$git("push", pushArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
});
|
||||
return { success: true, tag: params.tag };
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
import { type } from "arktype";
|
||||
import { buildPullfrogFooter, stripExistingFooter } from "../utils/buildPullfrogFooter.ts";
|
||||
import { log } from "../utils/cli.ts";
|
||||
import { containsSecrets } from "../utils/secrets.ts";
|
||||
import { $ } from "../utils/shell.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
@@ -34,25 +33,6 @@ export function CreatePullRequestTool(ctx: ToolContext) {
|
||||
const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false });
|
||||
log.debug(`Current branch: ${currentBranch}`);
|
||||
|
||||
// validate PR title and body for secrets
|
||||
if (containsSecrets(title) || containsSecrets(body)) {
|
||||
throw new Error(
|
||||
"PR creation blocked: secrets detected in PR title or body. " +
|
||||
"Please remove any sensitive information (API keys, tokens, passwords) before creating a PR."
|
||||
);
|
||||
}
|
||||
|
||||
// validate all changes that would be in the PR (from base to HEAD)
|
||||
// FORK PR NOTE: origin/<base> is fetched by setupGit, so this works for both fork and same-repo PRs
|
||||
// use two-dot (..) not three-dot (...) for reliable diffs with shallow clones
|
||||
const diff = $("git", ["diff", `origin/${base}..HEAD`], { log: false });
|
||||
if (containsSecrets(diff)) {
|
||||
throw new Error(
|
||||
"PR creation blocked: secrets detected in changes. " +
|
||||
"Please remove any sensitive information (API keys, tokens, passwords) before creating a PR."
|
||||
);
|
||||
}
|
||||
|
||||
const bodyWithFooter = buildPrBodyWithFooter(ctx, body);
|
||||
|
||||
const result = await ctx.octokit.rest.pulls.create({
|
||||
|
||||
+9
-9
@@ -59,8 +59,8 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
|
||||
`{ path: 'src/api.ts', start_line: 42, line: 44, suggestion: ' const result = await fetch(url);\\n if (!result.ok) {\\n log.error(result.status);\\n throw new Error("request failed");\\n }' }`,
|
||||
parameters: CreatePullRequestReview,
|
||||
execute: execute(async ({ pull_number, body, commit_id, comments = [] }) => {
|
||||
// set PR context
|
||||
ctx.toolState.prNumber = pull_number;
|
||||
// set issue context (PRs are issues)
|
||||
ctx.toolState.issueNumber = pull_number;
|
||||
|
||||
// compose the request
|
||||
const params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"] = {
|
||||
@@ -281,8 +281,8 @@ export function StartReviewTool(ctx: ToolContext) {
|
||||
}
|
||||
}
|
||||
|
||||
// set PR context and review state
|
||||
ctx.toolState.prNumber = pull_number;
|
||||
// set issue context (PRs are issues) and review state
|
||||
ctx.toolState.issueNumber = pull_number;
|
||||
ctx.toolState.review = {
|
||||
nodeId: reviewNodeId,
|
||||
id: reviewId,
|
||||
@@ -400,19 +400,19 @@ export function SubmitReviewTool(ctx: ToolContext) {
|
||||
if (!ctx.toolState.review) {
|
||||
throw new Error("No review session started. Call start_review first.");
|
||||
}
|
||||
if (ctx.toolState.prNumber === undefined) {
|
||||
if (ctx.toolState.issueNumber === undefined) {
|
||||
throw new Error("No PR context. Call checkout_pr or start_review first.");
|
||||
}
|
||||
|
||||
const reviewId = ctx.toolState.review.id;
|
||||
log.debug(
|
||||
`submitting review: id=${reviewId}, nodeId=${ctx.toolState.review.nodeId}, prNumber=${ctx.toolState.prNumber}`
|
||||
`submitting review: id=${reviewId}, nodeId=${ctx.toolState.review.nodeId}, issueNumber=${ctx.toolState.issueNumber}`
|
||||
);
|
||||
|
||||
// build quick links footer
|
||||
const apiUrl = process.env.API_URL || "https://pullfrog.com";
|
||||
const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${ctx.toolState.prNumber}?action=fix&review_id=${reviewId}`;
|
||||
const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${ctx.toolState.prNumber}?action=fix-approved&review_id=${reviewId}`;
|
||||
const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${ctx.toolState.issueNumber}?action=fix&review_id=${reviewId}`;
|
||||
const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${ctx.toolState.issueNumber}?action=fix-approved&review_id=${reviewId}`;
|
||||
|
||||
const footer = buildPullfrogFooter({
|
||||
workflowRun: { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId },
|
||||
@@ -425,7 +425,7 @@ export function SubmitReviewTool(ctx: ToolContext) {
|
||||
const result = await ctx.octokit.rest.pulls.submitReview({
|
||||
owner: ctx.repo.owner,
|
||||
repo: ctx.repo.name,
|
||||
pull_number: ctx.toolState.prNumber,
|
||||
pull_number: ctx.toolState.issueNumber,
|
||||
review_id: reviewId,
|
||||
event: "COMMENT",
|
||||
body: bodyWithFooter,
|
||||
|
||||
+11
-5
@@ -1,7 +1,7 @@
|
||||
import "./arkConfig.ts";
|
||||
import { createServer } from "node:net";
|
||||
// this must be imported first
|
||||
import { FastMCP, type Tool } from "fastmcp";
|
||||
import { createServer } from "node:net";
|
||||
import type { Agent } from "../agents/index.ts";
|
||||
import { ghPullfrogMcpName } from "../external.ts";
|
||||
import type { Mode } from "../modes.ts";
|
||||
@@ -16,7 +16,10 @@ export type BackgroundProcess = {
|
||||
};
|
||||
|
||||
export interface ToolState {
|
||||
prNumber?: number;
|
||||
// where we're allowed to push - base repo initially, fork URL for fork PRs
|
||||
// set by setupGit, updated by checkout_pr. always set before push validation.
|
||||
pushUrl?: string;
|
||||
// issue or PR number (same number space in GitHub)
|
||||
issueNumber?: number;
|
||||
selectedMode?: string;
|
||||
backgroundProcesses: Map<string, BackgroundProcess>;
|
||||
@@ -60,6 +63,7 @@ export interface ToolContext {
|
||||
payload: ResolvedPayload;
|
||||
octokit: OctokitWithPlugins;
|
||||
githubInstallationToken: string;
|
||||
gitToken: string;
|
||||
apiToken: string;
|
||||
agent: Agent;
|
||||
modes: Mode[];
|
||||
@@ -84,7 +88,7 @@ import {
|
||||
AwaitDependencyInstallationTool,
|
||||
StartDependencyInstallationTool,
|
||||
} from "./dependencies.ts";
|
||||
import { CommitFilesTool, CreateBranchTool, PushBranchTool } from "./git.ts";
|
||||
import { DeleteBranchTool, GitFetchTool, GitTool, PushBranchTool, PushTagsTool } from "./git.ts";
|
||||
import { IssueTool } from "./issue.ts";
|
||||
import { GetIssueCommentsTool } from "./issueComments.ts";
|
||||
import { GetIssueEventsTool } from "./issueEvents.ts";
|
||||
@@ -156,9 +160,11 @@ function buildTools(ctx: ToolContext): Tool<any, any>[] {
|
||||
ListPullRequestReviewsTool(ctx),
|
||||
GetCheckSuiteLogsTool(ctx),
|
||||
AddLabelsTool(ctx),
|
||||
CreateBranchTool(ctx),
|
||||
CommitFilesTool(ctx),
|
||||
PushBranchTool(ctx),
|
||||
GitTool(ctx),
|
||||
GitFetchTool(ctx),
|
||||
DeleteBranchTool(ctx),
|
||||
PushTagsTool(ctx),
|
||||
UploadFileTool(ctx),
|
||||
SetOutputTool(ctx),
|
||||
];
|
||||
|
||||
Reference in New Issue
Block a user