Conversation
Automated sync from stranske/Workflows Template hash: 69bf45f7ea5a Changes synced from sync-manifest.yml
🤖 Keepalive Loop StatusPR #147 | Agent: Codex | Iteration 0/5 Current State
🔍 Failure Classification| Error type | infrastructure | |
|
Status | ✅ no new diagnostics |
There was a problem hiding this comment.
Pull request overview
This PR syncs workflow template files from the central Workflows repository, introducing a new prompt routing system for keepalive workflows and enhancing error classification for transient failures.
Key Changes:
- Adds prompt routing logic to dynamically select instruction templates based on scenario (fix CI, verify acceptance, or normal task progression)
- Implements task attempt tracking to avoid repeatedly trying the same incomplete tasks
- Enhances transient error detection for dirty git state issues (workflow artifacts, untracked session files)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.github/scripts/keepalive_prompt_routing.js |
New file implementing prompt mode resolution logic based on scenario, mode, action, and reason inputs |
.github/scripts/keepalive_loop.js |
Major update adding task attempt history, verification workflow support, improved error handling for skipped/cancelled runs, and integration with prompt routing |
.github/scripts/keepalive_instruction_template.js |
Refactored to support multiple template paths and dynamic template selection based on prompt mode |
.github/scripts/error_classifier.js |
Added patterns for detecting transient git workspace state issues |
.github/scripts/agents-guard.js |
Modified approval logic to allow automated PRs (dependabot, renovate) to bypass approval when they have the allow label |
| content = fs.readFileSync(TEMPLATE_PATH, 'utf8').trim(); | ||
| } catch (fallbackError) { | ||
| console.warn( | ||
| `Warning: Could not load keepalive instruction template from ${resolvedPath}: ${fallbackError.message}` |
There was a problem hiding this comment.
The warning message at line 84 displays resolvedPath in the error, but the error actually originated from trying to read TEMPLATE_PATH (the fallback). This could be confusing for debugging. Consider updating the message to clarify that both the requested path and the fallback path failed to load.
| `Warning: Could not load keepalive instruction template from ${resolvedPath}: ${fallbackError.message}` | |
| `Warning: Could not load keepalive instruction template. ` | |
| + `Primary path "${resolvedPath}" failed with: ${err.message}. ` | |
| + `Fallback path "${TEMPLATE_PATH}" also failed with: ${fallbackError.message}` |
| // Allow label to bypass approval for automated PRs (dependabot, renovate) | ||
| const isAutomatedPR = normalizedAuthor && (normalizedAuthor === 'dependabot[bot]' || normalizedAuthor === 'renovate[bot]'); | ||
| const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !(hasAllowLabel && isAutomatedPR); |
There was a problem hiding this comment.
The logic allows automated PRs (dependabot, renovate) to bypass approval requirements if they have the allow label. However, this creates a potential security risk where an automated PR with the allow label can modify protected workflow files without human review. Consider whether this bypass should require both the label AND some form of approval, or if there should be additional constraints on what types of changes automated PRs can make to protected paths.
| // Allow label to bypass approval for automated PRs (dependabot, renovate) | |
| const isAutomatedPR = normalizedAuthor && (normalizedAuthor === 'dependabot[bot]' || normalizedAuthor === 'renovate[bot]'); | |
| const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !(hasAllowLabel && isAutomatedPR); | |
| // Automated PRs (dependabot, renovate) must always get approval for protected changes, | |
| // even if they have the allow label. The label can still bypass approval for | |
| // human-authored PRs. | |
| const isAutomatedPR = normalizedAuthor && (normalizedAuthor === 'dependabot[bot]' || normalizedAuthor === 'renovate[bot]'); | |
| const automatedBypassAllowed = hasAllowLabel && !isAutomatedPR; | |
| const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !automatedBypassAllowed; |
|
|
||
| if (agent && typeof agent === 'object') { | ||
| params = agent; | ||
| resolvedAgent = params.agent; |
There was a problem hiding this comment.
The function signature change for getKeepaliveInstructionWithMention makes the first parameter accept either a string (agent name) or an object (options). However, the backward compatibility logic at lines 120-123 has a potential issue: if agent is an object with a falsy agent property (e.g., { agent: '' }), the resolved agent will be falsy and fall back to 'codex', but this might not be the intended behavior. Consider explicitly checking if agent.agent exists in the object rather than just using its value.
| resolvedAgent = params.agent; | |
| if (Object.prototype.hasOwnProperty.call(params, 'agent')) { | |
| resolvedAgent = params.agent; | |
| } |
| 'eai_again', | ||
| // Git workspace state issues - agent encountered unexpected changes | ||
| 'unexpected changes', | ||
| 'untracked', |
There was a problem hiding this comment.
The pattern 'untracked' at line 46 is too generic and could match legitimate error messages about untracked files that are not transient infrastructure issues. Consider making this pattern more specific, such as 'untracked.*workflows-lib' or 'untracked.*codex-session', to avoid false positives where actual code issues involve untracked files.
| 'untracked', | |
| 'untracked.*workflows-lib', | |
| 'untracked.*codex-session', |
| let action = 'wait'; | ||
| let reason = 'pending'; | ||
| const verificationStatus = normalise(state?.verification?.status); | ||
| const verificationDone = ['done', 'verified', 'complete'].includes(verificationStatus.toLowerCase()); |
There was a problem hiding this comment.
The verification status is normalized to lowercase at line 1091, but line 1092 calls .toLowerCase() again on verificationStatus which is already lowercase. This is redundant. The check should be ['done', 'verified', 'complete'].includes(verificationStatus) since the value has already been normalized to lowercase.
| const verificationDone = ['done', 'verified', 'complete'].includes(verificationStatus.toLowerCase()); | |
| const verificationDone = ['done', 'verified', 'complete'].includes(verificationStatus); |
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Manifest:
.github/sync-manifest.yml