feat: add commit-id config option to pin review attribution to the reviewed commit#48738
commit-id config option to pin review attribution to the reviewed commit#48738Conversation
…reate-pull-request-review-comment
Adds a `commit-id` configuration option to both `submit-pull-request-review`
and `create-pull-request-review-comment` safe output handlers. When set, the
configured SHA is used as the `commit_id` for `pulls.createReview()` instead
of the live PR head SHA fetched during `safe_outputs`.
This prevents attribution drift in workflow_run scenarios where a new commit
may be pushed to the PR during the agent's run. Workflow authors can pin the
review to the exact commit that was checked out and reviewed:
```yaml
safe-outputs:
submit-pull-request-review:
commit-id: ${{ needs.eligibility.outputs.head_sha }}
```
Changes:
- Go compiler: add CommitId field to SubmitPullRequestReviewConfig and
CreatePullRequestReviewCommentsConfig, parse commit-id from YAML config,
emit commit_id in handler registry JSON
- JS runtime: read config.commit_id in submit_pr_review.cjs and
create_pr_review_comment.cjs, store as commitId on ReviewContext
- JS buffer: use reviewContext.commitId || pullRequest.head.sha as
commit_id when calling pulls.createReview()
- Tests: add unit tests for all new behavior in Go and JavaScript
Closes #48661
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
commit-id config option to pin review attribution to the reviewed commit
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds configurable commit pinning so pull request reviews target the commit actually reviewed instead of a newer PR head.
Changes:
- Adds
commit-idcompiler configuration and runtime propagation. - Uses the pinned SHA when submitting reviews.
- Adds Go and JavaScript tests for commit selection.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/submit_pr_review.go |
Parses review commit configuration. |
pkg/workflow/submit_pr_review_footer_test.go |
Tests compiler configuration handling. |
pkg/workflow/safe_outputs_handler_registry.go |
Emits commit IDs to handlers. |
pkg/workflow/create_pr_review_comment.go |
Parses comment commit configuration. |
actions/setup/js/submit_pr_review.test.cjs |
Tests submit-handler pinning. |
actions/setup/js/submit_pr_review.cjs |
Propagates pinned review commits. |
actions/setup/js/pr_review_buffer.test.cjs |
Tests commit selection during submission. |
actions/setup/js/pr_review_buffer.cjs |
Submits reviews against pinned commits. |
actions/setup/js/create_pr_review_comment.test.cjs |
Tests comment-handler pinning. |
actions/setup/js/create_pr_review_comment.cjs |
Propagates pins into comment buffers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 6
- Review effort level: Medium
| // Parse optional commit-id override | ||
| if commitId, exists := configMap["commit-id"]; exists { | ||
| if commitIdStr, ok := commitId.(string); ok && commitIdStr != "" { | ||
| config.CommitId = commitIdStr |
| // Parse optional commit-id override | ||
| if commitId, exists := configMap["commit-id"]; exists { | ||
| if commitIdStr, ok := commitId.(string); ok && commitIdStr != "" { | ||
| prReviewCommentsConfig.CommitId = commitIdStr |
| // Use the pinned commit ID when configured, falling back to the live PR head SHA. | ||
| // A pinned commitId ensures the review is attributed to the commit the agent actually | ||
| // reviewed, preventing attribution drift when new commits are pushed during the run. | ||
| const resolvedCommitId = commitId || pullRequest.head.sha; |
| owner: repoParts.owner, | ||
| repo: repoParts.repo, | ||
| pull_number: pullRequestNumber, | ||
| commit_id: pullRequest.head.sha, | ||
| commit_id: resolvedCommitId, |
| // Optional pinned commit SHA. When set, overrides the live PR head SHA so that the | ||
| // review is attributed to the commit the agent actually reviewed, not whatever head | ||
| // the PR has at post time (which may differ under workflow_run triggers). | ||
| const pinnedCommitId = typeof config.commit_id === "string" && config.commit_id.trim() ? config.commit_id.trim() : null; |
| // Optional pinned commit SHA. When set, overrides the live PR head SHA so that the | ||
| // review is attributed to the commit the agent actually reviewed, not whatever head | ||
| // the PR has at post time (which may differ under workflow_run triggers). | ||
| const pinnedCommitId = typeof config.commit_id === "string" && config.commit_id.trim() ? config.commit_id.trim() : null; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: add commit-id config option
The implementation is well-structured and the test coverage is thorough. One blocking issue found:
Go naming convention violation — CommitId should be CommitID (Go requires initialisms to be fully uppercased per the Go wiki). This affects:
CommitIdfield insubmit_pr_review.goandcreate_pr_review_comment.go- Local variables
commitId/commitIdStrin both Go files - Test assertions using
CommitIdinsubmit_pr_review_footer_test.go
Everything else — the JS runtime changes, the fallback logic, the test cases — looks correct and well thought-out.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.6 AIC · ⌖ 4.67 AIC · ⊞ 5K
| Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review body: "always" (default), "none", or "if-body" (only when review has body text) | ||
| AllowedEvents []string `yaml:"allowed-events,omitempty"` // Optional list of allowed review event types: APPROVE, COMMENT, REQUEST_CHANGES. If omitted, all event types are allowed. | ||
| SupersedeOlderReviews bool `yaml:"supersede-older-reviews,omitempty"` // When true, dismisses older same-workflow REQUEST_CHANGES reviews after a replacement review is posted. | ||
| CommitId string `yaml:"commit-id,omitempty"` // Optional commit SHA to use as the reviewed commit. Pins the review to the commit the agent actually reviewed, preventing attribution drift when new commits are pushed during the run. |
There was a problem hiding this comment.
Go naming convention: CommitId → CommitID
Go naming conventions require initialisms (ID, URL, HTTP, SHA) be fully uppercased. CommitId should be CommitID. The same applies to the identical field in create_pr_review_comment.go and local variables commitId/commitIdStr.
This will be flagged by golint/staticcheck.
// current
CommitId string `yaml:"commit-id,omitempty"`
// correct
CommitID string `yaml:"commit-id,omitempty"`@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes — the commit-id pinning applies a single global value to every PR in multi-PR/registry mode, mislabeling attribution instead of fixing it.
💡 Themes and rationale
The core idea (pin commit_id to the SHA the agent actually reviewed) is sound and well-tested for the single-PR case. However, both submit_pr_review.cjs and create_pr_review_comment.cjs support multi-PR registry mode (target: "*"), and in that mode the same workflow-level commit-id string is spread into every PR's buffer context regardless of which PR is being processed. Since commit-id is intended to come from a single upstream job output tied to one specific PR/run, applying it uniformly across multiple PRs in one workflow invocation will silently attribute reviews/comments to the wrong commit for every PR except the one the value was actually computed for — this is worse than the bug the PR is fixing, and it is untested (the only registry-mode test with commit_id uses a single PR).
Additionally, there's no validation that commit-id is a plausible SHA or belongs to the target PR's history before being sent to the GitHub API, so a stale/misconfigured upstream value fails opaquely at request time rather than with a clear config error.
Recommend: gate commit-id pinning to single-PR-target configurations (warn/skip in registry multi-PR mode), or support a per-PR mapping, plus add multi-PR regression tests before merge.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 52.3 AIC · ⌖ 7.26 AIC · ⊞ 7.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/submit_pr_review.cjs:287
A single workflow-level commit-id is pinned identically onto every PR resolved via target: "*", silently attributing the wrong commit to all but one PR when multiple PRs are reviewed in the same run.
<details><summary>💡 Details and fix</summary>
pinnedCommitId is computed once from config.commit_id (a single string) and spread unconditionally into every buffer's review context in both setReviewContextOnBuffer call sites (payload PR and fetched PR), regardless of which PR is being p…
actions/setup/js/create_pr_review_comment.cjs:25
Same global-pinning bug as submit_pr_review.cjs: one commit-id is applied to every PR comment buffer in multi-PR/registry mode, mislabeling comments on all but one PR.
<details><summary>💡 Details and fix</summary>
This file's test suite (see describe("create_pr_review_comment.cjs — registry mode (multiple reviews)")) confirms multi-PR usage is supported, yet pinnedCommitId is derived once from top-level config.commit_id and unconditionally spread into every buffer's review context …
pkg/workflow/submit_pr_review.go:430
No validation that commit-id is a well-formed SHA or actually reachable on the target PR before it flows into pulls.createReview().
<details><summary>💡 Details and fix</summary>
The parser accepts any non-empty string via configMap["commit-id"] with only a non-empty check, no format validation (e.g. 40/7-char hex) and no ancestry check against the PR branch. Since this value is expected to come from expressions like ${{ needs.eligibility.outputs.head_sha }}, a misconfigured or stale …
🧪 Test Quality Sentinel Report
📊 Metrics (17 tests)
Test Breakdown
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (194 new lines across 📄 Draft ADR committed:
📋 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 ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting changes on a naming violation and a docs gap.
📋 Key Themes & Highlights
Key Themes
- Go naming (
CommitId→CommitID): Bothsubmit_pr_review.goandcreate_pr_review_comment.gouseCommitIdwhere Go conventions requireCommitID. This will triggergolint/revivefindings. - Duplicated parsing logic: The
commit-idextraction block is copy-pasted verbatim into two separate parse functions — a shared helper would prevent drift. - Missing reference docs:
commit-idis a new user-facing YAML option but isn't documented in the spec or glossary alongside the other options (allowed-events,supersede-older-reviews).
Positive Highlights
- ✅ Excellent test coverage — 408 lines of new tests across all code paths (payload PR, fetched PR, registry mode, empty config, whitespace-only config)
- ✅
setReviewContextis correctly write-once, so there's no risk of a later handler call overwriting a previously pinned commit ID - ✅
AddIfNotEmptycorrectly gates emission so emptycommit_idis never sent to the JS runtime - ✅ Fallback to
pullRequest.head.shais clean and well-commented - ✅ The PR description is clear and the
workflow_runrace condition is well-explained
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 82.5 AIC · ⌖ 7.02 AIC · ⊞ 6.7K
Comment /matt to run again
| Footer *string `yaml:"footer,omitempty"` // Controls when to show footer in PR review body: "always" (default), "none", or "if-body" (only when review has body text) | ||
| AllowedEvents []string `yaml:"allowed-events,omitempty"` // Optional list of allowed review event types: APPROVE, COMMENT, REQUEST_CHANGES. If omitted, all event types are allowed. | ||
| SupersedeOlderReviews bool `yaml:"supersede-older-reviews,omitempty"` // When true, dismisses older same-workflow REQUEST_CHANGES reviews after a replacement review is posted. | ||
| CommitId string `yaml:"commit-id,omitempty"` // Optional commit SHA to use as the reviewed commit. Pins the review to the commit the agent actually reviewed, preventing attribution drift when new commits are pushed during the run. |
There was a problem hiding this comment.
[/codebase-design] CommitId violates Go naming conventions — initialisms must be fully uppercase. The Go team's [naming guide]((go.dev/redacted) and golint/revive both flag Id as incorrect; it should be CommitID.
💡 Suggested fix
Rename in both structs and all references:
// submit_pr_review.go and create_pr_review_comment.go
CommitID string `yaml:"commit-id,omitempty"`Also update safe_outputs_handler_registry.go (c.CommitId → c.CommitID) and local variables in the two parseXxx functions (commitId → commitID). Short mechanical rename but needed to stay lint-clean.
@copilot please address this.
| } | ||
|
|
||
| // Parse optional commit-id override | ||
| if commitId, exists := configMap["commit-id"]; exists { |
There was a problem hiding this comment.
[/codebase-design] The commit-id parsing block is copy-pasted verbatim in both parseSubmitPullRequestReviewConfig and parsePullRequestReviewCommentsConfig. Duplicated parsing logic is a maintenance hazard — a future change (e.g., validation, normalisation) would need to be applied in two places.
💡 Suggested refactor
Extract a small helper:
// parseOptionalCommitID extracts and validates the commit-id config field.
func parseOptionalCommitID(configMap map[string]any, log *log.Logger) string {
raw, exists := configMap["commit-id"]
if !exists {
return ""
}
s, ok := raw.(string)
if !ok || s == "" {
return ""
}
log.Printf("Commit ID override: %s", s)
return s
}Then both parsers become a single call site:
config.CommitID = parseOptionalCommitID(configMap, submitPRReviewLog)@copilot please address this.
| // Optional pinned commit SHA. When set, overrides the live PR head SHA so that the | ||
| // review is attributed to the commit the agent actually reviewed, not whatever head | ||
| // the PR has at post time (which may differ under workflow_run triggers). | ||
| const pinnedCommitId = typeof config.commit_id === "string" && config.commit_id.trim() ? config.commit_id.trim() : null; |
There was a problem hiding this comment.
[/grill-with-docs] The commit-id option is user-facing YAML configuration but has no entry in docs/src/content/docs/specs/safe-outputs-specification.md (where allowed-events, supersede-older-reviews, etc. are documented, see line 1648). Without docs, users discovering this feature must read the source or the PR description.
💡 What to add
In the spec, under submit-pull-request-review fields and create-pull-request-review-comment fields, add:
- `commit-id` *(optional)* — SHA of the commit the agent actually reviewed. When set, the review is pinned to this commit instead of the live PR head SHA at post time. Useful under `workflow_run` triggers where a new commit may land between the agent run and the review post. GitHub will mark the review as outdated if HEAD has since moved — which is the correct behavior.Also consider adding a glossary entry alongside supersede-older-reviews (line 406 of glossary.md).
@copilot please address this.
Under
workflow_runtriggers,safe_outputsruns afteragent— if a new commit lands during the agent's run,pulls.get()returns the new HEAD sha and the posted review is attributed to a commit the agent never saw, making inline comments appear fabricated or stale.Changes
Go compiler
CommitId stringfield (commit-idin YAML) toSubmitPullRequestReviewConfigandCreatePullRequestReviewCommentsConfigcommit_idviaAddIfNotEmptyinto the runtime JSON config for both handlersJS runtime
pr_review_buffer.cjs: ExtendedReviewContextwith optionalcommitId;submitReview()usesreviewContext.commitId || pullRequest.head.shaascommit_idinpulls.createReview()submit_pr_review.cjs: Readsconfig.commit_idaspinnedCommitIdand threads it through both the payload and fetched-PR paths ofsetReviewContextOnBuffercreate_pr_review_comment.cjs: Same — readsconfig.commit_idand passescommitIdtobuffer.setReviewContext()Usage
When
commit-idis set, the review is pinned to that SHA. GitHub will automatically mark the review as outdated if HEAD has since moved — which is the correct behavior — rather than attributing a stale review to the new commit.commit_idis the head at post time, not the commit the agent reviewed #48661