DEV Community

JinHyuk Sung
JinHyuk Sung

Posted on

Would you block a PR that changes GitHub Actions contents permission from read to write?

A sandbox PR changed one GitHub Actions workflow permission:

permissions:
  contents: write
Enter fullscreen mode Exit fullscreen mode

The base branch had:

permissions:
  contents: read
Enter fullscreen mode Exit fullscreen mode

That is the concrete case I am trying to calibrate.

Agent Gate reported:

Agent Gate: NEEDS HUMAN DECISION
Decision: warn
Why: contents permission increased from read to write.
Path: .github/workflows/demo-release.yml
Recommended next step: review the workflow permission change before merging.
Policy status: warning today; eligible to become a merge gate after tuning.

Rule: workflow/permission-escalation
Policy source: built-in default
Enter fullscreen mode Exit fullscreen mode

Live PR comment proof:
https://github.com/sjh9714/agent-gate-install-smoke-20260617/pull/13#issuecomment-4828248162

What matters to me is that this did not depend on an LLM noticing the change.

The Action did not:

  • checkout PR code
  • run repository scripts
  • call an LLM at runtime
  • load policy from the PR head branch

The first-run repo config was also absent. Agent Gate used its built-in default policy and recorded:

configSource: default
Enter fullscreen mode Exit fullscreen mode

I am not trying to claim that the PR is automatically bad. A permission increase can be intentional.

The question is what CI should do when it sees this kind of boundary change.

My current default is:

  • warn on first run
  • keep the report human-readable
  • let teams promote this finding to block after tuning

For AI-generated PRs, I think deterministic CI evidence is useful because agent changes can touch workflow and security boundaries as part of ordinary work.

But this specific finding is broader than AI: any PR that raises GitHub Actions permissions may deserve deliberate review.

Question:

In your repo, is this block, warn, or noise?

What extra evidence would make it actionable?

Repo:
https://github.com/sjh9714/Agent-Gate

Disclosure: I used AI assistance to help draft and edit this article, and I reviewed the technical claims before publishing.

Top comments (15)

Collapse
 
unitbuilds profile image
UnitBuilds

I think it'd be useful if it did a check to see whether the PR actually requires the permission change, or not. Whether it's scoped or not and what sectors are affected by the permission change.

These can catch mistakes, drafts and help plug newly created holes before it's published to production

Collapse
 
sjh9714 profile image
JinHyuk Sung

Thanks — this is a useful way to frame it.

I agree that read -> write by itself should not automatically mean “bad PR.” A workflow may legitimately need contents: write for things like releases, tags, generated commits, or publishing flows.

The first-run behavior I’m aiming for is narrower:

  • surface the permission escalation
  • show the exact workflow path
  • keep it in warn mode by default
  • ask a human to decide whether the increased permission is expected

The next evidence layer is probably what you’re pointing at:

  • what job or workflow gained the permission
  • whether the changed workflow appears to contain steps that need that permission
  • whether the permission is scoped to the smallest job/workflow possible
  • which repo areas or release paths could be affected
  • whether the right reviewer approved that boundary change

I’d be careful about claiming Agent Gate can fully prove “this permission is required,” because that can become semantic judgment pretty quickly. But I do think it can make the review much more actionable by saying something like:

contents: write was added to .github/workflows/release.yml; this can affect release/tag/repo-content writes; no matching reviewer evidence was found; please confirm whether this is expected.

That feels like a better direction than either silently allowing the change or blocking every permission increase by default.

Appreciate the feedback — this points pretty directly toward a “scope + reviewer evidence” layer after the basic warning.

Collapse
 
unitbuilds profile image
UnitBuilds

Never blindly accept, instead, these are the parameters passed to the reviewer. Chances are these changes are either done by a senior dev, who understands their purpose and this just validates for them that it's done properly, or it's a junior that messed up and created a leak, which with this, the senior dev can quickly see if it's done correctly based on the task and scope assigned.

Imo, this is the kind of thing I'd put as a pre-commit too, so the person creating the PR can look at it first to validate themselves and maybe add notes to each/ each violation on why it was done, maybe with a section where they can dump their scoped assignment, so it's more practical to understand what each commit was made for, who signed off on it, what were the potential problems and what was the criteria of the task. Could actually be a good way to seed training data for a LLM now that I think of it?

Thread Thread
 
sjh9714 profile image
JinHyuk Sung

Yeah, I agree with this framing.

I don’t want Agent Gate to blindly accept or block the PR by itself. The more useful behavior is to put the right facts in front of the reviewer:

  • what changed
  • which workflow/job changed
  • which permission changed
  • what that permission can affect
  • whether the author explained why it was needed
  • whether someone appropriate signed off on it

That way, if it’s an intentional release/publishing change, the report helps validate the boundary. If it’s an accidental permission leak, the reviewer can spot it much faster.

The author-side idea is interesting too. I probably wouldn’t call these “violations” by default, because contents: write can be legitimate. But I can imagine the author seeing the finding before opening the PR and adding a note like “needed for release tags” or “needed for generated commit push.” Then the CI report can carry that context into review.

The training-data angle is interesting, but I’d keep that separate and explicitly opt-in. The first thing I’d want to trust is a clean review/audit record.

So yes, I think the next useful layer is not smarter auto-blocking. It’s better review context around the finding.

Thread Thread
 
unitbuilds profile image
UnitBuilds

And it creates an audit trail. If a vulnerability surfaces, they can inspect why, when, where, by who and who signed off on it.

Agreed, commits are a touchy field, because some people work in a broken branch just to make sure they save all progress states, but on submit ready commits, they want to still be able to denote it's a stable state and validate it. Eg. maybe they left some things open for their particular dev environment that is different in production and the detect is there to make sure they dont accidentally put their opening on a live server? So definitely focus on the review-phase, because that's what people will be using most. The detect just needs to work, but the review stage is accountability... If a dev signs off on it, it means they've reviewed the code + understand the reasoning of it all, the reviewer does the same when accepting.

Thread Thread
 
sjh9714 profile image
JinHyuk Sung

Yes — “review-phase accountability” feels like the right center of gravity.

Detection matters, but the durable value is the audit trail around the decision:

  • what changed
  • why it was accepted
  • who understood the boundary change
  • who signed off
  • what decision was made at review time

I’d want that to be a decision record, not a blame trail. Agent Gate should not pretend it can infer intent by itself. It should surface the boundary change, preserve the reasoning around it, and make the human review easier to trust later.

I also agree that commit-time/pre-commit behavior can get noisy, because branches often represent messy working state. The review phase is cleaner: by then the author is saying “this is ready to evaluate,” so asking for justification/sign-off makes more sense.

This is pushing me toward treating reviewer/sign-off evidence as a separate layer after the raw finding evidence:

finding -> author reason -> reviewer sign-off -> decision record

Thread Thread
 
unitbuilds profile image
UnitBuilds

I think it should be a separate layer. Given that reviewing isnt just justifying, it's acknowledgement of intent and implementation. It's not a blame-game, it's meant to be a state tracker, that shows when a feature was upgraded and retro-actively see where past commits added things that were too broad scope, at which point revision is required to re-scope it. Maybe a feature was half implemented, then commented out, but the permissions were left open? That's the 2nd great value of it, being able to audit codebases' past, to see where high-risk changes were never remedied. If you're dealing with projects Chromium's scale, simply know what, when, where, why is the difference between a day-zero exploit getting patched before it's uncovered, or a leak staying open for years.

Thread Thread
 
sjh9714 profile image
JinHyuk Sung

Yes — I think that is the right split.

Raw detection should stay one layer:

this permission boundary changed

Review/sign-off should be a separate layer:

this is why it was accepted, who reviewed it, and what state the repo was in when the decision was made

I like “state tracker” better than “blame trail.” The useful part is being able to come back later and understand why a broad permission was introduced, whether it was still needed, and whether it was ever narrowed again.

I’d be careful not to make the first version too broad, though. I don’t want Agent Gate to pretend it can infer long-term intent from history by itself. The safer first step is to preserve structured review facts at PR time:

  • finding
  • author reason
  • affected workflow/job/permission
  • reviewer or sign-off evidence
  • final decision

Then a later audit/replay mode could ask:

Which high-risk boundary changes were accepted and never re-scoped?

That feels like the right north star: not smarter auto-blocking, but a durable decision record for high-risk changes.

Thread Thread
 
unitbuilds profile image
UnitBuilds

Sounds good, but definitely look into the ability to feed it a git history and have it trace it from initial commit-to-date, so it becomes a tool in what is currently the bulk of work across the globe: Technical debt recon. Now that AI can create a patch in minutes, people are focusing alot more on tidying up their codebases rather than chasing features, so the ability to find past-vulnerabilities, log the state as it progressed and give a final report on "X changes were made, Y were rescoped". That way it can be used to ticket the technical debt and have them assign the tasks of reviewing states and patching where necessary, so old leaks get plugged. Very common when the company hires and fires someone who didnt do a good job, but kept their commits.

Thread Thread
 
sjh9714 profile image
JinHyuk Sung

Yeah, that makes sense.

The history/replay angle is interesting: find permissions that were opened for a temporary reason, then never narrowed again.

I’d keep that separate from the PR-time check, though. The PR check should answer:

what changed, and what does the reviewer need to know right now?

A later audit mode could answer:

which high-risk boundary changes were accepted and never re-scoped?

And I agree with the “not a blame game” framing. The value is not “who messed up,” but “what state changed, why was it accepted, and is that state still appropriate today?”

Thread Thread
 
unitbuilds profile image
UnitBuilds

Exactly, I guess now you have a bit of a roadmap, step 1, separate the detect and review layers, step 2, add audit mode for backlogs.

Thread Thread
 
unitbuilds profile image
UnitBuilds

If your goal is to market it, the audit mode would probably be the premium feature you'd sell copies for, given that it's a massive aid for relieving technical debt being able to detect abandoned permissions that were left open.

Thread Thread
 
sjh9714 profile image
JinHyuk Sung

Yeah, that split makes sense.

Step 1: keep the PR-time path focused on detection + review context.

Step 2: treat history/backlog audit as a separate mode — find high-risk permissions that were opened, accepted, and never re-scoped.

I agree the audit mode is probably the stronger technical-debt use case, but it’s a bigger product surface. I wouldn’t want every PR check to become a historical analyzer.

So for now I’ll keep the immediate scope small, and treat audit/replay as a separate RFC/backlog item.

And yes, the framing should be “find abandoned open permissions,” not “blame whoever introduced them.”

Some comments may only be visible to logged-in visitors. Sign in to view all comments.