fix: address late-landing bot review findings on PR #1038 and #1041

.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

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.

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))

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))
 

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

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).

tests/unit/core/results/test_result_extension_contract.py

@@ -127,3 +127,38 @@ 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"
+
+
+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"

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."
+    )