chore: sweep fixes for late-landing bot findings (#1198, #1199, #1195, #1179)#1202
Merged
Merged
Conversation
PgDuckDBDDLGenerator inherits PostgreSQLDDLGenerator.SUPPORTED_TUNING_TYPES,
which includes sorting -- a tuned SORTING config is processed (as a
CLUSTER-index pair via postgresql.py's shared clustering path, then the
CLUSTER statement filtered out by pg_duckdb.py's override), not rejected.
The registry entry only covered PARTITIONING/CLUSTERING, so
get_capability("pg-duckdb", SORTING) returned None ("not compatible")
instead of rendered_via="none" ("compatible, but execution renders
nothing") -- and benchbox tuning platforms silently omitted the row.
release_gate_ordering_violations used timestamp.astimezone() to attach a timezone to append_lifecycle_log()'s naive Docker timestamps, but astimezone() interprets a naive datetime as being in the *checker process's* current local timezone, not the producer's. A boundary completed at 2026-05-30T01:00:00-07:00 (PDT) with a genuinely-later Docker naive timestamp of 2026-05-30T02:00:00 would get +00:00 attached instead of -07:00 when the checker runs under UTC, making it appear hours before the boundary and firing a false ordering violation. Both timestamps come from the same host/sweep run, so replace() with the boundary's own tzinfo is correct where astimezone() was not.
…endency gaps - uat-clickhouse-server-loader-type-coverage.yaml: w2's notes still told the implementer to use type defaults for genuinely non-Nullable columns when source semantics permit, directly contradicting the now-ratified open_questions[0] decision (no coercion, ever; fail loudly on empty-into-NOT-NULL). Rewrote w2 to match. Also added the ratified decision item (uat-clickhouse-loader-empty-value-nullability) to deps.needs so the DAG reflects the real prerequisite, not just prose. - tpch-throughput-parameter-aware-rowcounts.yaml: scope_limit restricted to tests/uat/throughput.py + tests/uat/test_throughput.py, but the real validation seam is benchbox/core/tpch + expected_results + validation (per PR #1142) and the parent's own anti_patterns explicitly forbid special-casing in tests/uat. Expanded scope_limit to the real seam. - uat-throughput-result-path-manifest.yaml: scope_limit omitted tests/uat/runner.py, whose `official` branch calls resolve_official_result_path directly without passing/parsing stdout_text -- if w1 picks the quiet-stdout-line contract, runner.py's official branch must change too, or w2 cannot retire the glob within scope. Added runner.py + its test file, and documented the handoff in w2's notes.
joeharris76
enabled auto-merge (squash)
July 17, 2026 02:56
![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: 139ca6046f
ℹ️ 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".
The #1179 fix (this same branch) patched release_gate_ordering_violations to reuse the boundary's own offset for naive Docker timestamps, but that degrades across a DST transition mid-sweep: a boundary and a later event can genuinely carry different UTC offsets (e.g. PDT -07:00 -> PST -08:00 after fall-back), and reusing the boundary's fixed offset for the later event misreads its wall-clock time, producing a false ordering violation. Fix at the real source instead: append_lifecycle_log() now writes datetime.now().astimezone() (offset-aware) rather than plain datetime.now() (naive), so each Docker lifecycle event carries its own real offset and survives a DST transition correctly. The boundary-offset substitution in release_gate_ordering_violations remains as a documented best-effort fallback for uat_lifecycle.log files written by an older BenchBox version that never recorded an offset at all.
…llowup-sweep-15 # Conflicts: # _project/TODO/main/planning/uat-clickhouse-server-loader-type-coverage.yaml
…ep-15 FAST_LANE_VIOLATION: fast lane count 25085 exceeds limit 25080. Shared repo-wide fast-lane budget outpaced again by cumulative batch merges. Bump max_fast_tests 25080 -> 25100 per maintainer direction.
…(25220) Resolves the ratchet conflict with the byte-identical composed policy content shared across the open-PR merge queue (see the ceiling note). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018y41mPzRqiLqvZLZCxxS6j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Consolidated fixes for
chatgpt-codex-connectorreview findings that landed on already-merged PRs:pg-duckdb's capability registry entry only coveredPARTITIONING/CLUSTERING, butPgDuckDBDDLGeneratorinheritsPostgreSQLDDLGenerator.SUPPORTED_TUNING_TYPES, which includesSORTING(processed as a CLUSTER-index pair, then the CLUSTER statement filtered out by pg_duckdb's override).get_capability("pg-duckdb", SORTING)returnedNone("not compatible") instead ofrendered_via="none"("compatible, execution renders nothing"), sobenchbox tuning platformssilently omitted the row. Added the missing entry.uat-clickhouse-server-loader-type-coverage.yaml'sw2notes still instructed the implementer to use type defaults for genuinely non-Nullable columns "when source semantics permit" — directly contradicting the now-ratifiedopen_questions[0]decision (no coercion, ever; fail loudly on empty-into-NOT-NULL as a benchmark data defect). Rewrotew2to match the ratified policy. Also added the ratified decision item todeps.needsso the dependency graph reflects the real prerequisite instead of only prose.scope_limitgaps that would make their own stated work impossible —tpch-throughput-parameter-aware-rowcounts.yamlrestrictedonly_modifytotests/uat/*even though the real validation seam isbenchbox/core/tpch/expected_results/validation(per PR fix(validation): exclude TPC-H Q11/16/18/20 from EXACT row-count checks under non-reference seeds #1142) and the parent's ownanti_patternsexplicitly forbid special-casing intests/uat;uat-throughput-result-path-manifest.yamlomittedtests/uat/runner.py, whoseofficialbranch callsresolve_official_result_pathdirectly without passing/parsingstdout_text— ifw1picks the quiet-stdout-line contract,runner.py's official branch must change too, orw2can't retire the glob within scope. Expanded bothscope_limits and documented the handoff.release_gate_ordering_violationsusedtimestamp.astimezone()to attach a timezone to naive Docker lifecycle timestamps, butastimezone()interprets a naive datetime as being in the checker process's current local timezone, not the producer's. A boundary completed at2026-05-30T01:00:00-07:00(PDT) with a genuinely-later Docker naive timestamp of2026-05-30T02:00:00would get+00:00attached instead of-07:00when the checker runs under UTC, making it appear hours before the boundary and firing a false ordering violation. Fixed to use the boundary's own offset (.replace(tzinfo=...)) since both timestamps come from the same host/sweep run.Type of Change
Testing
uv run --project _project/scripts -- python _project/scripts/validate_todo.py <each edited TODO file>— all validuv run --project _project/scripts -- python _project/scripts/todo_cli.py check-graph— 125 items, no cycles/dangling refsuv run -- python -m pytest tests/unit/core/tuning/test_capability_source_drift.py -q— 32 passed (new regression test, verified genuine failure pre-fix via stash discipline)uv run -- python -m pytest tests/uat/test_report.py -q— 23 passed (new regression test, verified genuine failure pre-fix via stash discipline)uv run -- python -m pytest tests/unit/core/tuning tests/uat/test_report.py -q— 577 passeduv run -- ruff check/ruff format --checkon all changed files — cleanPublic Contract Check
docs/reference/public-contracts.md, or this PR does not change public, beta-public, deprecated, generated, experimental, or repo-only contract surfaces.Documentation
Code Quality
Artifact Hygiene
Notes
The 4th #1195 finding (
uat-clickhouse-loader-empty-value-nullability.yaml:43, "keep w0 pending until the required inventory exists") needed no fix: that decision item is nowstatus: Completedand moved toDONE/, withw1/w2notes explicitly recording "RATIFIED 2026-07-16 by the repo owner." The real human sign-off the automated completeness gate exists to guarantee already happened, superseding the concern.None of the touched paths fall under CODEOWNERS soundness-critical prefixes (
benchbox/core/**/validation.py,benchbox/core/equivalence/**,benchbox/core/query_plans/parsers/**,benchbox/core/expected_results/**, etc.), so this is eligible for standard squash auto-merge.Generated by Claude Code