Conversation
Automated sync from stranske/Workflows Template hash: 8b8ad95b59c0 Changes synced from sync-manifest.yml
🤖 Keepalive Loop StatusPR #577 | 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 template updates from stranske/Workflows, primarily to modernize the weekly metrics workflow’s Node setup and to enhance the bot-comment App auth coverage preflight diagnostics.
Changes:
- Updated
agents-weekly-metricsworkflow to explicitly set up Node.js (instead of ad-hoc npm installs) and improved the coverage hard-block error annotation. - Enhanced
bot_comment_auth_coverage.jswith more robust parsing, artifact selection reporting, error counters (read/parse/non-auth), and organic-evidence configuration behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/agents-weekly-metrics.yml | Adds actions/setup-node and refines the hard-block error message emitted when coverage checks fail. |
| .github/scripts/bot_comment_auth_coverage.js | Improves coverage report generation robustness and detail; adds artifact-dir matching logic and additional counters/blockers. |
| wrapper: /^bot-comment-auth-coverage-wrapper-\d+$/, | ||
| reusable: /^bot-comment-auth-coverage-reusable-\d+$/, |
There was a problem hiding this comment.
AUTH_ARTIFACT_DIR_PATTERNS now requires the artifact directory name to match bot-comment-auth-coverage-*-<digits> exactly. However, artifact selection in weekly_metrics_artifacts.js treats these as prefix-based families, so artifact names (and therefore unzip dirs) may legally include non-numeric suffixes; those would no longer be discovered by isPotentialAuthCoverageFile, causing silent coverage undercounting. Consider relaxing the regex (e.g., prefix match) or deriving the match from the same family logic used by artifact selection.
| wrapper: /^bot-comment-auth-coverage-wrapper-\d+$/, | |
| reusable: /^bot-comment-auth-coverage-reusable-\d+$/, | |
| wrapper: /^bot-comment-auth-coverage-wrapper-\d+(?:-.+)?$/, | |
| reusable: /^bot-comment-auth-coverage-reusable-\d+(?:-.+)?$/, |
| function summarizeBotCommentAuthCoverage(records = [], options = {}) { | ||
| const policy = normalizePolicy(options); | ||
| const parseErrors = Number(options.parse_errors ?? options.parseErrors ?? 0); | ||
| const readErrors = Number(options.read_errors ?? options.readErrors ?? 0); | ||
| const parsedJsonRecordCount = Number( | ||
| options.parsed_json_record_count ?? options.parsedJsonRecordCount ?? records.length | ||
| ); | ||
| const nonAuthRecordCount = Number( | ||
| options.non_auth_record_count ?? | ||
| options.nonAuthRecordCount ?? | ||
| Math.max(0, parsedJsonRecordCount - records.length) |
There was a problem hiding this comment.
The new numeric conversions (e.g., read_errors / parsed_json_record_count / non_auth_record_count) use Number(...) without guarding against NaN. If these values come from env/args and are non-numeric, comparisons like readErrors > 0 will behave incorrectly and the report will emit NaN. Consider normalizing with Number.isFinite(...) ? ... : 0 (or a shared helper) before using these values.
| function summarizeBotCommentAuthCoverage(records = [], options = {}) { | |
| const policy = normalizePolicy(options); | |
| const parseErrors = Number(options.parse_errors ?? options.parseErrors ?? 0); | |
| const readErrors = Number(options.read_errors ?? options.readErrors ?? 0); | |
| const parsedJsonRecordCount = Number( | |
| options.parsed_json_record_count ?? options.parsedJsonRecordCount ?? records.length | |
| ); | |
| const nonAuthRecordCount = Number( | |
| options.non_auth_record_count ?? | |
| options.nonAuthRecordCount ?? | |
| Math.max(0, parsedJsonRecordCount - records.length) | |
| function normalizeFiniteNumber(value, fallback = 0) { | |
| const numericValue = Number(value); | |
| return Number.isFinite(numericValue) ? numericValue : fallback; | |
| } | |
| function summarizeBotCommentAuthCoverage(records = [], options = {}) { | |
| const policy = normalizePolicy(options); | |
| const parseErrors = normalizeFiniteNumber(options.parse_errors ?? options.parseErrors ?? 0, 0); | |
| const readErrors = normalizeFiniteNumber(options.read_errors ?? options.readErrors ?? 0, 0); | |
| const parsedJsonRecordCount = normalizeFiniteNumber( | |
| options.parsed_json_record_count ?? options.parsedJsonRecordCount ?? records.length, | |
| records.length | |
| ); | |
| const nonAuthRecordCount = normalizeFiniteNumber( | |
| options.non_auth_record_count ?? | |
| options.nonAuthRecordCount ?? | |
| Math.max(0, parsedJsonRecordCount - records.length), | |
| Math.max(0, parsedJsonRecordCount - records.length) |
|
|
||
| const hardBlockActive = policy.effective_mode === HARD_BLOCK_MODE; | ||
| const shouldFail = hardBlockActive && coverageStatus !== 'pass'; | ||
| const shouldFail = hardBlockActive && coverageStatus === 'warning'; |
There was a problem hiding this comment.
shouldFail now only triggers in hard-block mode when coverageStatus === 'warning', which means a hard-block run will not fail on no-data. This differs from terminal_disposition_coverage.js (hard-block fails on anything other than pass) and can allow hard-block enforcement to be bypassed when no auth coverage records are found. Consider aligning the semantics (fail on no-data too) or explicitly documenting why bot-comment coverage is treated differently.
| const shouldFail = hardBlockActive && coverageStatus === 'warning'; | |
| const shouldFail = hardBlockActive && coverageStatus !== 'pass'; |
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Manifest:
.github/sync-manifest.yml