Make targeted workflow compilation deterministic - #57745
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Reconciliation can overwrite explicit expiry values, misidentify enforcement, and modify the wrong lock file for direct paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns targeted workflow compilation with full compilation for implicit action-failure expiry markers.
Changes:
- Reconciles expiry markers after targeted compilation.
- Preserves custom-directory and
--no-emitbehavior. - Adds deterministic lock-file regression coverage.
File summaries
| File | Description |
|---|---|
pkg/workflow/maintenance_workflow_expiry.go |
Adds maintenance enforcement detection helper. |
pkg/cli/compile_pipeline.go |
Runs expiry reconciliation for targeted compilation. |
pkg/cli/compile_maintenance_test.go |
Compares targeted and full lock outputs. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if !config.NoEmit && config.WorkflowDir == "" { | ||
| if gitRoot, err := gitutil.FindGitRoot(); err == nil { | ||
| absWorkflowDir := getAbsoluteWorkflowDir(getWorkflowsDir(), gitRoot) | ||
| workflow.DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList, absWorkflowDir) |
There was a problem hiding this comment.
Fixed in 85e8e16: reconciliation now uses the actual emitted lock file path collected per compiled workflow (fileResult.lockFile) instead of reconstructing workflowDir + WorkflowID, so direct-path targets outside .github/workflows are patched correctly and unrelated lock files sharing the same workflow ID are left untouched.
| if _, err := os.Stat(filepath.Join(workflowDir, "agentics-maintenance.yml")); os.IsNotExist(err) { | ||
| disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir) |
There was a problem hiding this comment.
Fixed in 85e8e16: isActionFailureExpiryEnforced now also checks repoConfig.Maintenance.IsJobDisabled("close-expired-entities") (and maintenance: false) in addition to file existence, so a maintenance file that omits the sweeper no longer preserves the marker.
| if _, err := os.Stat(filepath.Join(workflowDir, "agentics-maintenance.yml")); os.IsNotExist(err) { | ||
| disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir) |
There was a problem hiding this comment.
Fixed in 85e8e16: DisableDefaultActionFailureExpiryMarkersIfUnenforced now takes *RepoConfig and returns early when repoConfig.IsActionFailureIssueExpiresExplicit() is true, so an explicit maintenance.action_failure_issue_expires value is never rewritten. Added TestCompileSpecificFiles_PreservesExplicitActionFailureExpiry to cover this.
|
✅ 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 failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
❌ Test Quality Sentinel failed during test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found one simplification worth cutting: the new wrapper adds indirection for a condition that already has a single implementation and a single caller. net: -15 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57745 · codex · mai10 · 3.37 AIC · ⌖ 0.504 AIC · ⊞ 14K
Comment /ponytail to run again
| // Note: Maintenance workflow generation requires parsing all workflows in the directory | ||
| // to check for expires fields, so we skip it when compiling specific files to avoid | ||
| // unnecessary parsing and warnings from unrelated workflows | ||
| if !config.NoEmit && config.WorkflowDir == "" { |
There was a problem hiding this comment.
pkg/cli/compile_pipeline.go:864: yagni: one-call helper for a tiny branch. Inline the file-existence check in runPostProcessing and call disableDefaultActionFailureExpiryMarkers directly.
There was a problem hiding this comment.
In 85e8e16 the branch grew beyond a tiny file-existence check (it now also verifies close-expired-entities isn't disabled and that maintenance isn't explicitly off), so it's kept as a named helper (isActionFailureExpiryEnforced) rather than inlined.
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.
This fix heads in the right direction, but it still leaves targeted compilation nondeterministic in edge cases.
Blocking themes
- The new reconciliation still assumes every targeted compile is operating on
.github/workflows, so direct-path inputs can miss their emitted lock file or patch the wrong one. - Enforcement is inferred from
agentics-maintenance.ymlexisting, but that file can exist with the expiry sweeper disabled. - The marker rewrite still cannot distinguish the implicit 168h default from an explicit configured expiry, so it can erase a user's opt-in configuration.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 38.8 AIC · ⌖ 7.48 AIC · ⊞ 23.5K
Comment /review to run again
| // action-failure expiry markers when no maintenance workflow exists to enforce them. | ||
| // The generated maintenance workflow always includes the global expiry sweeper. | ||
| func DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList []*WorkflowData, workflowDir string) { | ||
| if _, err := os.Stat(filepath.Join(workflowDir, "agentics-maintenance.yml")); os.IsNotExist(err) { |
There was a problem hiding this comment.
File existence is the wrong enforcement check. agentics-maintenance.yml can already exist while maintenance.disabled_jobs removes close-expired-entities, which means no expiry sweeper is generated even though this helper preserves a positive expiration marker.
💡 Base the decision on whether maintenance would actually emit the expiry jobs
Use the same repo-config logic that scanWorkflowsForExpires and maintenance generation use, or pass enough provenance into this helper to tell whether the close-expired jobs are enabled. Otherwise targeted compile still advertises expiration that no workflow will ever enforce.
There was a problem hiding this comment.
Fixed in 85e8e16: isActionFailureExpiryEnforced now checks repoConfig.Maintenance.IsJobDisabled("close-expired-entities") and maintenance: false in addition to file existence. Covered by TestCompileSpecificFiles_DisablesExpiryWhenCloseExpiredJobDisabled.
| if !config.NoEmit && config.WorkflowDir == "" { | ||
| if gitRoot, err := gitutil.FindGitRoot(); err == nil { | ||
| absWorkflowDir := getAbsoluteWorkflowDir(getWorkflowsDir(), gitRoot) | ||
| workflow.DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList, absWorkflowDir) |
There was a problem hiding this comment.
WorkflowDir == "" is not enough to identify the emitted lock file here: targeted compilation accepts direct paths outside .github/workflows, but this helper always rewrites <gitRoot>/.github/workflows/<workflowID>.lock.yml. That means custom/foo.md still keeps its stale marker while an unrelated .github/workflows/foo.lock.yml can be mutated instead.
💡 Reconcile the actual emitted lock path instead of reconstructing one from the workflow ID
The compile path already knows each generated lock filename (compileWorkflowFileResult.lockFile). Thread those concrete paths into post-processing, or filter targeted reconciliations to workflows whose resolved source lives under the default workflow directory before touching any lock file. That keeps the full compile and targeted compile outputs aligned without risking cross-directory corruption.
There was a problem hiding this comment.
Fixed in 85e8e16: compileSpecificFiles now accumulates compiledLockFiles from fileResult.lockFile and threads it into runPostProcessing/DisableDefaultActionFailureExpiryMarkersIfUnenforced, so the actual emitted lock path is reconciled. Covered by TestCompileSpecificFiles_PatchesDirectPathOutsideDefaultWorkflowDir.
| // The generated maintenance workflow always includes the global expiry sweeper. | ||
| func DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList []*WorkflowData, workflowDir string) { | ||
| if _, err := os.Stat(filepath.Join(workflowDir, "agentics-maintenance.yml")); os.IsNotExist(err) { | ||
| disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir) |
There was a problem hiding this comment.
This helper still rewrites every positive GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS value to "0", so a targeted compile can silently erase an explicit maintenance.action_failure_issue_expires opt-in when no maintenance file is present yet. Full compile keeps that explicit value because it generates maintenance, so the deterministic-output bug remains and user configuration gets discarded.
💡 Only disable the implicit default marker, never an explicit configured expiry
The helper needs provenance, not just the rendered lock text. Thread the repo config or an explicit/implicit flag through post-processing, then rewrite only lock files whose expiration came from the implicit 168-hour default. Add a regression that compiles with aw.json setting maintenance.action_failure_issue_expires and verifies targeted compile preserves that explicit value.
There was a problem hiding this comment.
Fixed in 85e8e16: DisableDefaultActionFailureExpiryMarkersIfUnenforced now accepts *RepoConfig and skips rewriting entirely when repoConfig.IsActionFailureIssueExpiresExplicit() is true. Added TestCompileSpecificFiles_PreservesExplicitActionFailureExpiry (aw.json with maintenance.action_failure_issue_expires: 48, no pre-existing maintenance file) to verify targeted compile preserves the explicit value.
|
@copilot Please address the current reviewer feedback and failing checks on this PR, refresh the branch if needed, and run the
Please incorporate the requested fixes, rerun the necessary validation, and use the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all outstanding review feedback in 85e8e16: reconciliation now patches the actual emitted lock file path per compiled workflow (not reconstructed from workflowDir+WorkflowID), checks whether the close-expired-entities maintenance job is actually enabled rather than only checking file existence, and preserves an explicitly configured |
|
Category: bug | Risk: medium | Score: 68 (impact 32, urgency 22, quality 14) | Action: fast_track | CI: failure | Workflow compile determinism fix; current CI is failing and this needs prompt follow-up.
|
Targeted compilation restored the implicit action-failure expiry to
"168", while full compilation disabled it to"0"when no maintenance workflow could enforce expiry. This caused lock-file churn between compile modes.Expiry reconciliation
agentics-maintenance.ymlis absent.--no-emit.Regression coverage
gh aw compile <workflow-id>leaves implicit GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS="168" that fullgh aw compilepatches to "0" — lock file output is not deterministic #57648Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/33554808944
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.