Normalize disabled failure-issue expiry in lockfiles - #56901
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Lean already. Ship. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
COMMENT — this change is tiny and the intent is sound, but I did not find a blocking correctness regression in the touched lines.
Review notes
- The new branch correctly forces the disabled expiry marker to
"0"whensafe-outputs.report-failure-as-issue: falseis explicitly set. - The added regression test covers the direct compiler output for that disabled case.
- I did not find a changed-line bug worth blocking on in this patch.
- Advisory only: the new condition uses a raw
String() == "false"check, while adjacent code already normalizes viastrings.TrimSpace/strings.EqualFold; that inconsistency looks sloppy, but given the type's parser constraints it does not appear to break valid config inputs here. - The requested
grumpy-codersub-agent was unavailable in this environment, so adjudication was based on direct review only.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 9.61 AIC · ⌖ 7.34 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
🟢 Approval recommended
The focused compiler fix matches the documented requirement and includes appropriate regression coverage.
Pull request overview
Ensures disabled failure-issue reporting produces deterministic lockfiles across platforms.
Changes:
- Emits expiry
"0"when failure-issue reporting is disabled. - Adds regression coverage for conclusion-job rendering.
File summaries
| File | Description |
|---|---|
pkg/workflow/notify_comment_conclusion_helpers.go |
Canonicalizes disabled expiry during rendering. |
pkg/workflow/notify_comment_test.go |
Verifies the canonical disabled value. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix correctly addresses the root cause (platform-dependent normalization of the disabled expiry value) and includes a regression test. Approving with one minor observation.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: the compiler now emits
"0"directly rather than relying on post-processing, removing the platform-dependent path entirely. - Regression coverage added:
TestConclusionJobActionFailureIssueExpiration_DisabledWhenFailureReportingIsOffcovers the happy path for the fix.
One observation
The condition data.SafeOutputs.ReportFailureAsIssue.String() == "false" works for literal false values but would silently skip the disabled-expiry path if ReportFailureAsIssue were set to a GitHub Actions expression (e.g. ${{ vars.DISABLE }}). The existing helper templatableBoolIsTrue is used elsewhere for symmetry. If expressions are out-of-scope here (likely), a brief comment noting that expressions are not handled would aid future readers.
Positive Highlights
- ✅ Minimal, surgical change — only the early-exit guard and its test are new.
- ✅ Existing
TestConclusionJobActionFailureIssueExpiration_DefaultFromRepoConfigand_UsesAWJSONConfigtests are unaffected. - ✅ PR description is clear and includes the issue reference.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 23.9 AIC · ⌖ 10.4 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with harden + audit lenses (bug-fix change type).
The change correctly short-circuits loadRepoConfig() when failure-issue reporting is explicitly disabled, setting expiresHours = 0 before the env var is emitted. The new test covers the disabled path end-to-end. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 16.4 AIC · ⌖ 10.5 AIC · ⊞ 6.2K
|
🎉 This pull request is included in a new release. Release: |
Workflows with
safe-outputs.report-failure-as-issue: falsecould emit platform-dependent expiry values. The compiler now renders the canonical disabled value directly.Compiler
"0"forGH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURSwhen failure-issue reporting is explicitly disabled.Regression coverage