From 59b23ecb2df5635e13108b19cc7e613e73d3d8af Mon Sep 17 00:00:00 2001 From: Negrescu Alexandru Date: Sat, 31 Oct 2026 19:37:02 +0200 Subject: [PATCH 1/3] fix(pstackpolicy): close merge bypasses and scope push refspecs to their command gh reads flags between a command group and its verb, so `gh pr -R o/r merge` passed every rule anchored on `gh pr merge`. Every gh rule now goes through one builder that allows flags on both sides of the group. The API rules missed `/merges`, the `mergeBranch` and `enablePullRequestAutoMerge` mutations, and a PATCH that moves a trunk ref. `gt merge` was missing from the Graphite rule. The merge-when-ready rule sat behind the plain merge rule, so its reason never fired. It now runs first. The push and local-history scans read every word after `git push` to the end of the text, so `git push origin feature && git rev-parse HEAD` was refused as a push of HEAD. They now read one simple command at a time. The cut skips quotes, backticks, brackets and redirections so it fails closed. A heredoc body line counts as a command. A separator glued to a word also ends it, which closes `git push origin main; echo done`. Tests look the handler up under `tool_call`, assert the rule each blocked command fires, and add one case per bypass and false positive. The DENY_REASONS export and its tautological test are gone. --- plugins/pstack/README.md | 16 +- .../pstack/extensions/pstackpolicy/index.js | 162 ++++++---- .../extensions/pstackpolicy/index.test.js | 276 +++++++++++------- 3 files changed, 288 insertions(+), 166 deletions(-) diff --git a/plugins/pstack/README.md b/plugins/pstack/README.md index cce4d92..41f2420 100644 --- a/plugins/pstack/README.md +++ b/plugins/pstack/README.md @@ -38,12 +38,16 @@ Then `/poteto-mode on`, or `alt+shift+t`, or `omp -p --poteto '...'` for headles `pstackpolicy` loads with the plugin and needs no configuration. It registers `tool_call` on `bash` and `eval`, and refuses the forge mutations the playbooks only ask for in prose: `gh pr merge`, merge and close through `gh api`, `gh pr close|reopen|delete`, `gh pr edit --base`, `gh pr review`, -merge-when-ready, `gh repo delete`, `gh release create|delete|upload`, -`gh secret|variable set|delete`, `gh workflow run --ref `, the same forge calls made with -`curl`, and `gt submit|rebase|restack`. Locally it refuses `git update-ref`, `git branch -D`, -`git reset --hard` against a remote, and `git reflog expire --expire=now --all`. Any `git push` -whose refspec lands on `main`, `master`, `trunk`, `develop`, or `release` is refused, forced or not; -a force push to a feature branch stays allowed, because that is the port's own rebase flow. +merge-when-ready, a trunk ref moved through `gh api`, `gh repo delete`, +`gh release create|delete|upload`, `gh secret|variable set|delete`, `gh workflow run --ref `, +the same forge calls made with `curl`, and `gt submit|rebase|restack|merge`. A `gh` flag such as +`-R o/r` may sit before or after the command group. Locally it refuses `git update-ref`, +`git branch -D`, `git reset --hard` against a remote, and `git reflog expire --expire=now --all`. +Any `git push` whose refspec lands on `main`, `master`, `trunk`, `develop`, or `release` is +refused, forced or not; a force push to a feature branch stays allowed, because that is the port's +own rebase flow. A push's refspecs end at its own command, so a later `&&`, `||`, `;`, `|` or +newline does not lend it arguments. Each line of a heredoc body is checked as a command, because a +shell may run it. To land, set `PSTACK_LANDING_GRANT=1` in the environment omp starts from. It is an environment variable and not a flag or a slash command because a flag an agent can pass is not a grant you made. diff --git a/plugins/pstack/extensions/pstackpolicy/index.js b/plugins/pstack/extensions/pstackpolicy/index.js index 74867d9..187ef8d 100644 --- a/plugins/pstack/extensions/pstackpolicy/index.js +++ b/plugins/pstack/extensions/pstackpolicy/index.js @@ -31,26 +31,26 @@ // PSTACK_LANDING_GRANT=1 allow the forge mutations below for this process // // Three shapes of rule, because no one shape covers the ground. The `gh` rules are regexes, so -// they have to tolerate global flags between the binary and the subcommand -- `gh --repo o/r pr -// merge` is the documented way to target a non-default repo and the port uses it constantly, -// and anchoring on `gh pr` alone let every one of them through. The `git push` and -// `git branch -D` rules are token scans, because "does this push land on trunk" is a question -// about refspecs, not a substring: `git push origin HEAD:main` moves trunk with no force flag -// at all, which a force-token regex cannot see, and a naive trunk-word lookahead then blocks -// `feature/main-menu`. And both of those token scans have to read past git's global options, -// because `git -C push ...` and `/usr/bin/git push ...` are the same push; a scan keyed on -// two adjacent raw tokens of exactly `git` then `push` was proven to let +// they have to tolerate flags on both sides of the command group. `gh --repo o/r pr merge` is +// the documented way to target a non-default repo and the port uses it constantly, gh reads +// `gh pr -R o/r merge` as the same merge, and anchoring on `gh pr merge` let both through. The +// `git push` and `git branch -D` rules are token scans, because "does this push land on trunk" +// is a question about refspecs, not a substring: `git push origin HEAD:main` moves trunk with no +// force flag at all, which a force-token regex cannot see, and a naive trunk-word lookahead then +// blocks `feature/main-menu`. And both of those token scans have to read past git's global +// options, because `git -C push ...` and `/usr/bin/git push ...` are the same push; a scan +// keyed on two adjacent raw tokens of exactly `git` then `push` was proven to let // `git -C /tmp/bypass/work push -q origin HEAD:trunk` create a branch on a real remote. const GRANT = "PSTACK_LANDING_GRANT"; const TRUNK = new Set(["main", "master", "trunk", "develop", "release"]); -// Regexes cannot read a Set, and two rules have to name trunk inside a pattern. +// Regexes cannot read a Set, and some rules have to name trunk inside a pattern. const TRUNK_ALT = [...TRUNK].join("|"); -// gh accepts global flags before the subcommand. Each rule permits any number of them, including -// `--repo owner/name`, which is the form the port actually uses. +// gh accepts flags before the command group and between the group and its verb. Each rule +// permits any number of them, including `--repo owner/name`, which is the form the port uses. const GH_FLAGS = String.raw`(?:\s+(?:-[A-Za-z]|[-]{2}[A-Za-z][\w-]*)(?:[= ]\S+)?)*`; // Any run of characters that is not a newline, except a backslash immediately followed by one. A @@ -58,6 +58,15 @@ const GH_FLAGS = String.raw`(?:\s+(?:-[A-Za-z]|[-]{2}[A-Za-z][\w-]*)(?:[= ]\S+)? // the flag it gates sits on the far side of the continuation. const CONT = String.raw`(?:[^\n\\]|\\\n)*`; +const gh = (group, verbs) => + String.raw`\bgh\b${GH_FLAGS}\s+(?:${group})${GH_FLAGS}\s+(?:${verbs})\b`; + +const GH_API = String.raw`\bgh\b${GH_FLAGS}\s+api\b`; + +// Every API spelling of a merge: the pull request and branch merge endpoints, and the GraphQL +// mutations that merge or arm a merge. +const API_MERGE = String.raw`\/merges?\b|mergePullRequest|mergeBranch|enablePullRequestAutoMerge`; + // Two normalisations, because these rules match text and the text is not always a shell line. // // Quotes are syntax once the shell or the host language has read them, so `gh "pr" merge 12` is @@ -73,57 +82,54 @@ const normalize = (command) => .replace(/[()[\]{},]/g, " "); const FORBIDDEN = [ + // Ahead of the plain merge rule, which matches every `--auto` merge as well. [ - "merging a pull request", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+pr\s+merge\b`), + "armoring merge-when-ready", + new RegExp( + String.raw`${gh("pr", "merge")}${CONT}--auto\b|${GH_API}${CONT}enablePullRequestAutoMerge` + ), ], + ["merging a pull request", new RegExp(gh("pr", "merge"))], + ["merging through the API", new RegExp(String.raw`${GH_API}${CONT}(?:${API_MERGE})`)], [ - "merging a pull request through the API", + "closing, reopening, or deleting a pull request, issue, or thread", new RegExp( - String.raw`\bgh\b${GH_FLAGS}\s+api\b${CONT}(?:\/pulls\/\d+\/merge|mergePullRequest|\/merge\b)` + String.raw`${gh("pr|issue", "close|reopen|delete")}|${GH_API}${CONT}(?:-X\s*DELETE[^\n]*\/(issues|pulls|git\/refs)|state\s*=\s*closed|closePullRequest|closeIssue)` ), ], [ - "closing, reopening, or deleting a pull request, issue, or thread", + "moving a trunk branch through the API", + // The ref is a path, so the method decides. gh api sends GET unless told otherwise, and a + // PATCH to a feature ref is the API's feature-branch force push, which stays allowed. new RegExp( - String.raw`\bgh\b${GH_FLAGS}\s+(pr|issue)\s+(close|reopen|delete)\b|\bgh\b${GH_FLAGS}\s+api\b${CONT}(?:-X\s*DELETE[^\n]*\/(issues|pulls|git\/refs)|state\s*=\s*closed|closePullRequest|closeIssue)` + String.raw`${GH_API}(?=${CONT}\s(?:-X\s*|--method[= ]\s*)(?!GET\b)[A-Za-z])${CONT}\bgit\/refs\/heads\/(?:${TRUNK_ALT})(?![\w/-])` ), ], [ "retargeting a pull request base, which rewrites the whole stack under it", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+pr\s+edit\b${CONT}--base\b`), + new RegExp(String.raw`${gh("pr", "edit")}${CONT}--base\b`), ], [ "posting a review or resolving a review thread", new RegExp( - String.raw`\bgh\b${GH_FLAGS}\s+pr\s+review\b|\bgh\b${GH_FLAGS}\s+api\b${CONT}(?:-X\s*POST[^\n]*(?:\/reviews|pulls\/\d+\/(comments|reviews))|resolveReviewThread|addPullRequestReview)` + String.raw`${gh("pr", "review")}|${GH_API}${CONT}(?:-X\s*POST[^\n]*(?:\/reviews|pulls\/\d+\/(comments|reviews))|resolveReviewThread|addPullRequestReview)` ), ], - [ - "armoring merge-when-ready", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+pr\s+merge\b${CONT}--(?:auto|auto-merge)\b`), - ], // The enumeration below used to stop at the verbs the port happened to use, so every other // forge mutation gh ships was a clean pass: `gh repo delete` destroys the repository the whole // stack lives in, and a release publish or a secret write is not undoable from a later step. - [ - "deleting the repository", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+repo\s+(delete|remove|rm)\b`), - ], - [ - "publishing, replacing, or deleting a release", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+release\s+(create|delete|upload)\b`), - ], + ["deleting the repository", new RegExp(gh("repo", "delete|remove|rm"))], + ["publishing, replacing, or deleting a release", new RegExp(gh("release", "create|delete|upload"))], [ "writing a secret or a deployment variable on the forge", - new RegExp(String.raw`\bgh\b${GH_FLAGS}\s+(secret|variable)\s+(set|delete)\b`), + new RegExp(gh("secret|variable", "set|delete")), ], [ "running a workflow against a trunk branch", // `--ref` is the trigger, and the ref is what decides: `gh workflow run ci.yml --ref // feature/x` is the port's own CI path and stays allowed. new RegExp( - String.raw`\bgh\b${GH_FLAGS}\s+workflow\s+run\b${CONT}(?:--ref|-r)(?![A-Za-z0-9-])[= ]+["']?(?:${TRUNK_ALT})(?![\w-])` + String.raw`${gh("workflow", "run")}${CONT}(?:--ref|-r)(?![A-Za-z0-9-])[= ]+["']?(?:${TRUNK_ALT})(?![\w-])` ), ], [ @@ -131,21 +137,32 @@ const FORBIDDEN = [ // Same request as the `gh api` rules above, and equally available with the token already in // the environment. The trigger is the method or a request body, since curl POSTs by default. new RegExp( - String.raw`\bcurl\b${CONT}(?:(?:-X|--request)\s*[A-Za-z]+|--data[\s=])${CONT}(?:\/pulls\/\d+\/merge|\/merge\b|mergePullRequest|state\s*=\s*closed|\/git\/refs)`, + String.raw`\bcurl\b${CONT}(?:(?:-X|--request)\s*[A-Za-z]+|--data[\s=])${CONT}(?:${API_MERGE}|state\s*=\s*closed|\/git\/refs)`, "i" ), ], [ "merging through Graphite", - new RegExp(String.raw`\bgt\s+(submit|rebase|restack)\b`), + new RegExp(String.raw`\bgt\s+(submit|rebase|restack|merge)\b`), ], ]; +// `;`, `|` and `&` end a shell word and the simple command it is in, except inside a redirection: +// the `&` of `2>&1`, `<&` or `&>`, and the `|` of `>|`. +function separates(text, i) { + const ch = text[i]; + if (ch === ";") return true; + if (ch === "|") return text[i - 1] !== ">"; + if (ch === "&") return !/[<>]/.test(text[i - 1] ?? "") && text[i + 1] !== ">"; + return false; +} + /** - * Shell-shaped tokeniser: whitespace separates, single and double quotes group, a backslash - * escapes the next character and a backslash-newline disappears. Quotes are stripped, because - * `'HEAD:main'` and `HEAD:main` are one refspec to git and the strip is what lets the refspec - * rules see the colon. + * Shell-shaped tokeniser: whitespace and the separators above end a word, single and double + * quotes group, a backslash escapes the next character and a backslash-newline disappears. Quotes + * are stripped, because `'HEAD:main'` and `HEAD:main` are one refspec to git and the strip is what + * lets the refspec rules see the colon. A glued `;` is not part of the word, so + * `(git push origin main; true)` pushes `main`, not `main;`. */ function shellTokens(command) { const out = []; @@ -182,7 +199,7 @@ function shellTokens(command) { open = true; continue; } - if (/\s/.test(ch)) { + if (/\s/.test(ch) || separates(command, i)) { if (open) out.push(current); current = ""; open = false; @@ -195,6 +212,47 @@ function shellTokens(command) { return out; } +/** + * The simple commands in a command line, cut where the shell ends one: `&&`, `||`, `;`, `|`, `&` + * and newline. A push's refspecs end with its own command, so `git push origin feature && git + * rev-parse HEAD` is a feature push and a read, not a push of HEAD. + * + * Every miss leaves text joined, which fails closed. Nothing inside quotes, backticks or brackets + * is cut, so the `;` in `$(echo; echo main)` and the newlines of a list literal stay with the + * push they feed, and the `&` of `2>&1` or `&>` is a redirection, not a separator. A heredoc body + * gets no exception: each line is scanned as its own command, because `bash < { + out.push(command.slice(start, at)); + start = at + 1; + }; + for (let i = 0; i < command.length; i += 1) { + const ch = command[i]; + if (quote === "'") { + if (ch === "'") quote = null; + } else if (ch === "\\") { + i += 1; + } else if (quote) { + if (ch === quote) quote = null; + } else if (ch === "'" || ch === '"' || ch === "`") { + quote = ch; + } else if ("([{".includes(ch)) { + depth += 1; + } else if (")]}".includes(ch)) { + depth = Math.max(0, depth - 1); + } else if (depth === 0 && (ch === "\n" || separates(command, i))) { + cut(i); + } + } + out.push(command.slice(start)); + return out; +} + // git, or git under any path: an absolute binary is the same program with the same global // options in front of the subcommand. const GIT_BASENAME = /(?:^|[/\\])git(?:\.exe)?$/; @@ -232,14 +290,16 @@ function gitSubcommand(tokens, start) { return -1; } -/** Every `git ` in the command, with its arguments and no global options. */ +/** Every `git ` in the command, with the arguments its own simple command gives it. */ function eachGitCommand(command, visit) { - const tokens = shellTokens(command); - for (let i = 0; i < tokens.length; i += 1) { - if (!GIT_BASENAME.test(tokens[i])) continue; - const at = gitSubcommand(tokens, i); - if (at < 0) continue; - visit(tokens[at], tokens.slice(at + 1)); + for (const simple of simpleCommands(command)) { + const tokens = shellTokens(normalize(simple)); + for (let i = 0; i < tokens.length; i += 1) { + if (!GIT_BASENAME.test(tokens[i])) continue; + const at = gitSubcommand(tokens, i); + if (at < 0) continue; + visit(tokens[at], tokens.slice(at + 1)); + } } } @@ -339,8 +399,6 @@ function localDestruction(command) { return why; } -export const DENY_REASONS = FORBIDDEN.map(([why]) => why); - function granted() { const raw = process.env[GRANT]; // Anything but an explicit affirmative is absent. A stray "0" or "false" must not read as @@ -370,9 +428,9 @@ export default function pstackpolicy(pi) { const text = normalize(command); const why = - pushOffence(text) ?? + pushOffence(command) ?? (FORBIDDEN.find(([, pattern]) => pattern.test(text)) ?? null)?.[0] ?? - localDestruction(text); + localDestruction(command); if (why) { return { diff --git a/plugins/pstack/extensions/pstackpolicy/index.test.js b/plugins/pstack/extensions/pstackpolicy/index.test.js index 54774f7..2fd9d78 100644 --- a/plugins/pstack/extensions/pstackpolicy/index.test.js +++ b/plugins/pstack/extensions/pstackpolicy/index.test.js @@ -1,10 +1,17 @@ import { describe, expect, test } from "bun:test"; -import policy, { DENY_REASONS } from "./index.js"; +import policy from "./index.js"; // The handler is the whole backstop, so it is tested against commands that must be refused and // commands that must not be. A rule set that blocks nothing is indistinguishable from no // backstop at all until something tries to merge. Every call awaits: the handler is async, and // comparing a Promise against undefined would pass for the wrong reason. +// +// omp vetoes only from `tool_call`, so the handler is looked up under that name. A fake host that +// ignored the event name passed every test with the backstop registered on `tool_result`. +const handlers = new Map(); +policy({ on: (event, fn) => handlers.set(event, fn) }); +const toolCall = handlers.get("tool_call"); + async function call(toolName, input, env) { const saved = {}; for (const [k, v] of Object.entries(env ?? {})) { @@ -12,10 +19,8 @@ async function call(toolName, input, env) { if (v === undefined) delete process.env[k]; else process.env[k] = v; } - let handler; - policy({ on: (_event, fn) => { handler = fn; } }); try { - return await handler({ toolName, input }); + return await toolCall({ toolName, input }); } finally { for (const [k, v] of Object.entries(saved)) { if (v === undefined) delete process.env[k]; @@ -28,87 +33,135 @@ async function run(command, env) { return call("bash", { command }, env); } +// Each command with a fragment of the reason its own rule gives, so a command refused by the +// wrong rule fails and a rule another rule shadows cannot hide. const MUST_BLOCK = [ // A refspec this port cannot read is not a refspec. `git push origin HEAD` and a bare // `git push` land on whatever the checkout is on, which from a trunk checkout is trunk, and a // quoted refspec hid the destination from the colon split. - "git push origin HEAD", - "git push", - "git push origin", - "git push --force origin", - "git push origin 'HEAD:main'", - "gh pr edit 12 \\\n --base main", - "gh pr merge 12 \\\n --auto", - "gh --repo o/r pr merge 12 \\\n --squash", + ["git push origin HEAD", "current branch"], + ["git push", "explicit refspec"], + ["git push origin", "explicit refspec"], + ["git push --force origin", "explicit refspec"], + ["git push origin 'HEAD:main'", "pushing to trunk"], + ["gh pr edit 12 \\\n --base main", "retargeting"], + ["gh pr merge 12 \\\n --auto", "merge-when-ready"], + ["gh --repo o/r pr merge 12 \\\n --squash", "merging a pull request"], // The forms that defeated the first version of this file. - "gh --repo o/r pr merge 12 --squash", - "gh --repo o/r pr edit 12 --base main", - "gh --repo o/r pr review 12 --approve", - "gh --repo o/r issue close 5", - "git push origin HEAD:main", - "git push origin main:main", - "git push origin HEAD:refs/heads/main", - "git push origin +main", - "git push origin :feature/x", - "git push --mirror git@github.com:o/r.git", - "git update-ref refs/heads/main abc123", - 'gh api graphql -f query=\'mutation{mergePullRequest(input:{pullRequestId:"x"}){clientMutationId}}\'', - "gh api -X PATCH repos/o/r/pulls/12 -f state=closed", - "gh api -X DELETE /repos/o/r/git/refs/heads/main", + ["gh --repo o/r pr merge 12 --squash", "merging a pull request"], + ["gh --repo o/r pr edit 12 --base main", "retargeting"], + ["gh --repo o/r pr review 12 --approve", "posting a review"], + ["gh --repo o/r issue close 5", "closing, reopening"], + ["git push origin HEAD:main", "pushing to trunk"], + ["git push origin main:main", "pushing to trunk"], + ["git push origin HEAD:refs/heads/main", "pushing to trunk"], + ["git push origin +main", "pushing to trunk"], + ["git push origin :feature/x", "deleting a branch on the forge"], + ["git push --mirror git@github.com:o/r.git", "mirroring"], + ["git update-ref refs/heads/main abc123", "local history"], + [ + 'gh api graphql -f query=\'mutation{mergePullRequest(input:{pullRequestId:"x"}){clientMutationId}}\'', + "merging through the API", + ], + ["gh api -X PATCH repos/o/r/pulls/12 -f state=closed", "closing, reopening"], + ["gh api -X DELETE /repos/o/r/git/refs/heads/main", "closing, reopening"], // The forms it always caught. - "gh pr merge 12 --squash --delete-branch", - "gh pr merge 12 --squash --auto", - "gh pr edit 12 --base main", - "gh pr close 12", - "gh pr review 12 --approve", - "git push --force origin main", - "git push origin main --force-with-lease", - "git push origin --delete feature/x", - "git branch -D feature/x", - "gh api repos/o/r/pulls/12/merge -X POST", - "gt submit --stack", - "if [ -z \"$x\" ]; then gh pr merge 3; fi", - "cd /tmp && gh pr merge 9 --squash", + ["gh pr merge 12 --squash --delete-branch", "merging a pull request"], + ["gh pr merge 12 --squash --auto", "merge-when-ready"], + ["gh pr edit 12 --base main", "retargeting"], + ["gh pr close 12", "closing, reopening"], + ["gh pr review 12 --approve", "posting a review"], + ["git push --force origin main", "rewriting history"], + ["git push origin main --force-with-lease", "rewriting history"], + ["git push origin --delete feature/x", "deleting a branch on the forge"], + ["git branch -D feature/x", "local history"], + ["gh api repos/o/r/pulls/12/merge -X PUT", "merging through the API"], + ["gt submit --stack", "merging through Graphite"], + ['if [ -z "$x" ]; then gh pr merge 3; fi', "merging a pull request"], + ["cd /tmp && gh pr merge 9 --squash", "merging a pull request"], // git's global options between the binary and the subcommand. A push scan keyed on two // adjacent raw tokens of exactly `git` then `push` found none of these; the first of them was // executed against a real bare remote and created refs/heads/trunk there. - "git -C /tmp/bypass/work push -q origin HEAD:trunk", - "git -c user.name=bot push origin HEAD:main", - "git --git-dir=/tmp/repo/.git push origin main", - "git --work-tree=/tmp/wt push origin HEAD:refs/heads/main", - "git --namespace=refs/ns push origin master", - "git --exec-path=/usr/lib/git-core push origin develop", - "/usr/bin/git push origin HEAD:main", - "/usr/bin/git -C /tmp/work push origin HEAD", - "git --git-dir=/tmp/repo/.git push", - "git -c core.hooksPath=/dev/null push origin release", - "git --bare push origin HEAD:trunk", - "git -C /tmp/repo push -q origin :feature/x", - "git -C /tmp/repo push -q --mirror origin", + ["git -C /tmp/bypass/work push -q origin HEAD:trunk", "pushing to trunk"], + ["git -c user.name=bot push origin HEAD:main", "pushing to trunk"], + ["git --git-dir=/tmp/repo/.git push origin main", "pushing to trunk"], + ["git --work-tree=/tmp/wt push origin HEAD:refs/heads/main", "pushing to trunk"], + ["git --namespace=refs/ns push origin master", "pushing to trunk"], + ["git --exec-path=/usr/lib/git-core push origin develop", "pushing to trunk"], + ["/usr/bin/git push origin HEAD:main", "pushing to trunk"], + ["/usr/bin/git -C /tmp/work push origin HEAD", "current branch"], + ["git --git-dir=/tmp/repo/.git push", "explicit refspec"], + ["git -c core.hooksPath=/dev/null push origin release", "pushing to trunk"], + ["git --bare push origin HEAD:trunk", "pushing to trunk"], + ["git -C /tmp/repo push -q origin :feature/x", "deleting a branch on the forge"], + ["git -C /tmp/repo push -q --mirror origin", "mirroring"], // The same gap defeated the local-history rules. - "git -C /tmp/repo branch -D feat", - "git --git-dir=/tmp/repo/.git update-ref refs/heads/main abc123", - "git -C /tmp/repo reflog expire --expire=now --all", - "git --work-tree=/tmp/wt reset --hard origin/main", + ["git -C /tmp/repo branch -D feat", "local history"], + ["git --git-dir=/tmp/repo/.git update-ref refs/heads/main abc123", "local history"], + ["git -C /tmp/repo reflog expire --expire=now --all", "local history"], + ["git --work-tree=/tmp/wt reset --hard origin/main", "local history"], // The gh verbs the enumeration never listed. - "gh repo delete o/r --yes", - "gh --repo o/r repo delete --yes", - "gh release create v1.2.3", - "gh release delete v1.2.3", - "gh secret set DEPLOY_TOKEN", - "gh variable set FLAG", - "gh workflow run deploy.yml --ref main", - "gh workflow run deploy.yml --ref=trunk", + ["gh repo delete o/r --yes", "deleting the repository"], + ["gh --repo o/r repo delete --yes", "deleting the repository"], + ["gh release create v1.2.3", "a release"], + ["gh release delete v1.2.3", "a release"], + ["gh secret set DEPLOY_TOKEN", "a secret"], + ["gh variable set FLAG", "a secret"], + ["gh workflow run deploy.yml --ref main", "running a workflow"], + ["gh workflow run deploy.yml --ref=trunk", "running a workflow"], // A quoted subcommand is the same token to gh once the shell strips it. - 'gh "pr" merge 12', - "gh 'pr' merge 12 --squash", - 'gh "repo" delete o/r --yes', - 'gh "workflow" run deploy.yml --ref main', + ['gh "pr" merge 12', "merging a pull request"], + ["gh 'pr' merge 12 --squash", "merging a pull request"], + ['gh "repo" delete o/r --yes', "deleting the repository"], + ['gh "workflow" run deploy.yml --ref main', "running a workflow"], // curl reaches the same endpoints with the token already in the environment. - "curl -sS -X PUT https://api.github.com/repos/o/r/pulls/1/merge -d '{}'", - "curl --request POST -H 'Authorization: token x' https://api.github.com/repos/o/r/pulls/1/merge", - "curl -X PATCH https://api.github.com/repos/o/r/pulls/1 -d state=closed", - "curl -X DELETE https://api.github.com/repos/o/r/git/refs/heads/main", + ["curl -sS -X PUT https://api.github.com/repos/o/r/pulls/1/merge -d '{}'", "with curl"], + [ + "curl --request POST -H 'Authorization: token x' https://api.github.com/repos/o/r/pulls/1/merge", + "with curl", + ], + ["curl -X PATCH https://api.github.com/repos/o/r/pulls/1 -d state=closed", "with curl"], + ["curl -X DELETE https://api.github.com/repos/o/r/git/refs/heads/main", "with curl"], + // gh reads flags between the command group and its verb as well, so each of these is the + // same call as its `gh -R o/r ` spelling above. + ["gh pr -R o/r merge 12", "merging a pull request"], + ["gh pr --repo o/r merge 12 --squash", "merging a pull request"], + ["gh pr -R o/r merge 12 --auto", "merge-when-ready"], + ["gh pr -R o/r edit 12 --base main", "retargeting"], + ["gh pr --repo=o/r close 12", "closing, reopening"], + ["gh pr -R o/r review 12 --approve", "posting a review"], + ["gh issue -R o/r close 5", "closing, reopening"], + ["gh release -R o/r create v1.2.3", "a release"], + ["gh secret -R o/r set DEPLOY_TOKEN", "a secret"], + ["gh workflow -R o/r run deploy.yml --ref main", "running a workflow"], + // Merges the API rule never named, and trunk moved without a push. + ["gh api repos/o/r/merges -f base=main -f head=feature/x", "merging through the API"], + [ + 'gh api graphql -f query=\'mutation{mergeBranch(input:{repositoryId:"r",base:"main",head:"x"}){clientMutationId}}\'', + "merging through the API", + ], + [ + 'gh api graphql -f query=\'mutation{enablePullRequestAutoMerge(input:{pullRequestId:"x"}){clientMutationId}}\'', + "merge-when-ready", + ], + ["gh api -X PATCH repos/o/r/git/refs/heads/main -f sha=abc -F force=true", "moving a trunk branch"], + ["gh api --method=PATCH repos/o/r/git/refs/heads/release -f sha=abc", "moving a trunk branch"], + [ + `curl -X POST https://api.github.com/repos/o/r/merges -d '{"base":"main","head":"x"}'`, + "with curl", + ], + ["gt merge", "merging through Graphite"], + // Cutting a command line into simple commands must not open a hole. A redirection is not a + // separator, a substitution belongs to the push it feeds, and a heredoc body can be run. + ["git push origin 2>&1 HEAD:main", "pushing to trunk"], + ["git push 2>&1", "explicit refspec"], + ["git push origin $(echo feature; echo main)", "pushing to trunk"], + ["bash <<'EOF'\ngit push origin HEAD:main\nEOF", "pushing to trunk"], + ["cat > land.sh <<'EOF'\ngh pr merge 12 --squash\nEOF", "merging a pull request"], + // A separator glued to a refspec is not part of it. Both of these were allowed as a push of + // `main;`, which is not trunk. + ["git push origin main; echo done", "pushing to trunk"], + ["(git push origin main; true)", "pushing to trunk"], ]; const MUST_PASS = [ @@ -121,7 +174,7 @@ const MUST_PASS = [ "bun run test", "git push --force-with-lease origin main-menu", "git push --force origin feature/main-menu", - "git commit -m \"merge main into feature\"", + 'git commit -m "merge main into feature"', "gh pr list --search main", "git log main..HEAD", // The global-option tolerance must not cost the feature-branch push. @@ -145,26 +198,37 @@ const MUST_PASS = [ "curl -X GET https://api.github.com/repos/o/r", "gh api repos/o/r/pulls/12", "gh api -X GET repos/o/r/branches/main", + // Flags after the command group must not turn a read into a mutation. + "gh pr -R o/r view 12", + "gh pr -R o/r list --search merge", + // Reading a trunk ref, or moving a feature ref, is not moving trunk. + "gh api repos/o/r/git/refs/heads/main", + "gh api -X GET repos/o/r/git/refs/heads/main --jq .object.sha", + "gh api -X PATCH repos/o/r/git/refs/heads/feature/main-menu -f sha=abc -F force=true", + // A later command's arguments are not the push's refspecs. + "git push origin feature && git rev-parse HEAD", + "git push origin feature || git rev-parse HEAD", + "git push origin feature; git checkout main", + "git push origin feature | grep main", + "git push origin feature\ngit log -1 HEAD", + "git push origin feature 2>&1 && git rev-parse HEAD", + "cat > notes.md <<'EOF'\nPush with git push origin feature.\nEOF\ngit rev-parse HEAD", ]; describe("pstackpolicy", () => { - test("refuses every forge mutation when no grant is set", async () => { - for (const command of MUST_BLOCK) { - const result = await run(command, { PSTACK_LANDING_GRANT: undefined }); - expect(result?.block, `should block: ${command}`).toBe(true); - expect(result?.reason, `should explain: ${command}`).toContain("PSTACK_LANDING_GRANT"); - } + test.each(MUST_BLOCK)("refuses %p as %p without a grant", async (command, rule) => { + const result = await run(command, { PSTACK_LANDING_GRANT: undefined }); + expect(result?.block).toBe(true); + expect(result?.reason).toContain(rule); + expect(result?.reason).toContain("PSTACK_LANDING_GRANT"); }); - test("allows reads and ordinary pushes to pass through", async () => { - for (const command of MUST_PASS) { - expect(await run(command, { PSTACK_LANDING_GRANT: undefined }), `should pass: ${command}`) - .toBeUndefined(); - } + test.each(MUST_PASS)("allows %p", async (command) => { + expect(await run(command, { PSTACK_LANDING_GRANT: undefined })).toBeUndefined(); }); test("an operator grant lifts every rule", async () => { - for (const command of MUST_BLOCK) { + for (const [command] of MUST_BLOCK) { expect(await run(command, { PSTACK_LANDING_GRANT: "1" }), `granted: ${command}`) .toBeUndefined(); } @@ -182,36 +246,32 @@ describe("pstackpolicy", () => { test("an eval cell is gated the same way a bash call is", async () => { // omp://approval-mode.md: eval declares the exec tier and can spawn a shell, so a rule that // only read bash's `command` field was not a rule about the command. - for (const code of [ - 'await Bun.$`gh pr merge 12 --squash`', - 'subprocess.run(["git", "push", "origin", "HEAD:main"])', - 'await tool.bash({ command: "gh repo delete o/r --yes" })', + for (const [code, rule] of [ + ["await Bun.$`gh pr merge 12 --squash`", "merging a pull request"], + ['subprocess.run(["git", "push", "origin", "HEAD:main"])', "pushing to trunk"], + ['await tool.bash({ command: "gh repo delete o/r --yes" })', "deleting the repository"], + // A list literal broken over lines is still one command. + ['subprocess.run([\n "git", "push", "origin", "feature",\n "HEAD:main",\n])', "pushing to trunk"], ]) { - const result = await call("eval", { language: "js", code }, { PSTACK_LANDING_GRANT: undefined }); + const result = await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined }); expect(result?.block, `should block: ${code}`).toBe(true); - expect(result?.reason, `should explain: ${code}`).toContain("PSTACK_LANDING_GRANT"); + expect(result?.reason, `should name the rule: ${code}`).toContain(rule); } }); test("an eval cell with no forge mutation still passes", async () => { - const ok = await call( - "eval", - { language: "py", code: 'print("git push origin feature/x")' }, - { PSTACK_LANDING_GRANT: undefined } - ); - expect(ok?.block).toBeUndefined(); + for (const code of [ + 'print("git push origin feature/x")', + 'subprocess.run(["git", "push", "origin", "feature"])\nsubprocess.run(["git", "rev-parse", "HEAD"])', + ]) { + const ok = await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined }); + expect(ok, `should pass: ${code}`).toBeUndefined(); + } }); test("ignores tools that carry no command", async () => { - let handler; - policy({ on: (_e, fn) => { handler = fn; } }); - expect(await handler({ toolName: "read", input: { command: "gh pr merge 12" } })).toBeUndefined(); - expect(await handler({ toolName: "bash", input: {} })).toBeUndefined(); - expect(await handler({ toolName: "eval", input: {} })).toBeUndefined(); - }); - - test("every rule carries a reason the port can assert on", () => { - expect(DENY_REASONS.length).toBeGreaterThan(0); - for (const why of DENY_REASONS) expect(typeof why).toBe("string"); + expect(await toolCall({ toolName: "read", input: { command: "gh pr merge 12" } })).toBeUndefined(); + expect(await toolCall({ toolName: "bash", input: {} })).toBeUndefined(); + expect(await toolCall({ toolName: "eval", input: {} })).toBeUndefined(); }); -}); \ No newline at end of file +}); From 84e53e9324a7c805406ff75601591e35b95a3966 Mon Sep 17 00:00:00 2001 From: Negrescu Alexandru Date: Sat, 31 Oct 2026 19:37:02 +0200 Subject: [PATCH 2/3] test(potetomode): cover /poteto-mode task forwarding Removing the forwarding passed both the unit tests and smoke. The fake host now records sendUserMessage and exposes isIdle. `/poteto-mode ` must pin and send the trimmed text, use `deliverAs: "followUp"` while a turn runs, and `status` must send nothing and keep the pin. --- .../extensions/potetomode/index.test.js | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/plugins/pstack/extensions/potetomode/index.test.js b/plugins/pstack/extensions/potetomode/index.test.js index 3be6bf0..21038bb 100644 --- a/plugins/pstack/extensions/potetomode/index.test.js +++ b/plugins/pstack/extensions/potetomode/index.test.js @@ -4,10 +4,12 @@ import potetomode from "./index.js"; const entry = (pinned) => ({ type: "custom", customType: "poteto-mode", data: { pinned } }); function host(flag = true) { const events = new Map(); - let command, shortcut, entries = [], status = ""; + let command, shortcut, entries = [], status = "", idle = true; + const sent = []; const ctx = { sessionManager: { getBranch: () => entries }, ui: { theme: { fg: (_color, text) => text }, setStatus: (_key, value) => { status = value; }, notify() {} }, + isIdle: () => idle, }; potetomode({ registerCommand: (_name, value) => { command = value.handler; }, @@ -16,7 +18,7 @@ function host(flag = true) { on: (name, handler) => events.set(name, handler), getFlag: () => flag, appendEntry: (_name, data) => entries.push(entry(data.pinned)), - sendUserMessage() {}, + sendUserMessage: (text, options) => sent.push({ text, options }), }); return { start: async (branch = []) => { entries = branch; await events.get("session_start")({}, ctx); }, @@ -25,6 +27,8 @@ function host(flag = true) { command: (args) => command(args, ctx), shortcut: () => shortcut(ctx), status: () => status, + busy: () => { idle = false; }, + sent, }; } @@ -93,3 +97,31 @@ test("switch, branch and tree navigation reload the pin from the branch they lan expect((await session.before()).systemPrompt[1]).toContain("poteto-mode is PINNED"); } }); + +test("/poteto-mode pins the session and starts the task", async () => { + const session = host(false); + await session.start(); + await session.command(" Fix the bug in parser.ts "); + expect(session.sent).toHaveLength(1); + expect(session.sent[0].text).toBe("Fix the bug in parser.ts"); + expect(session.sent[0].options?.deliverAs).toBeUndefined(); + expect(await session.before()).toBeDefined(); +}); + +test("/poteto-mode queues the task as a follow-up while a turn is running", async () => { + const session = host(false); + await session.start(); + session.busy(); + await session.command("fix the bug"); + expect(session.sent).toEqual([{ text: "fix the bug", options: { deliverAs: "followUp" } }]); +}); + +test("/poteto-mode status sends nothing and leaves the pin where it was", async () => { + for (const flag of [true, false]) { + const session = host(flag); + await session.start(); + await session.command("status"); + expect(session.sent).toEqual([]); + expect((await session.before()) !== undefined).toBe(flag); + } +}); From 3ea983e50d9b899af52b30e0c77455bcc0e62d7a Mon Sep 17 00:00:00 2001 From: Negrescu Alexandru Date: Sat, 31 Oct 2026 21:06:06 +0200 Subject: [PATCH 3/3] fix(pstackpolicy): refuse unreadable refspecs and GraphQL ref moves Closes the four gaps PR #35 left open. - GraphQL updateRef, updateRefs and deleteRef through gh api or curl are refused. They name the ref by node ID or inside an input list, so no text rule can tell trunk from a feature branch. - A push destination with `$`, a backtick substitution or a `*` pattern resolves only when it runs and may be trunk. It is refused with a "may be trunk" reason, like HEAD. A refspec that is certainly trunk still wins the reason. normalize spells a backtick substitution as `$` and its command, so a push inside backticks is scanned as a push. - A comment runs from a word-initial `#` to its newline whatever quotes it holds, and is dropped without cutting the command it sits in. - A subshell or brace group at command start separates its commands like unparenthesised text. A substitution or list literal still keeps its text together. - A redirection and its target are not refspecs, so `> "$LOG"` does not read as a run-time refspec. - The eval cases are one test each. --- plugins/pstack/README.md | 16 ++- .../pstack/extensions/pstackpolicy/index.js | 111 ++++++++++++------ .../extensions/pstackpolicy/index.test.js | 85 ++++++++++---- 3 files changed, 149 insertions(+), 63 deletions(-) diff --git a/plugins/pstack/README.md b/plugins/pstack/README.md index 41f2420..6231807 100644 --- a/plugins/pstack/README.md +++ b/plugins/pstack/README.md @@ -38,16 +38,20 @@ Then `/poteto-mode on`, or `alt+shift+t`, or `omp -p --poteto '...'` for headles `pstackpolicy` loads with the plugin and needs no configuration. It registers `tool_call` on `bash` and `eval`, and refuses the forge mutations the playbooks only ask for in prose: `gh pr merge`, merge and close through `gh api`, `gh pr close|reopen|delete`, `gh pr edit --base`, `gh pr review`, -merge-when-ready, a trunk ref moved through `gh api`, `gh repo delete`, -`gh release create|delete|upload`, `gh secret|variable set|delete`, `gh workflow run --ref `, -the same forge calls made with `curl`, and `gt submit|rebase|restack|merge`. A `gh` flag such as +merge-when-ready, a trunk ref moved through `gh api`, the GraphQL `updateRef`, `updateRefs` and +`deleteRef` mutations, `gh repo delete`, `gh release create|delete|upload`, +`gh secret|variable set|delete`, `gh workflow run --ref `, the same forge calls made with +`curl`, and `gt submit|rebase|restack|merge`. A `gh` flag such as `-R o/r` may sit before or after the command group. Locally it refuses `git update-ref`, `git branch -D`, `git reset --hard` against a remote, and `git reflog expire --expire=now --all`. Any `git push` whose refspec lands on `main`, `master`, `trunk`, `develop`, or `release` is refused, forced or not; a force push to a feature branch stays allowed, because that is the port's -own rebase flow. A push's refspecs end at its own command, so a later `&&`, `||`, `;`, `|` or -newline does not lend it arguments. Each line of a heredoc body is checked as a command, because a -shell may run it. +own rebase flow. A refspec the scan cannot read may be trunk and is refused too: `HEAD`, no refspec +at all, a `$VAR`, `${VAR}`, `$(...)` or backtick substitution, and a `*` pattern. A push's +refspecs end at its own command, so a later `&&`, `||`, `;`, `|` or newline does not lend it +arguments, inside a subshell or brace group as well. A comment ends at its newline whatever quotes +it holds, and a redirection target is not a refspec. Each line of a heredoc body is checked as a +command, because a shell may run it. To land, set `PSTACK_LANDING_GRANT=1` in the environment omp starts from. It is an environment variable and not a flag or a slash command because a flag an agent can pass is not a grant you made. diff --git a/plugins/pstack/extensions/pstackpolicy/index.js b/plugins/pstack/extensions/pstackpolicy/index.js index 187ef8d..0b11889 100644 --- a/plugins/pstack/extensions/pstackpolicy/index.js +++ b/plugins/pstack/extensions/pstackpolicy/index.js @@ -67,7 +67,7 @@ const GH_API = String.raw`\bgh\b${GH_FLAGS}\s+api\b`; // mutations that merge or arm a merge. const API_MERGE = String.raw`\/merges?\b|mergePullRequest|mergeBranch|enablePullRequestAutoMerge`; -// Two normalisations, because these rules match text and the text is not always a shell line. +// Three normalisations, because these rules match text and the text is not always a shell line. // // Quotes are syntax once the shell or the host language has read them, so `gh "pr" merge 12` is // `gh pr merge 12` and `'HEAD:main'` is one refspec. And inside an `eval` cell the command is @@ -76,9 +76,14 @@ const API_MERGE = String.raw`\/merges?\b|mergePullRequest|mergeBranch|enablePull // what separated nothing in a shell line. Both are rewritten to a space. Only the loose direction // is taken: an unpaired quote is left alone, and a comma between two branch names becomes two // branch names, which this scan already treats as two refspecs. +// +// A backtick substitution becomes `$` and its command, the text a `$(...)` leaves once its +// brackets go. So `` `git push origin main` `` is still a push, and a refspec built from one reads +// as a value the shell fills in when it runs. const normalize = (command) => command .replace(/(["'])([^"'\n]*)\1\s*,?/g, "$2 ") + .replace(/`([^`]*)`/g, "$$ $1 ") .replace(/[()[\]{},]/g, " "); const FORBIDDEN = [ @@ -105,6 +110,12 @@ const FORBIDDEN = [ String.raw`${GH_API}(?=${CONT}\s(?:-X\s*|--method[= ]\s*)(?!GET\b)[A-Za-z])${CONT}\bgit\/refs\/heads\/(?:${TRUNK_ALT})(?![\w/-])` ), ], + [ + "moving or deleting a ref through the GraphQL API, which may be trunk", + // GraphQL names the ref by node ID or inside an input list, so no text rule can tell trunk + // from a feature branch. Every ref update or delete is refused wherever it points. + new RegExp(String.raw`(?:${GH_API}|\bcurl\b)${CONT}\b(?:updateRefs?|deleteRef)\b`), + ], [ "retargeting a pull request base, which rewrites the whole stack under it", new RegExp(String.raw`${gh("pr", "edit")}${CONT}--base\b`), @@ -215,20 +226,28 @@ function shellTokens(command) { /** * The simple commands in a command line, cut where the shell ends one: `&&`, `||`, `;`, `|`, `&` * and newline. A push's refspecs end with its own command, so `git push origin feature && git - * rev-parse HEAD` is a feature push and a read, not a push of HEAD. + * rev-parse HEAD` is a feature push and a read, not a push of HEAD. A subshell or brace group cuts + * the same way, so `(cd x && git push origin feature && git rev-parse HEAD)` reads the same. * - * Every miss leaves text joined, which fails closed. Nothing inside quotes, backticks or brackets - * is cut, so the `;` in `$(echo; echo main)` and the newlines of a list literal stay with the - * push they feed, and the `&` of `2>&1` or `&>` is a redirection, not a separator. A heredoc body - * gets no exception: each line is scanned as its own command, because `bash <&1` or `&>` is a redirection, not a + * separator. A comment is dropped up to its newline, quotes and all, so the apostrophe in + * `# don't` opens nothing. A heredoc body gets no exception: each line is scanned as its own + * command, because `bash < { - out.push(command.slice(start, at)); + out.push(kept + command.slice(start, at)); + kept = ""; start = at + 1; }; for (let i = 0; i < command.length; i += 1) { @@ -241,15 +260,20 @@ function simpleCommands(command) { if (ch === quote) quote = null; } else if (ch === "'" || ch === '"' || ch === "`") { quote = ch; + } else if (ch === "#" && /^$|[\s;&|()]/.test(command[i - 1] ?? "")) { + kept += command.slice(start, i); + start = command.indexOf("\n", i); + if (start < 0) start = command.length; + i = start - 1; } else if ("([{".includes(ch)) { - depth += 1; + brackets.push(ch !== "[" && /^[\s({]*$/.test(kept + command.slice(start, i))); } else if (")]}".includes(ch)) { - depth = Math.max(0, depth - 1); - } else if (depth === 0 && (ch === "\n" || separates(command, i))) { + brackets.pop(); + } else if (brackets.every(Boolean) && (ch === "\n" || separates(command, i))) { cut(i); } } - out.push(command.slice(start)); + out.push(kept + command.slice(start)); return out; } @@ -303,15 +327,40 @@ function eachGitCommand(command, visit) { } } +// A redirection with its target glued on, and an operator alone, whose target is the next token. +// The target names a file, not a refspec. +const REDIRECTION = /^(?:\d+|&)?[<>]/; +const REDIRECTION_ALONE = /^(?:\d+|&)?[<>]+[&|]?$/; + +/** + * Why a push destination may be trunk although it names no trunk branch, or null. + * + * `HEAD` is whatever this checkout is on, which from a trunk checkout is trunk, and `git push + * origin HEAD` is the idiom an agent actually writes. `$` opens a parameter or command + * substitution, and normalize spells a backtick one with `$` too, so the shell fills the name in + * when it runs. `*` is a pattern git matches against every local branch. + */ +function unreadable(dest) { + if (dest === "HEAD") return "pushing the current branch, which may be trunk"; + if (/[$*]/.test(dest)) { + return "pushing a refspec that resolves only when it runs, which may be trunk"; + } + return null; +} + /** * Why this `git push` is refused, or null. * * Deliberately blocks every push whose refspec lands on a trunk branch, not only forced ones: * the port's own pause list now names "pushing to trunk" as irreversible, and a plain * `git push origin HEAD:main` does that with no force flag to key on. A push to a feature branch - * is the port's normal rebase flow and stays allowed. + * is the port's normal rebase flow and stays allowed. A destination this scan cannot read is + * refused too, but only once no refspec is certainly trunk, so the reason names the surest one. */ -function pushRefuse(args) { +function pushRefuse(argv) { + const args = argv.filter( + (token, i) => !REDIRECTION.test(token) && !REDIRECTION_ALONE.test(argv[i - 1] ?? "") + ); const has = (token) => args.includes(token); if (has("--dry-run") || has("-n")) return null; if (has("--mirror") || has("--all") || has("--tags")) { @@ -320,6 +369,7 @@ function pushRefuse(args) { if (has("--delete")) return "deleting a branch on the forge"; const forced = args.some((t) => /^--force/.test(t) || t === "-f"); let sawRefspec = false; + let unsure = null; // The first bare, colon-free, non-HEAD token is the remote name, not a refspec. Counting it as // one made `git push origin` look like a push with a destination. const bare = args.filter((t) => !t.startsWith("-")); @@ -328,31 +378,22 @@ function pushRefuse(args) { // `:branch` is git's refspec delete form; it carries no --delete flag. if (token.startsWith(":")) return "deleting a branch on the forge"; if (token.startsWith("-")) continue; - if (!token.includes(":")) { - if (!remoteSeen) { - remoteSeen = true; - continue; - } - // A bare destination, or HEAD, is whatever this checkout is on. From a trunk checkout that - // is trunk, and there is nothing in the command to tell, so it is treated as trunk-risky - // rather than allowed. `git push origin HEAD` is the idiom an agent actually writes. - sawRefspec = true; - // A leading `+` is git's own force spelling, and the destination can still be trunk. - const bareDest = token.replace(/^\+/, "").replace(/^refs\/heads\//, ""); - if (bareDest === "HEAD") return "pushing the current branch, which may be trunk"; - if (TRUNK.has(bareDest)) { - return forced ? "rewriting history on a trunk branch" : "pushing to trunk"; - } + if (!remoteSeen && !token.includes(":")) { + remoteSeen = true; continue; } sawRefspec = true; - const afterColon = token.slice(token.lastIndexOf(":") + 1); - const clean = afterColon.replace(/^\+/, "").replace(/^refs\/heads\//, ""); - if (TRUNK.has(clean)) { - return forced ? "rewriting history on a trunk branch" : "pushing to trunk"; - } + // The destination follows the last colon, or is the whole token. A leading `+` is git's own + // force spelling, and the destination can still be trunk. + const dest = token + .slice(token.lastIndexOf(":") + 1) + .replace(/^\+/, "") + .replace(/^refs\/heads\//, ""); + if (TRUNK.has(dest)) return forced ? "rewriting history on a trunk branch" : "pushing to trunk"; + unsure ??= unreadable(dest); } - // No refspec at all: `git push` with only flags, or `git push origin`. Same reasoning. + if (unsure) return unsure; + // No refspec at all: `git push` with only flags, or `git push origin`. Same reasoning as HEAD. if (!sawRefspec) return "pushing without an explicit refspec, which may be trunk"; return null; } diff --git a/plugins/pstack/extensions/pstackpolicy/index.test.js b/plugins/pstack/extensions/pstackpolicy/index.test.js index 2fd9d78..e57f048 100644 --- a/plugins/pstack/extensions/pstackpolicy/index.test.js +++ b/plugins/pstack/extensions/pstackpolicy/index.test.js @@ -162,6 +162,39 @@ const MUST_BLOCK = [ // `main;`, which is not trunk. ["git push origin main; echo done", "pushing to trunk"], ["(git push origin main; true)", "pushing to trunk"], + // A subshell still ends its commands, and a substitution inside one still feeds its push. + ["(cd x && git push origin main)", "pushing to trunk"], + ["(git push origin $(echo feature; echo main))", "pushing to trunk"], + // GraphQL names a ref by node ID or inside an input list, so any ref mutation may be trunk. + [ + 'gh api graphql -f query=\'mutation{updateRef(input:{refId:"r",oid:"abc",force:true}){clientMutationId}}\'', + "through the GraphQL API", + ], + [ + 'gh api graphql -f query=\'mutation{updateRefs(input:{repositoryId:"r",refUpdates:[{name:"refs/heads/main",afterOid:"abc",force:true}]}){clientMutationId}}\'', + "through the GraphQL API", + ], + [ + 'gh api graphql -f query=\'mutation{deleteRef(input:{refId:"r"}){clientMutationId}}\'', + "through the GraphQL API", + ], + [ + `curl https://api.github.com/graphql -d '{"query":"mutation{updateRef(input:{refId:\\"r\\",oid:\\"abc\\"}){clientMutationId}}"}'`, + "through the GraphQL API", + ], + // A refspec the shell or git expands is only known when it runs, and it may be trunk. + ["git push origin $(git branch --show-current)", "resolves only when it runs"], + ["git push origin `git branch --show-current`", "resolves only when it runs"], + ['git push origin "${BRANCH}"', "resolves only when it runs"], + ['git push -u origin "$BRANCH"', "resolves only when it runs"], + ['git push origin HEAD:"$(git branch --show-current)"', "resolves only when it runs"], + ["git push origin 'refs/heads/*:refs/heads/*'", "resolves only when it runs"], + // A backtick substitution runs the command inside it, the same as `$(...)`. + ["echo `git push origin main`", "pushing to trunk"], + // A comment ends at its newline whatever quotes it holds, and `$#` is a parameter, not a comment. + ["# don't push trunk from here\ngit push origin main", "pushing to trunk"], + ["git status # it's clean\ngit push origin HEAD:main", "pushing to trunk"], + ["echo $#; git push origin main", "pushing to trunk"], ]; const MUST_PASS = [ @@ -213,6 +246,14 @@ const MUST_PASS = [ "git push origin feature\ngit log -1 HEAD", "git push origin feature 2>&1 && git rev-parse HEAD", "cat > notes.md <<'EOF'\nPush with git push origin feature.\nEOF\ngit rev-parse HEAD", + // A subshell or brace group cuts the same as the commands written without it. + "(cd x && git push origin feature && git rev-parse HEAD)", + "{ git push origin feature; git rev-parse HEAD; }", + // A comment is not an argument, and a redirection target is not a refspec. + "git push origin feature # never main", + 'git push origin feature > "$LOG" 2>&1', + // Reading a ref through GraphQL is not moving it. + "gh api graphql -f query='query{repository(owner:\"o\",name:\"r\"){ref(qualifiedName:\"main\"){target{oid}}}}'", ]; describe("pstackpolicy", () => { @@ -243,30 +284,30 @@ describe("pstackpolicy", () => { expect(ok?.block).toBeUndefined(); }); - test("an eval cell is gated the same way a bash call is", async () => { - // omp://approval-mode.md: eval declares the exec tier and can spawn a shell, so a rule that - // only read bash's `command` field was not a rule about the command. - for (const [code, rule] of [ - ["await Bun.$`gh pr merge 12 --squash`", "merging a pull request"], - ['subprocess.run(["git", "push", "origin", "HEAD:main"])', "pushing to trunk"], - ['await tool.bash({ command: "gh repo delete o/r --yes" })', "deleting the repository"], - // A list literal broken over lines is still one command. - ['subprocess.run([\n "git", "push", "origin", "feature",\n "HEAD:main",\n])', "pushing to trunk"], - ]) { - const result = await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined }); - expect(result?.block, `should block: ${code}`).toBe(true); - expect(result?.reason, `should name the rule: ${code}`).toContain(rule); - } + // omp://approval-mode.md: eval declares the exec tier and can spawn a shell, so a rule that + // only read bash's `command` field was not a rule about the command. + test.each([ + ["await Bun.$`gh pr merge 12 --squash`", "merging a pull request"], + ['subprocess.run(["git", "push", "origin", "HEAD:main"])', "pushing to trunk"], + ['await tool.bash({ command: "gh repo delete o/r --yes" })', "deleting the repository"], + // A list literal broken over lines is still one command, and a comment inside it does not + // open a quote that hides the rest. + ['subprocess.run([\n "git", "push", "origin", "feature",\n "HEAD:main",\n])', "pushing to trunk"], + ['subprocess.run([\n "git", "push", "origin", # don\'t\n "HEAD:main",\n])', "pushing to trunk"], + ["await Bun.$`git push origin HEAD:main`", "pushing to trunk"], + ])("refuses the eval cell %p as %p", async (code, rule) => { + const result = await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined }); + expect(result?.block).toBe(true); + expect(result?.reason).toContain(rule); }); - test("an eval cell with no forge mutation still passes", async () => { - for (const code of [ - 'print("git push origin feature/x")', - 'subprocess.run(["git", "push", "origin", "feature"])\nsubprocess.run(["git", "rev-parse", "HEAD"])', - ]) { - const ok = await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined }); - expect(ok, `should pass: ${code}`).toBeUndefined(); - } + test.each([ + 'print("git push origin feature/x")', + 'subprocess.run(["git", "push", "origin", "feature"])\nsubprocess.run(["git", "rev-parse", "HEAD"])', + "await Bun.$`git push origin feature`", + ])("allows the eval cell %p", async (code) => { + expect(await call("eval", { language: "py", code }, { PSTACK_LANDING_GRANT: undefined })) + .toBeUndefined(); }); test("ignores tools that carry no command", async () => {