From 00a76cf4b79d18df880922982f861cf83bbc91f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 11:33:05 +0000 Subject: [PATCH 1/2] fix: address late-landing bot review findings on PR #1038 and #1041 - #1038 (plan_metadata_utils.py): a brand-new query_id whose fingerprint_version differs from prev's default fell into the version-comparability-mismatch branch, which fell back to prev_metadata.plan_versions.get(query_id, 0) - writing an invalid version 0 (validate_plan_metadata rejects versions < 1). Bypass that branch entirely for query_ids absent from prev_metadata: there is no prior recording to be "not comparable" to, so a new query always starts at version 1. - #1038 (schema.py): plan_capture_error was added to ResultBuilder's in-memory query_results dict, but the actual JSON export goes through schema.py's _build_query_results_section, a separate serialization path that copies only specific fields onto the compact queries[] entry. Added plan_capture_error there too, so the real DataFrame capture-failure cause reaches the persisted bundle, not just the in-memory representation. - #1041 (validate-submission.yml): the "Reject non-maintainer vendor/ additions" step's OWNER/MEMBER/COLLABORATOR allowlist didn't account for the trusted same-repo auto/results-mirror-* PRs that sync-results-data-to-published.yml opens with secrets.GITHUB_TOKEN (authored by github-actions[bot]), so a maintainer's vendor bundle published through that path was blocked. Added the same two-signal trust check (head.repo.fork == false AND the mirror branch pattern) the "Validate bundles" step's manifest waiver already uses. Each fix has a regression test verified to fail before the fix and pass after. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- .github/workflows/validate-submission.yml | 30 +++++++++++++-- benchbox/core/manifest/plan_metadata_utils.py | 13 ++++++- benchbox/core/results/schema.py | 6 +++ benchbox/core/results/schema_specs.yaml | 1 + .../unit/core/manifest/test_plan_metadata.py | 23 ++++++++++++ .../results/test_result_extension_contract.py | 16 ++++++++ .../test_validate_submission_vendor_gate.py | 37 +++++++++++++++++++ 7 files changed, 120 insertions(+), 6 deletions(-) diff --git a/.github/workflows/validate-submission.yml b/.github/workflows/validate-submission.yml index 9dd513712..1d28bdb1c 100644 --- a/.github/workflows/validate-submission.yml +++ b/.github/workflows/validate-submission.yml @@ -49,6 +49,18 @@ jobs: env: BASE_SHA: ${{ github.event.pull_request.base.sha }} AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} + # Same two-signal trust check as the "Validate bundles" step below + # (see its comment for the full reasoning): a maintainer adding a + # vendor bundle on develop is published through a same-repo + # auto/results-mirror-* PR opened by sync-results-data-to-published.yml + # with secrets.GITHUB_TOKEN, so its author is github-actions[bot] - + # never OWNER/MEMBER/COLLABORATOR. Without this, that trusted mirror + # PR falls through to the else branch and blocks legitimate + # maintainer vendor bundles. head.repo.fork is the load-bearing + # signal (unforgeable from a fork); the branch-name pattern alone + # would be spoofable. + IS_FORK: ${{ github.event.pull_request.head.repo.fork }} + HEAD_REF: ${{ github.head_ref }} run: | # The vendor-supplied trust label is RANKING-ELIGIBLE and is granted # purely by a bundle living under results-data/bundles/vendor/ (see @@ -65,15 +77,25 @@ jobs: VENDOR_CHANGES=$(git diff --name-only "$BASE_SHA"...HEAD -- \ 'results-data/bundles/vendor/**' || true) if [ -n "$VENDOR_CHANGES" ]; then + TRUSTED_MIRROR="false" + if [ "$IS_FORK" = "false" ]; then + case "$HEAD_REF" in + auto/results-mirror-*) TRUSTED_MIRROR="true" ;; + esac + fi case "$AUTHOR_ASSOCIATION" in OWNER | MEMBER | COLLABORATOR) echo "Maintainer ($AUTHOR_ASSOCIATION) vendor/ addition - allowed." ;; *) - echo "::error::This PR adds or modifies files under results-data/bundles/vendor/, which grants the ranking-eligible vendor-supplied label. Only maintainers may do this:" - printf '%s\n' "$VENDOR_CHANGES" - echo "Community submissions belong in results-data/bundles/ (not vendor/). Author association: ${AUTHOR_ASSOCIATION}." - exit 1 + if [ "$TRUSTED_MIRROR" = "true" ]; then + echo "Trusted same-repo mirror PR ($HEAD_REF) vendor/ addition - allowed." + else + echo "::error::This PR adds or modifies files under results-data/bundles/vendor/, which grants the ranking-eligible vendor-supplied label. Only maintainers may do this:" + printf '%s\n' "$VENDOR_CHANGES" + echo "Community submissions belong in results-data/bundles/ (not vendor/). Author association: ${AUTHOR_ASSOCIATION}." + exit 1 + fi ;; esac fi diff --git a/benchbox/core/manifest/plan_metadata_utils.py b/benchbox/core/manifest/plan_metadata_utils.py index e849a4282..ae57c6832 100644 --- a/benchbox/core/manifest/plan_metadata_utils.py +++ b/benchbox/core/manifest/plan_metadata_utils.py @@ -131,8 +131,17 @@ def update_plan_versions( _require_matching_scheme(prev_metadata, current_metadata, operation="diff") for query_id, current_fp in current_metadata.plan_fingerprints.items(): - prev_fp = prev_metadata.plan_fingerprints.get(query_id) - prev_version = prev_metadata.plan_versions.get(query_id, 0) + if query_id not in prev_metadata.plan_fingerprints: + # Genuinely new query: no prior recording exists to be "not + # comparable" to, so the fingerprint-version-mismatch branch below + # must not apply here - it would fall back to + # prev_metadata.plan_versions.get(query_id, 0), writing an invalid + # version 0 (#1038 review). Always start a new query at version 1. + current_metadata.plan_versions[query_id] = 1 + continue + + prev_fp = prev_metadata.plan_fingerprints[query_id] + prev_version = prev_metadata.plan_versions.get(query_id, 1) # Legacy metadata predating plan_fingerprint_versions defaults to 1 # (LEGACY_FINGERPRINT_VERSION), matching QueryPlanDAG.from_dict's own # legacy-bundle default. diff --git a/benchbox/core/results/schema.py b/benchbox/core/results/schema.py index a1ab59041..edc289b96 100644 --- a/benchbox/core/results/schema.py +++ b/benchbox/core/results/schema.py @@ -388,6 +388,12 @@ def _build_query_results_section( entry["digest"] = result_digest if qr.get("dataframe_skip_summary"): entry["dataframe_skip_summary"] = qr["dataframe_skip_summary"] + # Real DataFrame plan-capture-error cause (qpc-05 / F4.4, #1038 review): + # this is the JSON export path, a separate serialization from + # ResultBuilder._format_query_results (in-memory only) - without this, + # the field was lost on export/reload despite being carried in memory. + if qr.get("plan_capture_error") is not None: + entry["plan_capture_error"] = qr["plan_capture_error"] queries_list.append(order_dict(entry, QUERY_KEY_ORDER)) diff --git a/benchbox/core/results/schema_specs.yaml b/benchbox/core/results/schema_specs.yaml index 6f09b96fe..289085b77 100644 --- a/benchbox/core/results/schema_specs.yaml +++ b/benchbox/core/results/schema_specs.yaml @@ -28,6 +28,7 @@ query_key_order: - status - digest - dataframe_skip_summary +- plan_capture_error config_key_order: - compression - seed diff --git a/tests/unit/core/manifest/test_plan_metadata.py b/tests/unit/core/manifest/test_plan_metadata.py index 93b8167b9..1c4b18372 100644 --- a/tests/unit/core/manifest/test_plan_metadata.py +++ b/tests/unit/core/manifest/test_plan_metadata.py @@ -325,6 +325,29 @@ def test_new_queries_start_at_version_one(self) -> None: assert current.plan_versions["q1"] == 3 assert current.plan_versions["q2"] == 1 + def test_new_query_across_fingerprint_version_boundary_starts_at_one(self) -> None: + """#1038 review: a brand-new query_id (absent from prev entirely) must + always start at version 1, even when its fingerprint_version differs + from prev's default - there is no prior recording to be "not + comparable" to, so the version-mismatch branch must not apply and + fall back to a 0 default (invalid per validate_plan_metadata).""" + prev = PlanMetadata( + plan_fingerprints={"q1": "a" * 64}, + plan_versions={"q1": 3}, + plan_fingerprint_versions={"q1": 1}, + ) + current = PlanMetadata( + plan_fingerprints={"q1": "a" * 64, "q2": "b" * 64}, # q2 is new, captured as v2 + plan_versions={}, + plan_fingerprint_versions={"q1": 1, "q2": 2}, + ) + + update_plan_versions(prev, current) + + assert current.plan_versions["q1"] == 3 + assert current.plan_versions["q2"] == 1 + assert not validate_plan_metadata(current) + def test_fingerprint_version_boundary_does_not_bump_version(self) -> None: """A pure fingerprint-encoding-version bump (qpc-03 v1 -> v2) makes the fingerprint STRING change for the same logical plan (#1028 review). diff --git a/tests/unit/core/results/test_result_extension_contract.py b/tests/unit/core/results/test_result_extension_contract.py index 1676e159d..a19635233 100644 --- a/tests/unit/core/results/test_result_extension_contract.py +++ b/tests/unit/core/results/test_result_extension_contract.py @@ -127,3 +127,19 @@ def test_platform_specific_extensions_round_trip_through_loader() -> None: assert reconstructed.platform_raw_metadata == payload["platform"]["raw_metadata"] assert round_trip["comparisons"] == payload["comparisons"] assert round_trip["phases"]["migration"] == payload["phases"]["migration"] + + +def test_plan_capture_error_reaches_the_exported_query_row() -> None: + """#1038 review: plan_capture_error was added to the in-memory + BenchmarkResults.query_results dict, but build_result_payload's + _build_query_results_section is a SEPARATE serialization path that + copies only specific fields onto the compact queries[] entry - the real + DataFrame capture-failure cause must reach the actual exported/persisted + JSON, not just the in-memory representation.""" + result = _extension_result() + result.query_results[0]["plan_capture_error"] = "TypeError: unsupported operand" + + payload = build_result_payload(result) + + entry = next(q for q in payload["queries"] if q["id"] == "1") + assert entry["plan_capture_error"] == "TypeError: unsupported operand" diff --git a/tests/unit/workflows/test_validate_submission_vendor_gate.py b/tests/unit/workflows/test_validate_submission_vendor_gate.py index 0e2664d60..0e2a8784d 100644 --- a/tests/unit/workflows/test_validate_submission_vendor_gate.py +++ b/tests/unit/workflows/test_validate_submission_vendor_gate.py @@ -10,6 +10,7 @@ from __future__ import annotations +import re from pathlib import Path import pytest @@ -60,3 +61,39 @@ def test_vendor_gate_fails_the_pr() -> None: # A non-maintainer vendor/ addition must hard-fail the job. assert "exit 1" in script assert "::error::" in script + + +def test_vendor_gate_allows_trusted_same_repo_mirror() -> None: + """#1041 review: a maintainer vendor/ addition on develop is published + through a same-repo auto/results-mirror-* PR opened by + sync-results-data-to-published.yml with secrets.GITHUB_TOKEN, authored by + github-actions[bot] - never OWNER/MEMBER/COLLABORATOR. The gate must let + that trusted mirror through instead of blocking it, gated on the same + unforgeable head.repo.fork signal the "Validate bundles" step's manifest + waiver uses (github.head_ref alone is attacker-controlled on fork PRs). + """ + step = _vendor_gate_step() + env = step.get("env") or {} + run = step["run"] + + fork_var = next((k for k, v in env.items() if "head.repo.fork" in str(v)), None) + assert fork_var is not None, ( + "The vendor gate step must bind github.event.pull_request.head.repo.fork " + "into the env so the mirror-bot bypass can gate on it." + ) + + guard_match = re.search( + rf'\[\s*"\${fork_var}"\s*=\s*"false"\s*\]\s*;\s*then\n(.*?)\nfi\b', + run, + re.DOTALL, + ) + assert guard_match is not None, ( + f'Could not find a `if [ "${fork_var}" = "false" ]; then ... fi` block - ' + "the fork guard must wrap a then/fi body, not just appear in a condition." + ) + guarded_body = guard_match.group(1) + assert "auto/results-mirror-*" in guarded_body, ( + "The mirror branch pattern check must be NESTED inside the " + f'[ "${fork_var}" = "false" ] guard body, not merely present elsewhere in the ' + "step - otherwise a same-named branch on a fork PR could still reach the bypass." + ) From 7947f16fb0473a4cb292cdd86f9823695b19bee9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 11:44:47 +0000 Subject: [PATCH 2/2] fix: reconstruct plan_capture_error on result reload (#1052 review) The export fix alone was incomplete: _reconstruct_query_results() copied iter/stream/run_type/test_type from the compact queries[] entry but not plan_capture_error, so an export -> load -> re-export round trip silently dropped the real DataFrame capture-failure cause again even though it survived the first export. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --- benchbox/core/results/loader.py | 6 ++++++ .../results/test_result_extension_contract.py | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/benchbox/core/results/loader.py b/benchbox/core/results/loader.py index 05913135a..fee96aa0f 100644 --- a/benchbox/core/results/loader.py +++ b/benchbox/core/results/loader.py @@ -550,6 +550,12 @@ def _reconstruct_query_results( result["run_type"] = q["run_type"] if q.get("test_type"): result["test_type"] = q["test_type"] + if q.get("plan_capture_error") is not None: + # Real DataFrame plan-capture-error cause (qpc-05 / F4.4, #1052 + # review): without this, the export -> load -> re-export round + # trip silently drops it again even though it survives the JSON + # export itself. + result["plan_capture_error"] = q["plan_capture_error"] if status not in ("SUCCESS", "SKIPPED") and query_id is not None: error_queue = query_errors_by_id.get(normalize_query_id(query_id)) diff --git a/tests/unit/core/results/test_result_extension_contract.py b/tests/unit/core/results/test_result_extension_contract.py index a19635233..484992f13 100644 --- a/tests/unit/core/results/test_result_extension_contract.py +++ b/tests/unit/core/results/test_result_extension_contract.py @@ -143,3 +143,22 @@ def test_plan_capture_error_reaches_the_exported_query_row() -> None: entry = next(q for q in payload["queries"] if q["id"] == "1") assert entry["plan_capture_error"] == "TypeError: unsupported operand" + + +def test_plan_capture_error_survives_export_load_reexport_round_trip() -> None: + """#1052 review: the export fix alone is incomplete - reload + (reconstruct_benchmark_results) must also carry plan_capture_error + forward, or export -> load -> re-export silently drops it again even + though it survives the first export.""" + result = _extension_result() + result.query_results[0]["plan_capture_error"] = "TypeError: unsupported operand" + + payload = build_result_payload(result) + reconstructed = reconstruct_benchmark_results(payload) + round_trip = build_result_payload(reconstructed) + + reconstructed_row = next(q for q in reconstructed.query_results if q.get("query_id") == "1") + assert reconstructed_row["plan_capture_error"] == "TypeError: unsupported operand" + + round_trip_entry = next(q for q in round_trip["queries"] if q["id"] == "1") + assert round_trip_entry["plan_capture_error"] == "TypeError: unsupported operand"