Conversation
Automated sync from stranske/Workflows Template hash: 2e01e11c7edc Changes synced from sync-manifest.yml
🤖 Keepalive Loop StatusPR #288 | 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 and supporting scripts from stranske/Workflows to expand weekly metrics reporting, add artifact download manifest tracking, and enhance coverage/telemetry contracts for terminal dispositions and bot-comment auth.
Changes:
- Add artifact download manifest generation (
metric-artifact-download-manifest.{json,md}) and include it in weekly metrics artifacts and outputs. - Extend weekly metrics aggregation to emit a machine-readable JSON summary contract alongside the markdown summary.
- Enhance terminal-disposition and bot-comment auth coverage contracts/markdown with priority-family visibility and verifier model compatibility metadata.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/aggregate_agent_metrics.py |
Enriches parsed metrics with source metadata, adds parse-error detail tracking, emits JSON summary contract, and reads optional download manifest contract. |
.github/workflows/agents-weekly-metrics.yml |
Initializes/records/finalizes the artifact download manifest during artifact fetch; uploads new JSON outputs and manifest artifacts. |
.github/workflows/agents-bot-comment-handler.yml |
Adds skip_reason output and writes/uploads a wrapper terminal-disposition artifact for traceability. |
.github/workflows/agents-81-gate-followups.yml |
Switches jq invocation to compact output (-cn) for emitted metrics JSON. |
.github/scripts/weekly_metrics_download_manifest.js |
New script to maintain the download/unzip manifest contract + markdown summary. |
.github/scripts/weekly_metrics_artifacts.js |
Adds priority-family status reporting (missing families + per-family availability/selection metadata). |
.github/scripts/terminal_disposition_coverage.js |
Adds verifier model compatibility checks and integrates priority-family statuses into the coverage report/markdown. |
.github/scripts/terminal_disposition.js |
Extends normalized terminal disposition records with llm_model, model_selection_reason, and verifier_mode. |
.github/scripts/coverage_monitor_summary.js |
New weekly “coverage monitor” summary contract combining terminal + bot-auth monitor outputs. |
.github/scripts/bot_comment_auth_coverage.js |
Improves organic evidence reporting with “skipped/missing requirements” and wrapper/reusable decision fields. |
.github/scripts/agents_pr_meta_update_body.js |
Adds embedded issue metadata marker and Closes #… line to the PR preamble when a source issue exists. |
| return (manifest.artifacts || []).find((artifact) => { | ||
| if (cleanId && cleanString(artifact.id) === cleanId) return true; | ||
| return cleanName && cleanString(artifact.name) === cleanName; | ||
| }); |
There was a problem hiding this comment.
findArtifact() falls back to matching by name even when an id is provided but doesn’t match. If artifact names are reused across runs, a bad/changed ID could silently update the wrong entry. Consider: when id is present, match by id only; only fall back to name matching when id is not provided.
| return (manifest.artifacts || []).find((artifact) => { | |
| if (cleanId && cleanString(artifact.id) === cleanId) return true; | |
| return cleanName && cleanString(artifact.name) === cleanName; | |
| }); | |
| const artifacts = manifest.artifacts || []; | |
| if (cleanId) { | |
| return artifacts.find((artifact) => cleanString(artifact.id) === cleanId); | |
| } | |
| if (cleanName) { | |
| return artifacts.find((artifact) => cleanString(artifact.name) === cleanName); | |
| } | |
| return undefined; |
| raw = line.strip() | ||
| if not raw: | ||
| continue | ||
| raw_lines.append(raw) |
There was a problem hiding this comment.
In _read_ndjson, raw_lines is populated for every non-empty line (even when parsing succeeds). For large NDJSON inputs this can significantly increase memory usage. Consider only buffering lines when you actually need the whole-file fallback (e.g., only while file_entries is still empty, or by rereading the file when fallback is triggered).
| raw_lines.append(raw) | |
| if not file_entries: | |
| raw_lines.append(raw) |
| def _attach_metric_source(entry: dict[str, Any], path: Path) -> dict[str, Any]: | ||
| source = _metric_source(path) | ||
| enriched = dict(entry) | ||
| enriched.setdefault("artifact_name", source.artifact) | ||
| enriched.setdefault("artifact_family", source.artifact_family) |
There was a problem hiding this comment.
_attach_metric_source recomputes _metric_source(path) for every parsed entry, but the source is constant per file. For high-volume metrics files this adds avoidable overhead. Consider computing the MetricSource once per path inside _read_ndjson and passing it into the attach/enrich helper.
| unsupported_model_dispositions[str(disposition)] += 1 | ||
| elif is_verifier_terminal: | ||
| verifier_mode = str(entry.get("verifier_mode") or "").strip().lower() | ||
| if verifier_mode != "evaluate": |
There was a problem hiding this comment.
When verifier_mode is missing/empty, verifier_mode != "evaluate" evaluates true and the entry is counted as missing model metadata. This makes empty/unknown modes look like they require model metadata, which can inflate the warning counters. Consider requiring an explicit non-empty verifier_mode before treating the model field as mandatory (or handling an empty mode as unknown/evaluate).
| if verifier_mode != "evaluate": | |
| if verifier_mode and verifier_mode != "evaluate": |
| "count": len(parse_error_details), | ||
| "by_artifact_family": dict(sorted(family_counts.items())), | ||
| "by_artifact": dict(sorted(artifact_counts.items())), | ||
| "by_reason": dict(sorted(reason_counts.items())), | ||
| "details": [detail.as_dict() for detail in parse_error_details], |
There was a problem hiding this comment.
The JSON contract includes parse_errors.details for every parse failure with no size cap. If an input artifact is corrupted, this list can become extremely large and bloat the JSON output/artifact. Consider capping the stored details (and adding an omitted_count), similar to the markdown table cap.
|
|
||
| function overallStatus(monitors) { | ||
| if (monitors.some((monitor) => monitor.should_fail || monitor.status === 'fail')) return 'fail'; | ||
| if (monitors.some((monitor) => ['missing', 'parse-error', 'warning'].includes(monitor.status))) { |
There was a problem hiding this comment.
overallStatus() treats only missing, parse-error, and warning monitor statuses as warning, but normalizeStatus() can return unknown. If a monitor report is malformed and yields unknown, the overall status can incorrectly become pass. Consider treating unknown as a warning (or mapping it to warning) so invalid monitor reports don’t look healthy.
| if (monitors.some((monitor) => ['missing', 'parse-error', 'warning'].includes(monitor.status))) { | |
| if (monitors.some((monitor) => ['missing', 'parse-error', 'warning', 'unknown'].includes(monitor.status))) { |
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Manifest:
.github/sync-manifest.yml