Add comment support to approve-workflow-run safe output - #54504
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54504 does not have the implementation label and has 49 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review of PR #54504 complete. Reviewed the diff (approve_workflow_run.cjs comment posting, config plumbing, docs, schema, tests) against ponytail-review criteria. The new buildApprovalCommentBody/postApprovalComment functions mirror established patterns already used elsewhere (e.g. mark_pull_request_as_ready_for_review.cjs footer+comment composition), the config/permission plumbing is minimal and directly required, and the try/catch is the standard log-warning-dont-fail convention used across safe-output handlers. No dead code, reinvented stdlib, unneeded deps, speculative abstractions, or unnecessary flexibility found. Lean already. Ship.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new PR-comment side effect is not idempotent: repeated approval attempts for the same run will post duplicate "run started" comments, creating persistent noise on every linked pull request.
Blocking theme
- The handler always calls
issues.createCommentafter approval and never checks whether that run was already announced. - Because approval attempts are retryable across job/process boundaries, the duplicate-comment path is realistic rather than theoretical.
- This needs a stable marker or update-in-place strategy before the feature is safe to merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.79 AIC · ⌖ 6.91 AIC · ⊞ 7K
Comment /review to run again
| for (const pullRequest of run.pull_requests) { | ||
| const pullRequestNumber = parsePositiveInt(pullRequest.number); | ||
| if (pullRequestNumber !== undefined) { | ||
| await postApprovalComment(githubClient, pullRequestNumber, run.html_url); |
There was a problem hiding this comment.
This change can spam every linked pull request with a duplicate “run started” comment on retries or repeated approvals, because it unconditionally calls issues.createComment and never checks whether the same run was already announced. That turns a benign re-run path into noisy, user-visible churn.
💡 Why this should be fixed
approve_workflow_run is explicitly retryable (processedCount is only local state, and a new job/process starts from zero), so the same run_id can be approved again after a transient failure or a repeated safe-output invocation. With the new behavior, every successful approval posts another identical PR comment.
That creates persistent review noise on the pull request and is especially bad for workflows associated with multiple PRs because the duplication fans out to each one.
Please make the comment idempotent per (pull_request_number, run_id), for example by searching for an existing marker before posting or by updating an existing bot comment instead of always creating a new one.
const marker = `<!-- gh-aw-approve-workflow-run:${runId} -->`;
const body = `${marker}
${buildApprovalCommentBody(run.html_url, pullRequestNumber)}`;
// list comments and skip/update when marker already exists
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues before merge.
📋 Key Themes & Highlights
Key Themes
-
Zero-value trap on
Comment bool—boolzero-value isfalse, but the semantic default istrue. AnyApproveWorkflowRunConfigconstructed outsideparseApproveWorkflowRunConfig(e.g., in tests or future callers) silently under-requests permissions. The existingStagedfield uses*TemplatableBoolfor precisely this reason —Commentshould follow the same pattern. This also affects the permission logic insafe_output_handlers.gowhich relies on the field value directly. -
buildApprovalCommentBodyreads env vars directly — bypasses the config object and makes the function untestable without environment mutations. Pass values from config or as parameters instead. -
Weak test assertion —
toContain("gh-aw-agentic-workflow")tests an internal footer fragment rather than the user-visible content. Prefer asserting the run URL and a meaningful phrase.
Positive Highlights
- ✅ Comment failures are correctly downgraded to warnings — approval is never blocked
- ✅
pull-requests: writevsreadis properly conditioned on the config flag - ✅ Spec, schema, TypeScript types, and docs all updated in lock-step
- ✅ Three focused test cases (default on, explicit off, error resilience)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.4 AIC · ⌖ 11.5 AIC · ⊞ 7.8K
Comment /matt to run again
| @@ -8,6 +8,7 @@ var approveWorkflowRunLog = logger.New("workflow:approve_workflow_run") | |||
| type ApproveWorkflowRunConfig struct { | |||
| BaseSafeOutputConfig `yaml:",inline"` | |||
There was a problem hiding this comment.
[/codebase-design] Comment bool with omitempty and a semantic default of true is a zero-value trap. The Go zero value for bool is false, so any code that constructs ApproveWorkflowRunConfig{} directly (e.g., in tests or future callers) silently gets comment: false — wrong permissions (pull-requests: read instead of write) with no warning.
💡 Suggestion: use *bool like Staged does
Staged in BaseSafeOutputConfig already uses *TemplatableBool for exactly this reason. Apply the same pattern here:
Comment *bool `yaml:"comment,omitempty"`
// default in parseApproveWorkflowRunConfig:
defaultTrue := true
config.Comment = &defaultTrue
// permission logic:
commentEnabled := safeOutputs.ApproveWorkflowRun.Comment == nil || *safeOutputs.ApproveWorkflowRun.CommentThe test TestApproveWorkflowRunCommentDisabled already constructs ApproveWorkflowRunConfig{} with Comment: false — once you switch to *bool the zero value becomes nil (meaning "default true"), and the test would need to pass comment: &falseVal to express the opt-out correctly.
@copilot please address this.
| */ | ||
| function buildApprovalCommentBody(runHtmlUrl, pullRequestNumber) { | ||
| const workflowName = process.env.GH_AW_WORKFLOW_NAME || "Workflow"; | ||
| const workflowSource = process.env.GH_AW_WORKFLOW_SOURCE || ""; |
There was a problem hiding this comment.
[/codebase-design] buildApprovalCommentBody reads process.env.GH_AW_WORKFLOW_NAME and GH_AW_WORKFLOW_SOURCE* directly, bypassing any config object or parameter injection. This makes the function hard to unit-test in isolation and breaks the pattern used by other handlers that receive these values through the handler config map.
💡 Suggestion
Pass the env-derived values through config (they're already set on the config object for other handlers), or at minimum accept them as parameters so the unit test for buildApprovalCommentBody can control them without environment mutations:
function buildApprovalCommentBody(runHtmlUrl, pullRequestNumber, { workflowName, workflowSource, workflowSourceURL }) { ... }@copilot please address this.
| expect(call.body).toContain(pendingPullRequestRun.html_url); | ||
| expect(call.body).toContain("gh-aw-agentic-workflow"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The test for "posts a comment by default" asserts toContain("gh-aw-agentic-workflow") — this is testing a fragment of the attribution footer rather than the semantically meaningful parts of the comment (the run URL, approval verb, PR number). If the footer text changes, the test breaks without any real regression; if the comment omits the run URL entirely the test still passes.
💡 Suggestion
Assert the parts that matter for correctness and user-visible value:
expect(call.body).toContain(pendingPullRequestRun.html_url); // run link is present ✅ (already)
expect(call.body).toMatch(/approved|started/i); // action communicated
// remove the footer-fragment assertion or make it more specific@copilot please address this.
| // pull request associated with the approved run; otherwise read is sufficient. | ||
| pullRequestsLevel := PermissionRead | ||
| if safeOutputs.ApproveWorkflowRun != nil && safeOutputs.ApproveWorkflowRun.Comment { | ||
| pullRequestsLevel = PermissionWrite |
There was a problem hiding this comment.
[/codebase-design] The permission logic in safe_output_handlers.go checks safeOutputs.ApproveWorkflowRun.Comment (the bool field, default zero = false) rather than testing whether the config is nil before dereferencing:
if safeOutputs.ApproveWorkflowRun != nil && safeOutputs.ApproveWorkflowRun.Comment {When Comment defaults to true but is stored as bool (zero = false), and a config struct is constructed without calling parseApproveWorkflowRunConfig, this condition evaluates to false even though the user intent is "default on". The permissions would then be pull-requests: read instead of write, silently under-requesting the scope needed to post the comment at runtime.
This is a direct consequence of the bool vs *bool issue noted on approve_workflow_run.go:9 — fixing that one will fix this too.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds configurable PR notifications after workflow-run approval and adjusts permissions accordingly.
Changes:
- Posts attributed run-started comments by default.
- Adds
comment: falseand least-privilege permissions. - Updates tests, schema, types, and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_permissions_test.go |
Tests conditional PR permissions. |
pkg/workflow/safe_outputs_handler_registry.go |
Emits comment configuration. |
pkg/workflow/safe_output_handlers.go |
Selects read or write permission. |
pkg/workflow/approve_workflow_run.go |
Parses the new option. |
pkg/workflow/approve_workflow_run_test.go |
Tests parsing and defaults. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the schema field. |
docs/src/content/docs/specs/safe-outputs-specification.md |
Updates behavioral specification. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents usage and permissions. |
docs/src/content/docs/reference/glossary.md |
Updates glossary behavior. |
docs/src/content/docs/reference/frontmatter-full.md |
Adds frontmatter reference. |
actions/setup/js/types/safe-outputs-config.d.ts |
Adds the TypeScript option. |
actions/setup/js/approve_workflow_run.test.cjs |
Tests comment behavior. |
actions/setup/js/approve_workflow_run.cjs |
Posts approval comments. |
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 3
- Review effort level: Balanced
| const message = sanitizeContent(getRunStartedMessage({ workflowName, runUrl: runHtmlUrl, eventType: "pull request" })); | ||
| const footer = generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, undefined, pullRequestNumber, undefined, undefined); |
| pullRequestsLevel := PermissionRead | ||
| if safeOutputs.ApproveWorkflowRun != nil && safeOutputs.ApproveWorkflowRun.Comment { | ||
| pullRequestsLevel = PermissionWrite | ||
| } | ||
| return NewPermissionsFromMap(map[PermissionScope]PermissionLevel{ | ||
| PermissionActions: PermissionWrite, | ||
| PermissionPullRequests: PermissionRead, | ||
| PermissionPullRequests: pullRequestsLevel, |
| - `actions: write` - Workflow-run approval | ||
| - `pull-requests: read` - Listing associated pull request files | ||
| - `pull-requests: write` - Posting the run-started comment (when `comment` is enabled, the default); `pull-requests: read` is sufficient when `comment: false` |
There was a problem hiding this comment.
The implementation is clean and correct. Comment building uses the right URL in each position (approved run URL in the message, current agent run URL in the attribution footer), staged mode is unaffected, errors are downgraded to warnings and never fail the approval, and permission escalation is properly conditioned on the comment flag. Tests cover default-on, opt-out, and error-resilience cases.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 33.1 AIC · ⌖ 11.6 AIC · ⊞ 5.7K
|
🎉 This pull request is included in a new release. Release: |
approve-workflow-runapproved pending workflow runs silently, leaving no trace on the associated pull request that the run had started, and always requestedpull-requests: writeregardless of whether write access was actually needed.Comment on approval
New
commentconfig fieldcomment(defaulttrue) to disable this behavior:Least-privilege permissions
pull-requests: writeis now only requested whencommentis enabled (the default).pull-requests: readis used whencomment: false, matching prior behavior.Docs & schema
safe-outputs-pull-requests.md,safe-outputs-specification.md,glossary.md,frontmatter-full.md) to document the new field and permission model.