Ensure extra-empty-commit CI trigger push authenticates as CI token, not persisted checkout credentials#44180
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes CI triggering for the “extra empty commit” path by ensuring the push authenticates using the configured CI trigger token (instead of unintentionally reusing persisted checkout credentials), and adds supporting helpers, tests, and documentation.
Changes:
- Added shared helpers to detect/override/restore persisted
http.<server>/.extraheadergit auth state. - Updated the extra-empty-commit push flow to use a credential-free remote URL while explicitly overriding the persisted extraheader for the push, then restoring it.
- Updated docs and added a changeset describing the CI-trigger auth fix.
Show a summary per file
| File | Description |
|---|---|
docs/src/content/docs/reference/triggering-ci.mdx |
Clarifies expected trigger behavior and troubleshooting guidance for actor identity. |
actions/setup/js/git_auth_helpers.cjs |
Introduces reusable helpers to manage persisted git extraheader credentials. |
actions/setup/js/extra_empty_commit.test.cjs |
Updates tests to validate the new auth override + restore behavior and tokenless remote URL. |
actions/setup/js/extra_empty_commit.cjs |
Switches CI-trigger push to tokenless remote URL + explicit extraheader override/restore. |
actions/setup/js/dynamic_checkout.cjs |
Refactors persisted-extraheader detection to use the shared helper. |
.changeset/giant-hotels-wink.md |
Adds a patch release note for the CI-trigger auth fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Low
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44180 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). The 6 changed files are in .github/skills/, which is outside the default business logic directories. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (4 tests)
Analysis NotesThe single new test ( The three modified tests improve assertion precision ( Inflation check: test file added 26 lines vs. production files (extra_empty_commit.cjs + git_auth_helpers.cjs) added 127 lines → 0.2:1 ratio (well under the 2:1 threshold). Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
|
@copilot run pr-finisher skill |
This comment has been minimized.
This comment has been minimized.
![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 /diagnosing-bugs and /tdd — requesting changes on two correctness gaps in the new git_auth_helpers module and a missing test for the primary bug scenario.
📋 Key Themes & Highlights
Issues Found
-
overridePersistedExtraheadercalled outside thefinally-guarded block (high risk) — if the override itself throws, the outercatchswallows it and returns{ success: false }without ever restoring the previous extraheader, leaving checkout credentials in a broken state. -
Missing test for restore-with-previous-values path (medium risk) — the new test only exercises
--unset-all(no prior extraheader). The primary production scenario — persisted checkout credentials that must be restored after push — has no coverage. -
Silent error swallowing in
overridePersistedExtraheader(low-medium risk) —getExtraheaderValuesreturns[]on any error, so a failure to read previous values is invisible; no warning is logged before unconditionally overwriting the extraheader. -
Undocumented global
execdependency (low risk) — the module is only usable inside agithub-scriptcontext, but nothing in the module communicates this constraint.
Positive Highlights
- ✅ Excellent use of
try/finallyto guarantee cleanup of the temporary remote and auth state in the happy path - ✅ Clean extraction of auth helpers into a shared module eliminates duplication between
dynamic_checkoutandextra_empty_commit - ✅ Switching from token-in-URL to extraheader override correctly solves the duplicate-Authorization-header problem
- ✅ Targeted docs update with concrete troubleshooting step (check
actoridentity) is very helpful
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 110.7 AIC · ⌖ 6.58 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| const unsetCall = execCalls.find(c => c[0] === "git" && c[1] && c[1][0] === "config" && c[1][1] === "--unset-all"); | ||
| expect(unsetCall).toBeDefined(); | ||
| expect(execCalls.indexOf(unsetCall)).toBeGreaterThan(execCalls.indexOf(pushCall)); |
There was a problem hiding this comment.
[/tdd] The test only verifies the --unset-all path (no previous extraheader), so the critical restore-with-existing-credentials path — the main scenario this bug fix protects — is never exercised. A push error while previous checkout credentials are set could silently leave stale CI-token credentials in .git/config.
💡 Suggested addition
it('should restore a previously-set extraheader after push', async () => {
const prevHeader = 'Authorization: basic ' + Buffer.from('x-access-token:old_token').toString('base64');
mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: prevHeader + '\n', stderr: '' });
await pushExtraEmptyCommit({ branchName: 'feature-branch', repoOwner: 'test-owner', repoName: 'test-repo' });
const execCalls = mockExec.exec.mock.calls;
const pushIdx = execCalls.findIndex(c => c[0] === 'git' && c[1][0] === 'push');
const restoreCall = execCalls.find((c, i) => i > pushIdx && c[0] === 'git' && c[1][0] === 'config' && c[1][1] === '--replace-all');
expect(restoreCall).toBeDefined();
expect(restoreCall[1][3]).toBe(prevHeader);
});@copilot please address this.
There was a problem hiding this comment.
Added in commit d20a411 — should restore a previously-set extraheader after push test at line 190 of extra_empty_commit.test.cjs. It mocks getExecOutput to return a previous checkout header, runs pushExtraEmptyCommit, and asserts that a --replace-all call after push carries the original header value.
| const serverHostStripped = githubServerUrl.replace(/^https?:\/\//, ""); | ||
| const remoteUrl = `https://x-access-token:${token}@${serverHostStripped}/${repoOwner}/${repoName}.git`; | ||
| const remoteUrl = `${githubServerUrl}/${repoOwner}/${repoName}.git`; | ||
| const previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token); |
There was a problem hiding this comment.
[/diagnosing-bugs] overridePersistedExtraheader is called before the outer try/catch that converts push errors to { success: false }. If overridePersistedExtraheader itself throws (e.g. git config write fails), the exception propagates to the outermost catch at line 199 — which logs a warning and returns { success: false } without ever restoring the original extraheader. Move the override call inside the try block that has the finally clause, so restoration is guaranteed.
💡 Suggested fix
Change:
const previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token);
try {
// ... remote add, push ...
} finally {
await restorePersistedExtraheader(githubServerUrl, previousExtraheaders);
}To:
let previousExtraheaders = [];
try {
previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token);
// ... remote add, push ...
} finally {
await restorePersistedExtraheader(githubServerUrl, previousExtraheaders);
}This ensures restore runs even if overridePersistedExtraheader partially mutated git config before throwing.
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit d20a411 — previousExtraheaders is now declared as [] before the try block, and overridePersistedExtraheader is called as the first statement inside try, so the finally clause always runs restorePersistedExtraheader regardless of where the error occurs.
| return; | ||
| } | ||
|
|
||
| await exec.exec("git", ["config", "--replace-all", key, previousValues[0]]); |
There was a problem hiding this comment.
[/diagnosing-bugs] When restoring multiple previous values, the code calls --replace-all for the first value and then --add for each subsequent one. However, --replace-all also removes all existing values with that key first — so the first restore call wipes the CI-token extraheader written earlier, then the --add calls stack correctly. This is the intended flow, but it has an edge case: if the first --replace-all call succeeds but a later --add call throws, the extraheader is left with a partial list. Since multi-value extraheaders are unusual in practice, consider a comment documenting this intentional ordering so future maintainers don't accidentally reorder it.
💡 Suggested comment
// First --replace-all removes the CI-token entry and sets previousValues[0].
// Subsequent --add calls stack the remaining previous values onto the same key.
await exec.exec('git', ['config', '--replace-all', key, previousValues[0]]);
for (const value of previousValues.slice(1)) {
await exec.exec('git', ['config', '--add', key, value]);
}@copilot please address this.
There was a problem hiding this comment.
Fixed in commit d20a411. Lines 93–95 of git_auth_helpers.cjs now document the intentional --replace-all/--add ordering: // First --replace-all removes the CI-token entry and sets previousValues[0]. Subsequent --add calls stack the remaining previous values onto the same key without removing any already written.
There was a problem hiding this comment.
Review: Ensure extra-empty-commit CI trigger push authenticates as CI token
Good overall approach — moving from token-in-URL to extraheader override is the correct fix for the duplicate-Authorization problem. The helper extraction into git_auth_helpers.cjs is clean and the finally cleanup path is well-structured.
Two issues need addressing before merge:
Blocking
-
Missing changeset front-matter (
.changeset/giant-hotels-wink.md): The file is missing the required---delimiters that all other changesets use. The changeset parser will silently skip this file and omit the version bump from the release PR. -
Auth state not restored on errors before inner try (
extra_empty_commit.cjsline 151):overridePersistedExtraheadermutates git config outside the innertry/finally. If an error propagates to the outercatchafter the override has already run (e.g.git remote addthrows),restorePersistedExtraheaderis never called and the extraheader is left as the CI trigger token. Wrap the override + inner try/finally in their own outer try/finally to guarantee restoration in all error paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 126.1 AIC · ⌖ 5.46 AIC · ⊞ 4.8K
| @@ -0,0 +1,3 @@ | |||
| "gh-aw": patch | |||
There was a problem hiding this comment.
The changeset file is missing the required --- YAML front-matter delimiters. Every other changeset in this repo uses:
---
"gh-aw": patch
---
Description text...
Without the --- fences, the changeset CLI parser will not recognise the package/bump-type and will treat the entire file as description text. This means the release PR will be generated without the version bump for this fix.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit d20a411. The changeset file now has the required --- YAML front-matter delimiters around the package/bump-type entry.
| const serverHostStripped = githubServerUrl.replace(/^https?:\/\//, ""); | ||
| const remoteUrl = `https://x-access-token:${token}@${serverHostStripped}/${repoOwner}/${repoName}.git`; | ||
| const remoteUrl = `${githubServerUrl}/${repoOwner}/${repoName}.git`; | ||
| const previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token); |
There was a problem hiding this comment.
Auth state not restored if overridePersistedExtraheader succeeds but code between it and the inner try throws.
overridePersistedExtraheader (line 151) mutates git config before the inner try { ... } finally { ... } block (line 153). The finally on line 185 correctly calls restorePersistedExtraheader, but the outer catch (error) on line 199 does not.
If any code were ever inserted between the two lines, or if exec.exec("git remote remove") on line 156 throws in a future refactor, the outer catch would swallow the error and return { success: false } with git config permanently overridden to the CI trigger token.
Wrapping the override + inner try/finally in a single outer try/finally would make the invariant explicit and resilient:
const previousExtraheaders = await overridePersistedExtraheader(githubServerUrl, token);
try {
// ... inner logic ...
return { success: true };
} finally {
// ... remote removal and restorePersistedExtraheader ...
}The outer catch then only handles errors that occur before the auth override (cycle check, etc.).
@copilot please address this.
There was a problem hiding this comment.
Addressed in commit d20a411 — overridePersistedExtraheader is now the first call inside the inner try block (with previousExtraheaders = [] declared before), so the finally clause that calls restorePersistedExtraheader always runs, even if the override itself partially mutates git config before throwing.
|
@copilot please run the
|
…docs wording Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
…, double-normalization, silent flag, and push-failure test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nd improve warning message Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the latest
All 39 tests pass locally ( |
|
@copilot ensure it works for verified commits |
…tra-empty-commit push Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commits. Key details:
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Review summary 🧠Applied 📋 Blocking issues (2)
📋 Non-blocking suggestions (3)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a correctness gap (stale local HEAD after API commit) and a credential-safety issue in the restore path.
📋 Key Themes & Highlights
Key Themes
- Local branch not updated after API commit (
extra_empty_commit.cjs:235): whencreateCommitOnBranchsucceeds, the local branch HEAD stays at the pre-empty-commit SHA. Any downstream git state reads (e.g. the cycle-preventiongit logpath) will see stale data. - Partial restore window in multi-value extraheader restoration (
git_auth_helpers.cjs:103): the--replace-all+--addloop leaves git config in a half-restored state if any--addthrows, silently corrupting checkout credentials. - Missing dedicated unit tests for
git_auth_helpers.cjs: the module is tested only indirectly; multi-value parse/restore edge cases and thegetExtraheaderValuesthrow-path are uncovered. - Unnecessary
ci-triggerremote lifecycle on the GraphQL API path: the remote is created and cleaned up even when nogit pushis ever issued.
Positive Highlights
- ✅ Excellent
try/finallystructure ensuring auth restore even on push failure - ✅ The
overrideAppliedflag correctly guards against spurious--unset-allon early-exit paths - ✅ Comprehensive test suite for the new auth override/restore flow (+219 lines) with meaningful names that read as specifications
- ✅ Clean extraction of
checkoutHasPersistedExtraheaderinto the shared helpers module — DRY, consistent with the rest of the codebase - ✅ GraphQL fallback path is well-guarded and degrades gracefully
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 103.9 AIC · ⌖ 9.05 AIC · ⊞ 6.6K
Comment /matt to run again
|
@copilot review comments in #44180 (review) |
…uard, dedicated tests, remote lifecycle Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all four issues from review 4659378797 in commit
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
🎉 This pull request is included in a new release. Release: |
The extra-empty-commit CI trigger path was silently pushing as
github-actions[bot]when checkout persistedhttp.<server>/.extraheader, soGH_AW_CI_TRIGGER_TOKEN/github-token-for-extra-empty-commitdid not reliably trigger downstream CI. This change makes the trigger push explicitly use the configured CI token identity and restores prior git auth state afterward.Auth correctness for extra-empty-commit push
actions/setup/js/extra_empty_commit.cjsnow:${GITHUB_SERVER_URL}/${owner}/${repo}.git)http.<server>/.extraheaderviagit config --replace-allwith the CI trigger token before pushnewCommitCount === 1, and fetch/reset sync.Shared git auth helper extraction
actions/setup/js/git_auth_helpers.cjswith reusable helpers:checkoutHasPersistedExtraheaderoverridePersistedExtraheaderrestorePersistedExtraheaderactions/setup/js/dynamic_checkout.cjsnow importscheckoutHasPersistedExtraheaderfrom this shared module.Targeted docs update
docs/src/content/docs/reference/triggering-ci.mdxnow clarifies trigger behavior even with persisted checkout credentials and adds troubleshooting guidance to verify the runactoris the PAT/App identity (notgithub-actions[bot]).Release note