From f94248425609636eb0a18727e2201517e3949524 Mon Sep 17 00:00:00 2001 From: Tim Stranske Date: Sun, 26 Apr 2026 13:03:11 -0500 Subject: [PATCH] [codex] Fix sync review source regressions --- .github/workflows/agents-verify-to-new-pr.yml | 24 +++++------ scripts/aggregate_agent_metrics.py | 12 +++++- scripts/repo_review_evaluator.py | 6 +-- .../workflows/agents-verify-to-new-pr.yml | 24 +++++------ .../scripts/aggregate_agent_metrics.py | 12 +++++- tests/scripts/test_aggregate_agent_metrics.py | 40 +++++++++++++++++-- 6 files changed, 83 insertions(+), 35 deletions(-) diff --git a/.github/workflows/agents-verify-to-new-pr.yml b/.github/workflows/agents-verify-to-new-pr.yml index e921bd8d0..ab5fff8f6 100644 --- a/.github/workflows/agents-verify-to-new-pr.yml +++ b/.github/workflows/agents-verify-to-new-pr.yml @@ -32,7 +32,7 @@ jobs: steps: - name: Check PR is merged id: check-merged - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ github.token }} script: | @@ -66,7 +66,7 @@ jobs: - name: Checkout repository if: steps.check-merged.outputs.merged == 'true' - uses: actions/checkout@v6 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 with: repository: stranske/Workflows token: ${{ steps.select-token.outputs.token }} @@ -103,7 +103,7 @@ jobs: - name: Collect verification and original issue data id: collect if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ steps.select-token.outputs.token }} script: | @@ -248,7 +248,7 @@ jobs: - name: Check chain depth limit id: chain-check if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: CHAIN_DEPTH: ${{ steps.collect.outputs.chain_depth }} MAX_CHAIN_DEPTH: '2' @@ -389,7 +389,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: LINKED_ISSUE: ${{ steps.collect.outputs.original_issue_number }} NEEDS_HUMAN_REASON: ${{ steps.extract-verdict.outputs.needs_human_reason }} @@ -505,7 +505,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: FOLLOW_UP_DEPTH: ${{ steps.chain-check.outputs.next_depth }} EXTRACTED_VERDICT: ${{ steps.extract-verdict.outputs.verdict }} @@ -677,7 +677,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_TITLE: >- ${{ steps.generate.outputs.issue_title || @@ -779,7 +779,7 @@ jobs: steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' && steps.create-issue.outputs.issue_number != '' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} @@ -816,7 +816,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} ISSUE_URL: ${{ steps.create-issue.outputs.issue_url }} @@ -870,7 +870,7 @@ jobs: FOLLOWUP_ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} FOLLOWUP_ISSUE_URL: ${{ steps.create-issue.outputs.issue_url }} DISPATCH_AUTOPILOT_OUTCOME: ${{ steps.dispatch-autopilot.outcome }} - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ steps.select-token.outputs.token }} script: | @@ -1006,7 +1006,7 @@ jobs: - name: Upload terminal disposition artifact if: always() && steps.check-merged.outputs.merged == 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: verifier-terminal-disposition-${{ github.run_id }} path: | @@ -1017,7 +1017,7 @@ jobs: - name: Remove trigger label if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 continue-on-error: true with: github-token: ${{ steps.select-token.outputs.token }} diff --git a/scripts/aggregate_agent_metrics.py b/scripts/aggregate_agent_metrics.py index 8dcd793a1..5f2ee20c7 100755 --- a/scripts/aggregate_agent_metrics.py +++ b/scripts/aggregate_agent_metrics.py @@ -194,7 +194,17 @@ def _append_parse_error_detail( details[index] = replace(existing, count=existing.count + detail.count) return - details.append(replace(detail, line=None)) + for index, existing in enumerate(details): + if ( + existing.path == detail.path + and existing.artifact == detail.artifact + and existing.artifact_family == detail.artifact_family + and existing.reason == detail.reason + ): + details[index] = replace(existing, line=None, count=existing.count + detail.count) + return + + details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) def _parse_error_count(parse_error_details: list[ParseErrorDetail]) -> int: diff --git a/scripts/repo_review_evaluator.py b/scripts/repo_review_evaluator.py index 115e64661..a3725fda6 100644 --- a/scripts/repo_review_evaluator.py +++ b/scripts/repo_review_evaluator.py @@ -470,11 +470,7 @@ def build_review_execution(state: dict[str, Any]) -> dict[str, Any]: repo_path = Path(state["local_path"]) tracked_files = tracked_repo_files(repo_path) evidence_files = [path for path in tracked_files if not is_evidence_noise_file(path)] - implementation_files = [ - path - for path in evidence_files - if is_implementation_file(path) - ] + implementation_files = [path for path in evidence_files if is_implementation_file(path)] test_files = [path for path in evidence_files if is_test_file(path)] implementation_scan_files = implementation_files[:REVIEW_SCAN_FILE_LIMIT] test_scan_files = test_files[:REVIEW_SCAN_FILE_LIMIT] diff --git a/templates/consumer-repo/.github/workflows/agents-verify-to-new-pr.yml b/templates/consumer-repo/.github/workflows/agents-verify-to-new-pr.yml index e921bd8d0..ab5fff8f6 100644 --- a/templates/consumer-repo/.github/workflows/agents-verify-to-new-pr.yml +++ b/templates/consumer-repo/.github/workflows/agents-verify-to-new-pr.yml @@ -32,7 +32,7 @@ jobs: steps: - name: Check PR is merged id: check-merged - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ github.token }} script: | @@ -66,7 +66,7 @@ jobs: - name: Checkout repository if: steps.check-merged.outputs.merged == 'true' - uses: actions/checkout@v6 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 with: repository: stranske/Workflows token: ${{ steps.select-token.outputs.token }} @@ -103,7 +103,7 @@ jobs: - name: Collect verification and original issue data id: collect if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ steps.select-token.outputs.token }} script: | @@ -248,7 +248,7 @@ jobs: - name: Check chain depth limit id: chain-check if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: CHAIN_DEPTH: ${{ steps.collect.outputs.chain_depth }} MAX_CHAIN_DEPTH: '2' @@ -389,7 +389,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: LINKED_ISSUE: ${{ steps.collect.outputs.original_issue_number }} NEEDS_HUMAN_REASON: ${{ steps.extract-verdict.outputs.needs_human_reason }} @@ -505,7 +505,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: FOLLOW_UP_DEPTH: ${{ steps.chain-check.outputs.next_depth }} EXTRACTED_VERDICT: ${{ steps.extract-verdict.outputs.verdict }} @@ -677,7 +677,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_TITLE: >- ${{ steps.generate.outputs.issue_title || @@ -779,7 +779,7 @@ jobs: steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' && steps.create-issue.outputs.issue_number != '' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} @@ -816,7 +816,7 @@ jobs: steps.check-merged.outputs.merged == 'true' && steps.chain-check.outputs.exceeded != 'true' && steps.extract-verdict.outputs.needs_human != 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 env: ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} ISSUE_URL: ${{ steps.create-issue.outputs.issue_url }} @@ -870,7 +870,7 @@ jobs: FOLLOWUP_ISSUE_NUMBER: ${{ steps.create-issue.outputs.issue_number }} FOLLOWUP_ISSUE_URL: ${{ steps.create-issue.outputs.issue_url }} DISPATCH_AUTOPILOT_OUTCOME: ${{ steps.dispatch-autopilot.outcome }} - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ steps.select-token.outputs.token }} script: | @@ -1006,7 +1006,7 @@ jobs: - name: Upload terminal disposition artifact if: always() && steps.check-merged.outputs.merged == 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: verifier-terminal-disposition-${{ github.run_id }} path: | @@ -1017,7 +1017,7 @@ jobs: - name: Remove trigger label if: steps.check-merged.outputs.merged == 'true' - uses: actions/github-script@v9 + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 continue-on-error: true with: github-token: ${{ steps.select-token.outputs.token }} diff --git a/templates/consumer-repo/scripts/aggregate_agent_metrics.py b/templates/consumer-repo/scripts/aggregate_agent_metrics.py index 8dcd793a1..5f2ee20c7 100755 --- a/templates/consumer-repo/scripts/aggregate_agent_metrics.py +++ b/templates/consumer-repo/scripts/aggregate_agent_metrics.py @@ -194,7 +194,17 @@ def _append_parse_error_detail( details[index] = replace(existing, count=existing.count + detail.count) return - details.append(replace(detail, line=None)) + for index, existing in enumerate(details): + if ( + existing.path == detail.path + and existing.artifact == detail.artifact + and existing.artifact_family == detail.artifact_family + and existing.reason == detail.reason + ): + details[index] = replace(existing, line=None, count=existing.count + detail.count) + return + + details[-1] = replace(detail, line=None, count=details[-1].count + detail.count) def _parse_error_count(parse_error_details: list[ParseErrorDetail]) -> int: diff --git a/tests/scripts/test_aggregate_agent_metrics.py b/tests/scripts/test_aggregate_agent_metrics.py index 833e242ca..25616750e 100644 --- a/tests/scripts/test_aggregate_agent_metrics.py +++ b/tests/scripts/test_aggregate_agent_metrics.py @@ -416,6 +416,37 @@ def test_parse_error_contract_counts_compacted_details() -> None: assert "| artifact | artifact | metrics.ndjson | n/a | invalid-json | 7 |" in lines +def test_append_parse_error_detail_compacts_at_storage_limit() -> None: + details = [ + aggregate_agent_metrics.ParseErrorDetail( + path="metrics.ndjson", + artifact="artifact", + artifact_family="artifact", + line=index, + reason="invalid-json", + ) + for index in range(aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS) + ] + + aggregate_agent_metrics._append_parse_error_detail( + details, + aggregate_agent_metrics.ParseErrorDetail( + path="metrics.ndjson", + artifact="artifact", + artifact_family="artifact", + line=999, + reason="invalid-json", + ), + ) + + assert len(details) == aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS + assert sum(detail.count for detail in details) == ( + aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS + 1 + ) + assert details[0].line is None + assert details[0].count == 2 + + def test_read_ndjson_preserves_artifact_name_with_id_extraction_dir(tmp_path: Path) -> None: metrics_dir = ( tmp_path @@ -475,13 +506,14 @@ def test_read_ndjson_bounds_legacy_json_fallback_buffer(tmp_path: Path) -> None: entries, errors = aggregate_agent_metrics._read_ndjson([path]) assert entries == [] - assert len(errors) <= aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS + 2 + assert len(errors) <= aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS assert sum(error.count for error in errors) == ( aggregate_agent_metrics._MAX_LEGACY_JSON_FALLBACK_LINES + 2 ) - assert errors[-2].line is None - assert errors[-2].reason == "invalid-json" - assert errors[-2].count == ( + compacted_invalid_json = next( + error for error in errors if error.reason == "invalid-json" and error.line is None + ) + assert compacted_invalid_json.count >= ( aggregate_agent_metrics._MAX_LEGACY_JSON_FALLBACK_LINES + 1 - aggregate_agent_metrics._MAX_STORED_PARSE_ERROR_DETAILS