Conversation
Automated sync from stranske/Workflows Template hash: 61642342d6a9 Changes synced from sync-manifest.yml
🤖 Keepalive Loop StatusPR #82 | Agent: Codex | Iteration 0/5 Current State
🔍 Failure Classification| Error type | infrastructure | |
Keepalive Work Log (click to expand)
|
There was a problem hiding this comment.
Pull request overview
Syncs workflow templates from stranske/Workflows, extending the weekly metrics workflow to include bot-comment App authentication coverage signals alongside the existing terminal-disposition coverage reporting.
Changes:
- Add a new “bot-comment auth coverage” preflight step to the weekly metrics workflow and upload its JSON/MD outputs as artifacts.
- Expand weekly metrics artifact selection to include bot-comment auth coverage artifact families.
- Introduce
.github/scripts/bot_comment_auth_coverage.jsto aggregate/format bot-comment auth coverage into summary outputs (warning-only vs hard-block modes).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/agents-weekly-metrics.yml |
Adds checkout of the new script, runs the new preflight, uploads outputs, and gates via combined “coverage hard-blocks” step. |
.github/scripts/weekly_metrics_artifacts.js |
Recognizes and prioritizes bot-comment auth coverage artifact families during selection. |
.github/scripts/bot_comment_auth_coverage.js |
New aggregator/report generator for bot-comment App auth coverage (JSON + markdown), with optional hard-block enforcement. |
| BOT_COMMENT_AUTH_COVERAGE_MD: bot-comment-auth-coverage-summary.md | ||
| BOT_COMMENT_AUTH_COVERAGE_MODE: ${{ vars.BOT_COMMENT_AUTH_COVERAGE_MODE || 'warning-only' }} | ||
| BOT_COMMENT_AUTH_HARD_BLOCK_APPROVED: ${{ vars.BOT_COMMENT_AUTH_HARD_BLOCK_APPROVED || 'false' }} | ||
| BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE: ${{ vars.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE || 'client-id' }} |
There was a problem hiding this comment.
bot_comment_auth_coverage.js supports overriding wrapper allowed auth modes via BOT_COMMENT_WRAPPER_ALLOWED_AUTH_MODES, but this workflow only wires BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE and BOT_COMMENT_REUSABLE_ALLOWED_AUTH_MODES. If wrapper allowed-modes are meant to be configurable too, consider adding the missing env var here; otherwise consider dropping that unused override path from the script to avoid confusion.
| BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE: ${{ vars.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE || 'client-id' }} | |
| BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE: ${{ vars.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE || 'client-id' }} | |
| BOT_COMMENT_WRAPPER_ALLOWED_AUTH_MODES: ${{ vars.BOT_COMMENT_WRAPPER_ALLOWED_AUTH_MODES || 'client-id' }} |
| return { | ||
| ...base, | ||
| expected_mode: normalizeAuthMode( | ||
| options.wrapper_expected_mode ?? process.env.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE | ||
| ) === 'unknown' | ||
| ? base.expected_mode | ||
| : normalizeAuthMode( | ||
| options.wrapper_expected_mode ?? process.env.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE | ||
| ), |
There was a problem hiding this comment.
componentPolicy() calls normalizeAuthMode(options.wrapper_expected_mode …) twice when computing expected_mode. Store the normalized value in a local variable and reuse it to simplify the control flow and avoid duplicated parsing.
| return { | |
| ...base, | |
| expected_mode: normalizeAuthMode( | |
| options.wrapper_expected_mode ?? process.env.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE | |
| ) === 'unknown' | |
| ? base.expected_mode | |
| : normalizeAuthMode( | |
| options.wrapper_expected_mode ?? process.env.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE | |
| ), | |
| const wrapperExpectedMode = normalizeAuthMode( | |
| options.wrapper_expected_mode ?? process.env.BOT_COMMENT_WRAPPER_EXPECTED_AUTH_MODE | |
| ); | |
| return { | |
| ...base, | |
| expected_mode: wrapperExpectedMode === 'unknown' ? base.expected_mode : wrapperExpectedMode, |
| return { | ||
| component, | ||
| record_count: componentRecords.length, | ||
| latest, | ||
| expected_mode: componentPolicyConfig.expected_mode, | ||
| allowed_modes: componentPolicyConfig.allowed_modes, | ||
| status: blockers.length > 0 ? componentPolicyConfig.missing_record_severity === 'no-data' && !latest ? 'no-data' : 'warning' : 'pass', |
There was a problem hiding this comment.
The status: assignment for each component summary is a nested ternary on a single long line, which is hard to read and easy to break when changing the conditions. Consider rewriting it as a small if/else block (or a helper function) so the no-data vs warning vs pass logic is explicit.
| return { | |
| component, | |
| record_count: componentRecords.length, | |
| latest, | |
| expected_mode: componentPolicyConfig.expected_mode, | |
| allowed_modes: componentPolicyConfig.allowed_modes, | |
| status: blockers.length > 0 ? componentPolicyConfig.missing_record_severity === 'no-data' && !latest ? 'no-data' : 'warning' : 'pass', | |
| let status = 'pass'; | |
| if (blockers.length > 0) { | |
| if (componentPolicyConfig.missing_record_severity === 'no-data' && !latest) { | |
| status = 'no-data'; | |
| } else { | |
| status = 'warning'; | |
| } | |
| } | |
| return { | |
| component, | |
| record_count: componentRecords.length, | |
| latest, | |
| expected_mode: componentPolicyConfig.expected_mode, | |
| allowed_modes: componentPolicyConfig.allowed_modes, | |
| status, |
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Manifest:
.github/sync-manifest.yml