Repository navigation
fix(pstackpolicy): close merge bypasses and scope push refspecs to their command - #35
Merged
Merged
Conversation
…eir 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.
Removing the forwarding passed both the unit tests and smoke. The fake host now records sendUserMessage and exposes isIdle. `/poteto-mode <task>` must pin and send the trimmed text, use `deliverAs: "followUp"` while a turn runs, and `status` must send nothing and keep the pin.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pstackpolicyis the only thing that stops a headless pstack subagent from landing work without the operator'sPSTACK_LANDING_GRANT. A test audit found five merge or trunk-write commands it let through, one false positive that refused ordinary feature pushes, and tests too loose to notice either. This PR closes the holes, removes the false positive, and rewrites the tests so each blocked command names the rule that must fire. The second policy commit,3ea983e, closes the four gaps the first round listed as open.For an agent driving the port, a feature push chained with a later read now runs (
git push origin feature && git rev-parse HEAD), inside a subshell or brace group too ((cd x && git push origin feature && git rev-parse HEAD)). These now stop at the grant:gh pr -R o/r merge 12andgh pr --repo o/r merge 12. The same flag position also reachededit --base,close,review, and every other gh group the policy names (issue,release,secret,variable,workflow,repo). gh 2.102.1 resolvesgh <group> -R o/r <verb>to the verb forpr,issue,release,secretandworkflow, checked with--help.gh api repos/o/r/merges ...and the GraphQLmergeBranchmutation, pluscurlto/merges.gh api -X PATCH repos/o/r/git/refs/heads/main ..., a trunk ref moved without a push.enablePullRequestAutoMergemutation.gt merge.git push origin main; echo doneand(git push origin main; true). Both were read as a push ofmain;and allowed.updateRef,updateRefsanddeleteRefmutations, throughgh apiorcurl.git push origin $(git branch --show-current), and any push destination built from$VAR,${VAR}, a backtick substitution or a*pattern. Each may be trunk, so each is refused likeHEAD.# don't ...).echo `git push origin main`), and the same push in an eval cell'sBun.$`...`.Changes
extensions/pstackpolicy/index.jsgh(group, verbs)pattern builder lets flags sit before the group and between the group and the verb. Every gh rule uses it.API_MERGElists every API spelling of a merge. Thegh apimerge rule and thecurlrule share it.git/refs/heads/<trunk>. A read, an explicit GET, and a PATCH to a feature ref stay allowed, matching the push rule's treatment of feature branches.updateRefanddeleteRefname the ref by node ID andupdateRefsnames it inside an input list, so no text rule can tell trunk from a feature branch. The rule refuses each of them throughgh apiorcurl, wherever it points. A GraphQL read of a ref stays allowed.--automerge first, so this rule's reason never fired.git pushand local-history scans read each simple command on its own.simpleCommandscuts at&&,||,;,|,&and newline, inside a subshell or brace group as well. A(or{counts as a group only at the start of a command. It does not cut inside quotes, backticks, a substitution or a list literal, or at the&of2>&1or&>. Every miss leaves text joined, which fails closed.#to its newline, whatever quotes it holds. It is dropped without cutting the command around it, so a Python list literal with a comment inside stays one command.$#and${#x}are not comments.shellTokensends a word at the same separators, so a glued;no longer hides a refspec.$or*resolves only when it runs, so it is refused as "pushing a refspec that resolves only when it runs, which may be trunk".HEADgets the same treatment it had. When another refspec in the push is certainly trunk, that reason wins. The bare and colon refspec forms now share one destination check.normalizespells a backtick substitution as$and its command, which is the text a$(...)already left. A backtick refspec then reads as run-time, and a push inside backticks reaches the push scan.git push origin feature > "$LOG"stays allowed.DENY_REASONSis gone. Nothing read it but a test that could not fail.Heredoc decision. A heredoc body counts. Each body line is scanned as its own command, because
bash <<EOFruns it and a heredoc often writes a script that runs later. The forge-mutation regexes keep scanning the whole text, sogh pr mergeinside a heredoc body is still refused. A later line never lends its words to an earlier push.extensions/pstackpolicy/index.test.jstool_call. The old fake ignored the name and passed every test with the handler registered ontool_result.MUST_BLOCKis[command, reasonFragment]pairs, run withtest.each. A command refused by the wrong rule fails.$(...)substitution stays with its push, a heredoc body that runs a trunk push is refused, and a list literal split over lines in an eval cell is still one command. The second commit adds guards for a trunk push in a subshell, a substitution inside a subshell,$#, a redirection target holding$LOG, a GraphQL ref read, and aBun.$feature push.test.each, so each reports its own failure.extensions/potetomode/index.test.js/poteto-mode <task>pins the session and sends the trimmed task text with its case kept. When the agent is busy it sends withdeliverAs: "followUp"./poteto-mode statussends nothing and leaves the pin as it was. Removing the forwarding line used to pass both the unit tests and smoke.README.mdlists the new rules, the flag position, the push scoping, the run-time refspec rule, comments, groups, redirections, and the heredoc decision.Gaps still open
A throwaway probe found each of these allowed on
84e53e9and still allowed on3ea983e.//comment that holds an apostrophe hides the next line's push. Only#comments are modelled.subprocess.run(f"git push origin {b}", shell=True)is missed, because thefprefix glues ontogit. An identifier refspec in a list literal, as in["git", "push", "origin", branch], reads as a literal branch name and is allowed.curl https://api.github.com/repos/o/r/merges -d '...'is allowed. curl POSTs because of-d, and the curl rule keys only on-X,--requestand--data.gh api graphql -F query=@file) is invisible to every text rule.These err toward refusing.
git push origin "feature/$TICKET". This is new in3ea983e.if true; then (cd x && git push origin feature && git rev-parse HEAD); fiis still refused.Verification
All commands ran in this worktree on bun 1.4.4 and gh 2.102.1. The first round ran on omp 18.14.1 and the second on omp 18.14.2.
Second policy commit,
3ea983e.cd plugins/pstack && bun test ./extensionsgave 179 pass, 0 fail.bun test ./extensions/potetomode/index.test.js ./extensions/pstackpolicy/index.test.js, gave 179 pass, 0 fail.git stash push -- plugins/pstack/extensions/pstackpolicy/index.js, thenbun test ./extensions, thengit stash pop, each on its own. Against84e53e9'sindex.jsthe suite gave 161 pass, 18 fail. The 18 are every new case. Four are GraphQL ref mutations, six are run-time refspecs, one is a push inside backticks, two are trunk pushes after an apostrophe comment, three are false positives (the subshell, the brace group, a comment that namesmain), and two are eval cells (a Python list holding a comment, aBun.$trunk push). The 6 guards pass on both sources. After the pop the suite gave 179 pass, 0 fail.bun teston a temp copy after each of 13 one-change edits to the new source. All 13 made tests fail. The edits were no backtick rewrite, the GraphQL rule dead, the GraphQL rule withoutcurl, no$check, no*check, an unreadable refspec returning before a certain trunk one, no redirection filter, no comment handling,#anywhere as a comment, a comment that cuts the command, every bracket opaque, every(and{a group, and brace groups opaque.bash omp-port/check-port.shprintedcheck-port: PASS.OMP_SMOKE_REQUIRED=1 bun omp-port/smoke-plugin.mjsprintedPASS omp/18.14.2: 60/60 skills registered; /poteto-mode on/off recorded; pin reloaded on new_session; no model turn.First policy commit,
59b23ec, with the potetomode tests in84e53e9.cd plugins/pstack && bun test ./extensionsgave 151 pass, 0 fail, and so did the CI form.--autocases fail because the old code fired the plain merge rule, which is the dead rule. ThemergePullRequestand merge-endpoint cases fail only because their reason was renamed from "merging a pull request through the API" to "merging through the API". The 6 fail-closed guards pass against the old source, which never cut a command line. They guard the new cut, and the mutation run below shows the naive cut failing the redirection, substitution and list-literal guards. The two heredoc cases pin the decision above.allowfor all 9 must-block commands andBLOCKfor the chained feature push before the change. After the change it printedBLOCKfor all 9 andallowfor the chained push.bun teston a temp copy after each of 15 one-change edits. All 15 made tests fail. The edits were a naive splitter, no splitter, no glued-separator split,&as a separator inside a redirection, the auto rule moved back behind the merge rule, the handler registered ontool_result,gt mergedropped, the trunk-ref rule dropped, the trunk-ref rule ignoring the method, flags allowed only before the group, task forwarding removed, alwaysfollowUp, neverfollowUp,statusforwarded as a task, and the task text lowercased.gh <group> -R o/r <verb> --helpforpr merge,issue close,release create,secret setandworkflow runeach printed that verb's help.bash omp-port/check-port.shprintedcheck-port: PASS.OMP_SMOKE_REQUIRED=1 bun omp-port/smoke-plugin.mjsprintedPASS omp/18.14.1: 60/60 skills registered; /poteto-mode on/off recorded; pin reloaded on new_session; no model turn.