feat: propagate OAuth token check failure to conclusion job failure issue#44756
Conversation
…ssue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ure propagation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44756 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (55 additions). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This pull request ensures that when the activation job fails due to detecting an OAuth token (gho_...) in configured secrets, that failure signal is propagated so the conclusion job still runs and produces an actionable failure issue/comment with remediation guidance.
Changes:
- Persist
oauth_token_check_failed=trueto$GITHUB_OUTPUTbefore failing the OAuth token check step so downstream jobs can detect the failure. - Plumb
oauth_token_check_failedthrough activation job outputs into the conclusion job condition and into the failure handler env/context. - Add a dedicated remediation template (
oauth_token_check_failed.md) and wire it into the agent failure issue/comment rendering; recompile updated.lock.ymlworkflows.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/check_oauth_tokens.sh |
Writes an output flag before exiting non-zero so the step failure can be detected by downstream job logic. |
pkg/workflow/compiler_activation_job_builder.go |
Exposes oauth_token_check_failed as an activation job output derived from the check-oauth-tokens step output. |
pkg/workflow/compiler_activation_jobs_test.go |
Adds unit coverage asserting the activation job exports the new oauth_token_check_failed output reference. |
pkg/workflow/notify_comment_conclusion_helpers.go |
Ensures the conclusion job runs when OAuth token check fails and passes GH_AW_OAUTH_TOKEN_CHECK_FAILED into the failure handler. |
actions/setup/js/handle_agent_failure.cjs |
Adds a new failure category + context builder and injects {oauth_token_check_failed_context} into rendered issue/comment templates and guard logic. |
actions/setup/md/oauth_token_check_failed.md |
New remediation template shown in the failure issue/comment when an OAuth token is detected. |
actions/setup/md/agent_failure_issue.md |
Adds {oauth_token_check_failed_context} placeholder to the issue template. |
actions/setup/md/agent_failure_comment.md |
Adds {oauth_token_check_failed_context} placeholder to the comment template. |
.github/workflows/*.lock.yml (many files) |
Recompiled workflows to include the new activation output, updated conclusion-job if condition, and new env var wiring. |
.changeset/patch-propagate-oauth-token-check-failure.md |
Documents the patch-level change in behavior for release notes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 270/270 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: OAuth Token Check Failure Propagation
The implementation is correct and well-structured. It follows the existing patterns for activation guardrail failures (lockdown check, stale lock file) consistently across all layers:
check_oauth_tokens.sh: correctly writes output beforeexit 1so the value survives step failurecompiler_activation_job_builder.go: step IDcheck-oauth-tokensmatches the step definitionnotify_comment_conclusion_helpers.go:activationGuardrailsFailedcondition correctly ORs in the new check; env var propagation follows the patternhandle_agent_failure.cjs: early-return guard, title, category, context builder, template rendering, and module export are all wired consistentlyoauth_token_check_failed.md: clear remediation template with{run_url}placeholder- Tests cover the new
buildOAuthTokenCheckFailedContextfunction thoroughly
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 38.8 AIC · ⌖ 4.34 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on two test coverage gaps; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Signal propagation is correct end-to-end: The fix properly wires
oauth_token_check_failedfromcheck_oauth_tokens.sh→ activation job output → conclusion jobifcondition →GH_AW_OAUTH_TOKEN_CHECK_FAILEDenv var →handle_agent_failure.cjs. The pattern is consistent with the existinglockdown_check_failedandstale_lock_file_failedpeers. - Two test coverage gaps identified: (1)
buildConclusionJobConditionhas no unit test asserting the compiledifstring includesoauth_token_check_failed; (2) the JS failure handler has no test asserting the absent-env-var-means-false default. Neither is a blocker, but both would prevent silent regression.
Positive Highlights
- ✅
check_oauth_tokens.shcorrectly writes the output beforeexit 1, so the signal is available to downstream jobs - ✅ The
=== 'true'guard inhandle_agent_failure.cjssafely handles absent values - ✅ Remediation template (
oauth_token_check_failed.md) is clear and actionable - ✅ The JS tests added for
buildOAuthTokenCheckFailedContextare thorough - ✅ Go test for the activation job output wiring is in place
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 82.9 AIC · ⌖ 4.68 AIC · ⊞ 6.6K
Comment /matt to run again
| oauthTokenCheckFailed := BuildEquals(BuildPropertyAccess(fmt.Sprintf("needs.%s.outputs.oauth_token_check_failed", constants.ActivationJobName)), BuildStringLiteral("true")) | ||
| staleLockFileFailed := BuildEquals(BuildPropertyAccess(fmt.Sprintf("needs.%s.outputs.stale_lock_file_failed", constants.ActivationJobName)), BuildStringLiteral("true")) | ||
| activationGuardrailsFailed := BuildOr(lockdownCheckFailed, staleLockFileFailed) | ||
| activationGuardrailsFailed := BuildOr(lockdownCheckFailed, BuildOr(oauthTokenCheckFailed, staleLockFileFailed)) |
There was a problem hiding this comment.
[/tdd] The buildConclusionJobCondition function now includes oauth_token_check_failed in activationGuardrailsFailed, but there is no unit test asserting the compiled if string contains this signal.\n\n
💡 Suggested test
\n\nAdd a test alongside the existing activation output test:\n\ngo\nfunc TestBuildConclusionJobCondition_OAuthTokenCheckFailed(t *testing.T) {\n data := \&WorkflowData{Name: "Test", MarkdownContent: "# Test\n\nContent"}\n cond := buildConclusionJobCondition(data, "agent", nil)\n rendered := RenderCondition(cond)\n assert.Contains(t, rendered, "oauth_token_check_failed",\n "conclusion if condition should fire when OAuth token check fails")\n}\n\n\nWithout this, a future refactor could silently drop the signal — exactly the regression this PR was created to prevent.\n\n| // OAuth token check failure from the activation job — set when check_oauth_tokens.sh detects | ||
| // a gho_... OAuth token in COPILOT_GITHUB_TOKEN, GH_AW_GITHUB_TOKEN, or GH_AW_GITHUB_MCP_SERVER_TOKEN. | ||
| // The agent is skipped in this case; the conclusion job runs to surface remediation guidance. | ||
| const hasOAuthTokenCheckFailed = process.env.GH_AW_OAUTH_TOKEN_CHECK_FAILED === "true"; |
There was a problem hiding this comment.
[/diagnosing-bugs] The GH_AW_OAUTH_TOKEN_CHECK_FAILED env var is only passed to the conclusion job when needs.activation.outputs.oauth_token_check_failed is 'true'. If the activation job itself is skipped (e.g. due to a runner setup failure before this step), the output would be absent rather than false — the === 'true' guard in handle_agent_failure.cjs handles this correctly. This is fine, but worth a comment in the test to lock the expectation.\n\n
💡 Suggested assertion in JS test
\n\nInhandle_agent_failure.test.cjs, add a test covering the missing env var case:\n\njs\nit('treats missing GH_AW_OAUTH_TOKEN_CHECK_FAILED env var as false', () => {\n delete process.env.GH_AW_OAUTH_TOKEN_CHECK_FAILED;\n // Should not trigger oauth_token_check_failed handling\n expect(process.env.GH_AW_OAUTH_TOKEN_CHECK_FAILED === 'true').toBe(false);\n});\n\n\nThis pins the safe default and avoids future confusion if someone changes the absent-value handling.\n\n
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
There was a problem hiding this comment.
⚠️ REQUEST_CHANGES — one high-severity correctness issue, plus defensive hardening needed
The overall approach is sound: writing oauth_token_check_failed=true to GITHUB_OUTPUT before exit 1, propagating it through activation job outputs, extending the conclusion job condition, and threading the new env var into handle_agent_failure.cjs is all correct. However, there is one high-severity issue that can silently defeat the entire mechanism.
🔴 Blocking issue
GITHUB_OUTPUT write can be silently skipped (check_oauth_tokens.sh line 59): the $GITHUB_STEP_SUMMARY redirect at line 45 runs first under set -e. If that write fails (unset variable, read-only path, test harness without GHA environment), the script aborts before the GITHUB_OUTPUT write, the output is never set, the conclusion job condition evaluates false, and the user receives no actionable feedback — exactly the regression this PR exists to fix.
Fix: move echo "oauth_token_check_failed=true" >> "$GITHUB_OUTPUT" to before the summary block, or add || true on the summary redirect.
🟡 Non-blocking: defensive guard gaps in handle_agent_failure.cjs
The hasCompletedDespiteJobFailure guard (line 3115) and hasOnlyNoopOutputs guard (line 3105) do not explicitly exclude hasOAuthTokenCheckFailed. Both are safe today because the agent is skipped when the OAuth check fails, so those paths are unreachable. However, this relies on implicit coupling that is non-obvious and fragile against future step-ordering changes. Adding && !hasOAuthTokenCheckFailed to both guards would make the intent explicit.
🔎 Code quality review by PR Code Quality Reviewer · 176.3 AIC · ⌖ 4.67 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/sh/check_oauth_tokens.sh:59
GITHUB_OUTPUT write silently skipped if GITHUB_STEP_SUMMARY write fails under set -e: the redirect at line 45 runs before line 59; if $GITHUB_STEP_SUMMARY is unset/invalid, set -e aborts the script before oauth_token_check_failed=true is written — conclusion job gets no signal and the user sees no failure issue.
<details>
<summary>💡 Suggested fix</summary>
Write the output flag first, then guard the summary write with || true:
if [[ "$token_value" == gho_* ]]; then…
</details>
<details><summary>actions/setup/js/handle_agent_failure.cjs:3115</summary>
**`hasCompletedDespiteJobFailure` early-return guard does not exclude `hasOAuthTokenCheckFailed`**: if step ordering ever changes so the agent runs after an OAuth check failure, this guard would suppress the failure issue silently.
<details>
<summary>💡 Suggested fix</summary>
Add the guard explicitly for defensive consistency with the other exclusions:
```js
if (hasCompletedDespiteJobFailure && !hasReportIncomplete && !hasCacheMissMisconfiguration && !hasOAuthTokenCheckFailed) {Simila…
|
🎉 This pull request is included in a new release. Release: |
When a workflow is configured with an OAuth token (
gho_...), the activation job fails atcheck-oauth-tokensbut the conclusion job never ran — leaving the user with no actionable feedback. The conclusion job condition only fired whenagent.result != 'skipped'or specific activation guardrails were set; OAuth token failure was neither.Changes
check_oauth_tokens.sh— Writeoauth_token_check_failed=trueto$GITHUB_OUTPUTbeforeexit 1so the output survives the step failurecompiler_activation_job_builder.go— Addoauth_token_check_failedto activation job outputs, referencing the new step outputnotify_comment_conclusion_helpers.go— Extend the conclusion jobifcondition with|| needs.activation.outputs.oauth_token_check_failed == 'true'; passGH_AW_OAUTH_TOKEN_CHECK_FAILEDto thehandle_agent_failurestephandle_agent_failure.cjs— Parse the new env var, addoauth_token_check_failedfailure category, addbuildOAuthTokenCheckFailedContext(), include in both issue/comment template renders and the early-return guardoauth_token_check_failed.md(new) — Error message template with remediation steps (create fine-grained PAT, update secret)agent_failure_issue.md/agent_failure_comment.md— Add{oauth_token_check_failed_context}placeholder between{lockdown_check_failed_context}and{stale_lock_file_failed_context}