feat: infer gh CLI permissions for activation job pre-steps#32849
Conversation
Create a gh CLI command-to-permissions mapping (data/gh_cli_permissions.json and gh_cli_permissions.go) and integrate it into buildActivationPermissions so that activation job pre-steps calling `gh pr diff`, `gh issue view`, etc. automatically get the required GitHub Actions permissions (pull-requests: read, issues: read, actions: read, etc.) without manual declarations. Fixes: compiler omitting pull-requests: read from the activation job even when a pre-step calls `gh pr diff` (as reported for the gh-aw-docs-review workflow). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add word boundary \b to ghSubcommandRE to avoid false positives on partial action words - Add early len(ctx.data.Jobs) > 0 guard in buildActivationPermissions before extracting pre-step scripts - Clarify TestActivationJobPermissionsNoPreStepChanges: remove pull-requests: read from frontmatter and add comment explaining the activation job computes permissions independently from the main job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run "gh help" to scan additional commands available, fill up the mapping. Attempts to use write APIs should be flagged as errors since permissions: writes are not allowed |
…activation pre-steps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commits Expanded mapping — ran Write-command detection — Groups intentionally excluded from the mapping (no meaningful |
|
@copilot map every group to app permissions as well since the user can setup github apps too |
|
@copilot apply detector to agent job and activation job |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…uild context Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done across two commits ( Mapping — Every group in
The API path patterns also gained Runtime use — Maintenance — The subcommand regex is now compiled dynamically from the JSON group keys, so adding a new group to the JSON is sufficient — no regex update required. |
|
@copilot this applies to pre-steps, steps, post-steps, pre-agent-steps... |
…steps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…s check, fix test SHA Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. The write-command detector and read-permission inference now apply to both the activation job (already in place) and the agent job pre-steps. Changes in
Supporting additions: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit What changed:
15 new tests cover |
There was a problem hiding this comment.
Pull request overview
Adds inference of GitHub CLI permissions from workflow step scripts so generated jobs receive read permissions needed by gh commands.
Changes:
- Adds a JSON mapping and Go scanner for
ghsubcommands/API paths to permission scopes. - Merges inferred permissions into activation and agent job permission handling.
- Adds unit and integration tests for permission inference and write-command validation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/permissions_operations.go |
Adds helper for merging inferred permissions into existing YAML. |
pkg/workflow/gh_cli_permissions.go |
Adds embedded permission data loading, script scanning, and step run-script extraction helpers. |
pkg/workflow/gh_cli_permissions_test.go |
Adds tests for inference, extraction, merge behavior, and compile scenarios. |
pkg/workflow/data/gh_cli_permissions.json |
Defines gh subcommand/API path to permission mappings. |
pkg/workflow/compiler_main_job.go |
Infers and validates gh usage for agent job step sections. |
pkg/workflow/compiler_activation_job.go |
Propagates errors from activation permission building. |
pkg/workflow/compiler_activation_job_builder.go |
Caches activation scripts and merges inferred permissions into activation job permissions/app token scopes. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
| // Cache scripts from all step sections and inferred permissions once to avoid redundant | ||
| // extraction and inference calls in buildActivationPermissions and | ||
| // addActivationFeedbackAndValidationSteps. | ||
| activationJobName := string(constants.ActivationJobName) | ||
| for _, section := range []string{"pre-steps", "steps", "post-steps"} { |
| for _, section := range []string{"pre-steps", "steps", "pre-agent-steps", "post-steps"} { | ||
| agentAllScripts = append(agentAllScripts, extractRunScriptsFromJobSection(data.Jobs, agentJobName, section)...) |
| // ghAPIRE matches `gh api <path>` invocations. | ||
| // Capture group: (1) API path (up to the first whitespace, pipe, or quote). | ||
| var ghAPIRE = regexp.MustCompile(`(?m)(?:^|[\s|;])gh\s+api\s+([^\s|;&"'\\]+)`) |
|
|
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,683 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why an ADR for this PR?This PR introduces three durable architectural commitments worth recording:
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd (new feature with substantial test coverage) and consulted /zoom-out to understand how the new gh_cli_permissions subsystem slots into the compiler pipeline.
Key Themes
1. Broken URL in both error messages — github.github.com is not a real domain. Both activation-job and agent-job compile errors point to a 404. (Inline comment on compiler_activation_job_builder.go.)
2. Dead-code appearance in inferPermissionsFromShellScripts — The write-command fallthrough branches (writeCommands / appWriteCommands) are never reachable in practice because all callers error-out on write commands first via detectWriteCommandsInShellScripts. Either remove the branches or add a test that exercises them directly to document the intent. (Inline comment on gh_cli_permissions.go.)
3. Missing test for unknown-subcommand group-level fallback — When gh pr some-future-cmd is used and the action is not in any known list, the code silently falls back to group-level read permissions. This important behaviour has no test pinning it. (Inline comment on test file.)
4. Fragile permissions: {} string comparison — The exact-string match data.Permissions != "permissions: {}" is a hidden contract between the YAML serialiser and this check. A constant or predicate would make the intent explicit and resilient to serialiser changes. (Inline comment on compiler_main_job.go.)
5. Test naming style — 15+ TestFunc_Case functions instead of table-driven t.Run subtests. Not a blocker, but inconsistent with the project's testing conventions. (Inline comment on test file.)
Positive Highlights
- ✅ Data-driven architecture: Externalising the
ghsubcommand → permission mapping intogh_cli_permissions.jsonmeans adding new subcommands requires zero Go changes — a genuinely deep module design. - ✅ Caching in context: Storing
activationAllScriptsandactivationInferredPermsonactivationJobBuildContextavoids redundant YAML parsing and regex scanning across multiple call sites. - ✅ Compile-time write-command detection: Surfacing
gh pr create/gh issue closein pre-steps as a compile error rather than a silent runtime failure is great DX. - ✅ Integration tests:
TestActivationJobPermissionsWithGhPrDiffPreStepreproduces the exact user-reported scenario end-to-end — this is exactly the right style of regression test. - ✅
init()panic on bad JSON: Failing fast at startup rather than silently producing wrong permissions is the right trade-off for an embedded config file.
Verdict
The URL typo and the permissions: {} fragility are the two items I'd fix before merge; the others are lower priority. Overall the implementation is solid and the design is extensible.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 9.8M
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 69/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 63 new tests verify behavioral contracts with no mock library usage and a valid build tag. Minor suggestions: consolidate the 20+ single-assertion TestInferPermissionsFromShellScripts_* tests into a table-driven test, and add assertion messages to ~16 bare assertions.
|
@copilot review all comments |
|
|
|
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…erage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three review comments in commits Review comment 1 ( Review comment 2 ( Review comment 3 ( |
|
@copilot merge main and recompile |
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details.
|
Done. Merged |
The compiler omitted
pull-requests: readfrom theactivationjob even when ajobs.activation.pre-stepcalledgh pr diff— causing Vale (and similar tools) to silently lint nothing becausegh pr difffailed without the permission.buildActivationPermissionscomputed activation job permissions based solely on workflow-level features (reactions, label events, etc.) and never inspectedrunscripts in user-defined pre-steps.Changes
data/gh_cli_permissions.json— New data file mappingghsubcommand groups (pr,issue,workflow,run,release) and REST API path patterns to required GitHub Actions permissions, following the same pattern asgithub_toolsets_permissions.json.gh_cli_permissions.go— Loads the JSON at init and exposes:inferPermissionsFromShellScripts([]string)— scans shell scripts forghinvocations via regex, returns minimum required permissionsextractRunScriptsFromJobPreSteps(map[string]any, string)— extractsruncontent fromjobs.<name>.pre-stepscompiler_activation_job_builder.go—buildActivationPermissionsnow scansjobs.activation.pre-stepsand merges inferred permissions into the activation job's permission map.gh_cli_permissions_test.go— Unit tests for all scanner functions plus an integration test reproducing the exact reported scenario.Example
Mapping coverage:
gh pr *→pull-requests,gh issue *→issues,gh workflow */gh run *→actions,gh release *→contents,gh api /repos/.../pulls→pull-requests, etc.Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.