feat: add update-branch support to safe-outputs update-pull-request#27244
Merged
feat: add update-branch support to safe-outputs update-pull-request#27244
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/340b546b-743d-45b6-ac7e-8ec5735124d3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/340b546b-743d-45b6-ac7e-8ec5735124d3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/340b546b-743d-45b6-ac7e-8ec5735124d3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/340b546b-743d-45b6-ac7e-8ec5735124d3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
April 20, 2026 00:04
View session
pelikhan
reviewed
Apr 20, 2026
|
|
||
| if (mergeBase) { | ||
| core.info(`Updating pull request #${prNumber} branch with base branch changes`); | ||
| await github.rest.pulls.updateBranch({ |
Collaborator
There was a problem hiding this comment.
@copilot this api requires permissions: content:write
Add logging of operation fails, withRetry
Contributor
Author
There was a problem hiding this comment.
Implemented in 37b38f0.
- Added retry + transient-error handling for
pulls.updateBranchusingwithRetry. - Added explicit warning logging when the merge-base branch update operation fails.
- Updated safe-output permissions for
update-pull-requestto requirecontents: write(withpull-requests: write) and added a Go test covering this permission requirement. - Added JS tests for transient retry and failure logging behavior.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/64e4d53e-e239-416e-9bf1-20a33e83b86b Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 73/100
Test Classification DetailsView all 7 test classifications
Flagged Tests — Suggestions for Improvement
|
| Test file | Lines added | Production file | Prod lines added | Ratio |
|---|---|---|---|---|
update_pull_request.test.cjs |
86 | update_pull_request.cjs |
27 | ~3.2:1 |
compiler_safe_outputs_config_test.go |
59 | compiler_safe_outputs_handlers.go |
1 | ~59:1 |
safe_output_validation_config_test.go |
16 | safe_outputs_validation_config.go |
3 | ~5.3:1 |
The compiler_safe_outputs_config_test.go ratio looks extreme but is acceptable: the tested method (addHandlerManagerConfigEnvVar) required an end-to-end JSON parsing chain in the test, which is inherently verbose.
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 3 test functions — all unit (//go:build !integration) ✓ - 🟨 JavaScript (
*.test.cjs): 4 tests (vitest) ✓
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All tests verify observable behavioral contracts. Build tags are correct; no mock-library violations found. The main suggestion is to add error-path coverage for
updateBranchfailure scenarios in the JavaScript tests.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §24642644338
🧪 Test quality analysis by Test Quality Sentinel · ● 809.1K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 73/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 7 analyzed tests verify behavioral contracts. No coding-guideline violations detected (build tags ✓, no mock libraries ✓). See the comment above for improvement suggestions around error-path coverage.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional merge-base support to the safe-outputs.update-pull-request tool, enabling PR branch updates via GitHub’s pulls.updateBranch API (including merge-only updates), and propagates the new flag through config/schema/docs.
Changes:
- Added
merge-base(merge_base) to safe-outputs update-pull-request config parsing, handler config generation, and validation/schema/tooling. - Updated the
update_pull_requestJS handler to callpulls.updateBranchwhenmerge_baseis enabled and allow merge-only runs. - Added/updated tests, docs, and generated lock/tool outputs impacted by the schema/config updates.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/update_pull_request_helpers.go | Adds merge-base to YAML config struct and parsing. |
| pkg/workflow/safe_outputs_validation_config.go | Extends update_pull_request validation to allow merge_base as an update trigger. |
| pkg/workflow/safe_output_validation_config_test.go | Adds test coverage for the updated validation rule/field. |
| pkg/workflow/js/safe_outputs_tools.json | Updates tool schema to include merge_base. |
| pkg/workflow/compiler_safe_outputs_handlers.go | Passes merge_base into generated handler config. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Adds test for handler config emission of merge_base. |
| pkg/parser/schemas/main_workflow_schema.json | Adds merge-base to the workflow schema. |
| docs/src/content/docs/reference/frontmatter-full.md | Documents merge-base and reflects regenerated reference content. |
| actions/setup/js/update_pull_request.cjs | Implements merge_base behavior via pulls.updateBranch; supports merge-only updates. |
| actions/setup/js/update_pull_request.test.cjs | Adds JS tests for merge-base config inheritance and runtime behavior. |
| actions/setup/js/types/safe-outputs.d.ts | Extends TS typings for merge_base. |
| actions/setup/js/safe_outputs_tools.json | Updates runtime tool schema to include merge_base. |
| .github/workflows/smoke-claude.lock.yml | Regenerated lock output including updated handler config/validation schema. |
| .github/workflows/changeset.lock.yml | Regenerated lock output including updated handler config/validation schema. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/16 changed files
- Comments generated: 2
Comment on lines
+1240
to
+1261
| for _, step := range steps { | ||
| if strings.Contains(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG") { | ||
| parts := strings.Split(step, "GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ") | ||
| if len(parts) == 2 { | ||
| jsonStr := strings.TrimSpace(parts[1]) | ||
| jsonStr = strings.Trim(jsonStr, "\"") | ||
| jsonStr = strings.ReplaceAll(jsonStr, "\\\"", "\"") | ||
|
|
||
| var config map[string]map[string]any | ||
| err := json.Unmarshal([]byte(jsonStr), &config) | ||
| require.NoError(t, err) | ||
|
|
||
| updatePRConfig, ok := config["update_pull_request"] | ||
| require.True(t, ok, "Expected update_pull_request config") | ||
|
|
||
| mergeBaseValue, ok := updatePRConfig["merge_base"] | ||
| require.True(t, ok, "Expected merge_base key in update_pull_request config") | ||
| assert.Equal(t, tt.expected, mergeBaseValue) | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
This test never fails if the GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG line isn’t found in steps (the loop has no assertion after it). If addHandlerManagerConfigEnvVar stops emitting the handler config, the test could still pass.
Track a found boolean and require.True(t, found) (or similar) after the loop to ensure the JSON was actually located and parsed before asserting on merge_base.
Comment on lines
155
to
163
| "update_pull_request": { | ||
| DefaultMax: 1, | ||
| CustomValidation: "requiresOneOf:title,body", | ||
| CustomValidation: "requiresOneOf:title,body,merge_base", | ||
| Fields: map[string]FieldValidation{ | ||
| "title": {Type: "string", Sanitize: true, MaxLength: 256}, | ||
| "body": {Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | ||
| "operation": {Type: "string", Enum: []string{"replace", "append", "prepend"}}, | ||
| "merge_base": {Type: "boolean"}, | ||
| "draft": {Type: "boolean"}, |
There was a problem hiding this comment.
update_pull_request now uses requiresOneOf:title,body,merge_base, but the JS validator’s requiresOneOf check treats any non-undefined value as satisfying the rule. This means an item like { "merge_base": false } (or a templated boolean that resolves to false) will pass validation even though it results in a no-op update.
Consider introducing a custom validation that treats boolean fields as satisfied only when true (e.g., requiresOneOfTruthy) or updating the validator logic for requiresOneOf to ignore false values for boolean fields, so merge-base-only updates require merge_base: true.
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Collaborator
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2b4ea663-bae9-4763-b9ed-30316e844c32 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Collaborator
|
@copilot contents: write is needed ONLY if merge-base is true |
Collaborator
|
@copilot rename "merge-base" to "update-branch" to match api name |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2b4ea663-bae9-4763-b9ed-30316e844c32 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Reviewed and addressed all actionable review comments in commit
No UI changes were made, so there is no UI screenshot. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fad66084-394b-4a19-b77a-f699f706efe7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/fad66084-394b-4a19-b77a-f699f706efe7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Implemented in
Updated permission computation, tests, and recompiled lock outputs accordingly. No UI changes in this update.
Implemented in Renamed the option from No UI changes in this update. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
feat: add merge-base support to safe-outputs update-pull-request
feat: add update-branch support to safe-outputs update-pull-request
Apr 20, 2026
Contributor
|
Hey This PR looks ready for maintainer review. 🟢
|
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
update-branchboolean tosafe-outputs.update-pull-request(defaultfalse)merge-base/merge_basenaming toupdate-branch/update_branchacross config parsing, runtime handler config, validation, schemas/types, docs, and compiled lock outputsupdate_pull_requestruntime handler to call GitHub RESTpulls.updateBranchwhenupdate_branchis enabledupdate-pull-requestpermissions conditional:contents: writeonly whenupdate-branchis enabledcontents: readwithpull-requests: writerequiresOneOfvalidation does not treat booleanfalseas satisfying update requirementsTesting
make fmtmake recompilego test -v -run "TestUpdatePullRequestUpdateBranchHandlerConfig|TestUpdatePullRequestValidationConfig|TestValidationConfigConsistency|TestComputePermissionsForSafeOutputs" ./pkg/workflow/cd actions/setup/js && npm run test -- safe_output_type_validator.test.cjs update_pull_request.test.cjsmake agent-finish(fails on existing repository-wide gosec findings unrelated to this change, same as baseline)Validation
parallel_validationrun: