fix(safe-outputs): fall back to env vars when create_report_incomplete_issue omits workflow_name - #46400
Conversation
|
Hey
If you'd like a hand, you can assign this prompt to your coding agent:
|
…report_incomplete_issue messages Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds deployment-context fallbacks so incomplete-report safe outputs are not rejected when agents omit workflow metadata.
Changes:
- Falls back to safe-output job environment metadata.
- Adds regression tests for fallback and precedence behavior.
- Regenerates the auto-upgrade cron schedule.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/missing_issue_helpers.cjs |
Resolves missing workflow metadata from environment variables. |
actions/setup/js/missing_issue_helpers.test.cjs |
Tests metadata fallback and precedence. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly auto-upgrade schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
|
@copilot please run the Unresolved review feedback:
|
|
✅ 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. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46400 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. Field resolution is properly moved before validation, env var fallbacks follow the right precedence (message → env var → empty string), and the 4 new tests cover each fallback path plus the message-takes-precedence case. The beforeEach cleanup prevents test bleed. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 9.2 AIC · ⌖ 4.92 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause addressed: env var fallbacks applied before the hard-fail guard
- ✅ Solid regression tests: 4 new cases covering fallback priority and each optional field
- ✅
beforeEachenv cleanup prevents test bleed - ✅ Existing failure test updated to reflect the new two-pronged guard
Minor Suggestion
- The
runUrlternary is a long one-liner (see inline comment) — consider splitting or extracting a helper for readability
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 20 AIC · ⌖ 4.42 AIC · ⊞ 6.7K
Comment /matt to run again
| // Resolve fields from message, falling back to environment variables for fields | ||
| // that the agent may not know (e.g. workflow_name is not part of the agent's context). | ||
| const workflowName = message.workflow_name || process.env.GH_AW_WORKFLOW_NAME || ""; | ||
| const workflowSource = message.workflow_source || ""; |
There was a problem hiding this comment.
[/codebase-design] The runUrl ternary is a single long line — hard to scan during future edits.
💡 Suggested refactor
const runUrl =
message.run_url ||
(process.env.GITHUB_SERVER_URL &&
process.env.GITHUB_REPOSITORY &&
process.env.GITHUB_RUN_ID
? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}`
: "");Or extract a small helper buildRunUrl() alongside the other field resolutions to keep the handler body scannable.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 26.8 AIC · ⌖ 4.34 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/missing_issue_helpers.cjs:28
Unreadable one-liner: the runUrl ternary is ~160 chars on a single line, making it hard to review and a merge-conflict magnet.
<details>
<summary>💡 Suggested refactor</summary>
const runUrl =
message.run_url ||
(process.env.GITHUB_SERVER_URL &&
process.env.GITHUB_REPOSITORY &&
process.env.GITHUB_RUN_ID
? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}`
: "");Splitting across lines makes the three…
🧪 Test Quality Sentinel Report
📊 Metrics (21 tests)
|
|
@copilot run pr-finisher skill |
The cron schedule was inadvertently changed from "11 4 * * 6" to "21 3 * * 5" as an unintended side effect of running make recompile. This change is unrelated to the safe-outputs env-var fallback fix. Reverting to the original value. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Reverted the unintended cron schedule change in |
|
🎉 This pull request is included in a new release. Release: |
create_report_incomplete_issuemessages emitted by agents lack deployment-time metadata (workflow_name,workflow_source_url,run_url) because agents don't have that context. The handler was doing a hard fail on missingworkflow_name, silently dropping the incomplete-run report and turning thesafe_outputsjob red.Changes
missing_issue_helpers.cjsworkflow_name,workflow_source_url, andrun_urlfrom the message first, then fall back to env vars available in thesafe_outputsjob:workflow_name→GH_AW_WORKFLOW_NAMEworkflow_source_url→GH_AW_WORKFLOW_SOURCE_URLrun_url→ constructed fromGITHUB_SERVER_URL/GITHUB_REPOSITORY/GITHUB_RUN_IDworkflow_nameonly if both the message field and the env var fallback are emptymissing_issue_helpers.test.cjsbeforeEachnow clears all relevant env vars to prevent test bleedworkflow_nametest to assert failure only when both message field and env var are absent