[codex] Fix sync review source regressions - #1909
Conversation
Automated Status SummaryHead SHA: 6ce80c3
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeSync/Dependabot Campaign QueueRemote GitHub Actions owns discovery for sync-generated and Dependabot PR rounds. Local Codex should only claim items from this issue when SummaryContext for AgentRelated Issues/PRs
Tasks
Acceptance criteria
|
GitHub Actions
Error
51a11f1 to
f942484
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f942484256
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| details[index] = replace(existing, line=None, count=existing.count + detail.count) | ||
| return | ||
|
|
||
| details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) |
There was a problem hiding this comment.
When the detail buffer is full and a new error reason appears that is not already compacted, this branch overwrites details[-1] with the new reason while adding the overwritten row's count. That reassigns existing counts to the wrong reason/path bucket instead of just accounting for the new overflow event. A concrete case is many invalid-json rows followed by legacy-json-fallback-buffer-limit: the code can shift one invalid-json count into the legacy reason, which corrupts by_reason/detail reporting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Fixes regressions impacting sync PR review sources by restoring action SHA pinning and tightening parse-error detail compaction so stored error details remain within configured limits.
Changes:
- Update
_append_parse_error_detailto compact/merge parse-error details when the storage cap is reached, and add/adjust tests for the new behavior. - Re-pin previously-floated GitHub Actions in
agents-verify-to-new-pr.ymlback to commit SHAs (with inline tag comments) in both source + consumer template workflows. - Minor formatting cleanup in
repo_review_evaluator.py.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/aggregate_agent_metrics.py |
Changes parse-error detail compaction logic when the stored-detail limit is reached. |
templates/consumer-repo/scripts/aggregate_agent_metrics.py |
Mirrors the same parse-error compaction changes for consumer-synced script parity. |
tests/scripts/test_aggregate_agent_metrics.py |
Adds coverage for compaction-at-limit behavior and updates legacy fallback buffer assertions. |
.github/workflows/agents-verify-to-new-pr.yml |
Re-pins key actions (github-script/checkout/upload-artifact) to SHAs in the source workflow. |
templates/consumer-repo/.github/workflows/agents-verify-to-new-pr.yml |
Mirrors the same action SHA pins in the consumer template workflow. |
scripts/repo_review_evaluator.py |
Simplifies a list comprehension (no behavioral change intended). |
| details[index] = replace(existing, line=None, count=existing.count + detail.count) | ||
| return | ||
|
|
||
| details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) |
There was a problem hiding this comment.
When the parse-error detail list is at the storage limit and the incoming detail does not match any existing entry, the fallback details[-1] = replace(detail, ..., count=details[-1].count + detail.count) reassigns the previous last entry’s count to the new detail’s (path/artifact/reason). This can corrupt the by-reason/by-artifact counters (e.g., a large invalid-json count can be shifted into legacy-json-fallback-buffer-limit just because it was appended last). Consider aggregating the overflow count into an existing entry without changing its key, or using a dedicated sentinel/overflow bucket, so existing counts aren’t reclassified when the limit is hit.
| details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) | |
| details[-1] = replace(details[-1], line=None, count=details[-1].count + detail.count) |
| details[index] = replace(existing, line=None, count=existing.count + detail.count) | ||
| return | ||
|
|
||
| details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) |
There was a problem hiding this comment.
When the parse-error detail list is at the storage limit and the incoming detail does not match any existing entry, the fallback details[-1] = replace(detail, ..., count=details[-1].count + detail.count) reassigns the previous last entry’s count to the new detail’s (path/artifact/reason). This can corrupt the by-reason/by-artifact counters (e.g., a large invalid-json count can be shifted into legacy-json-fallback-buffer-limit just because it was appended last). Consider aggregating the overflow count into an existing entry without changing its key, or using a dedicated sentinel/overflow bucket, so existing counts aren’t reclassified when the limit is hit.
| details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) | |
| details[-1] = replace(details[-1], line=None, count=details[-1].count + detail.count) |
| - name: Check PR is merged | ||
| id: check-merged | ||
| uses: actions/github-script@v9 | ||
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 |
There was a problem hiding this comment.
This workflow now pins some actions by commit SHA, but actions/setup-python@v6 is still referenced by a floating tag later in the job. The repo’s action-pinning policy calls for SHA pinning to avoid tag retags changing workflow behavior (docs/ci/ACTIONS_PINNING.md). Please pin setup-python to a commit SHA as well for consistency and supply-chain integrity.
| - name: Check PR is merged | ||
| id: check-merged | ||
| uses: actions/github-script@v9 | ||
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 |
There was a problem hiding this comment.
This workflow now pins some actions by commit SHA, but actions/setup-python@v6 is still referenced by a floating tag later in the job. The repo’s action-pinning policy calls for SHA pinning to avoid tag retags changing workflow behavior (docs/ci/ACTIONS_PINNING.md). Please pin setup-python to a commit SHA as well for consistency and supply-chain integrity.
Related to campaign issue #1836
Automated Status Summary
Scope
Sync/Dependabot Campaign Queue
Remote GitHub Actions owns discovery for sync-generated and Dependabot PR rounds. Local Codex should only claim items from this issue when
needs-local-codexwork is queued.Summary
Context for Agent
Related Issues/PRs
Tasks
Acceptance criteria
sync/workflows-23b44c982516(e40a0a5e8b72)Head SHA: f942484
Latest Runs: ✅ success — Gate
Required: gate: ✅ success