fix: normalize non-dict dry-run schemas, fix ClickHouse DDL corruption, naive/aware datetime crash#1179
Merged
Conversation
…n, naive/aware datetime crash Consolidates three late-landing chatgpt-codex-connector review findings on PRs (#1172, #1174, #1171) that merged before the sweep could reach their branches: - benchbox/core/dryrun.py: _extract_ddl_preview called benchmark.get_schema().keys() unconditionally, but public wrapper benchmarks (e.g. JoinOrder) return a DDL string from get_schema() rather than a mapping, raising AttributeError and turning a tuned dry-run's DDL preview into a silent warning. Normalizes dict/list schemas the same way _generate_external_schema_sql() already does, falling back to an empty preview for anything else. - benchbox/core/tuning/metadata.py: the ClickHouse CREATE TABLE SQL used base_sql.replace(")", ...) to append the ENGINE clause, which rewrote every closing parenthesis in the statement -- including each VARCHAR(N) column width -- producing invalid DDL. base_sql already ends with the CREATE TABLE's closing paren, so appending the clause is sufficient and touches nothing else. - tests/uat/phases/report.py: release_gate_ordering_violations compares parsed Docker uat_lifecycle.log timestamps (naive local time, written by append_lifecycle_log) against native_stage_completed_at, which is offset-aware in production (orchestrator.py's datetime.now().astimezone(), #1162). Comparing naive and aware datetimes raised TypeError, crashing make uat-gate-check whenever a Docker stage had an action=up lifecycle entry. Normalizes the naive side onto native_stage_completed_at's awareness before comparing, preserving the existing naive-to-naive behavior other callers rely on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
![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: 9841c31058
ℹ️ 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".
| # astimezone(), which interprets a naive datetime as local time | ||
| # and attaches the current local UTC offset -- before comparing. | ||
| if timestamp.tzinfo is None and native_stage_completed_at.tzinfo is not None: | ||
| timestamp = timestamp.astimezone() |
There was a problem hiding this comment.
When make uat-gate-check is run in a different local timezone than the sweep that produced uat_lifecycle.log, timestamp.astimezone() interprets the naive Docker timestamp in the checker process's timezone, not the producer's timezone recorded on native_stage_completed_at. For example, a valid PDT run with stage 1 completed at 2026-05-30T01:00:00-07:00 and Docker up at naive 2026-05-30T02:00:00 is reported as a violation if the gate check runs under UTC. Use the boundary's offset for naive log entries or emit offset-aware lifecycle timestamps instead of attaching the current process timezone here.
Useful? React with 👍 / 👎.
joeharris76
pushed a commit
that referenced
this pull request
Jul 16, 2026
Brings in develop's post-branch-cut landings: #1179 (dry-run schema normalization + ClickHouse DDL corruption fix in metadata.py's own hand-rolled metadata-table DDL, unrelated to this branch's clickhouse workload.py tuned-rendering changes), #1170/#1177/#1178 (explorer ingest, soundness tests incl. the original test_capability_source_drift.py from PR #1177, docs/resolver), #1185/#1187/#1188/#1189 (corpus, metadata.py drift-detection widening, template packaging incl. tuning_resolver, FK load ordering), #1191/#1192 (TODO bookkeeping). Only one real conflict: tests/unit/core/tuning/test_capability_source_drift.py was created independently on both sides (this branch's w1 registry- consolidation tests; PR #1177's pre-existing three-source drift allowlist tests). Resolved by keeping both: this branch's registry-derivation + frozen-baseline tests as the primary content, with #1177's TestGeneratorRegistryVsCompatibilityMapDrift/TestCapabilityMapperVsCompatibilityMapDrift classes appended unchanged (their assertions cover the three sources' independent, as-presented-today behavior, which this branch didn't change -- only how _PLATFORM_COMPATIBILITY_MAP and the workload-profile dispatch are *computed* changed, not what they return). All 16 tests in the merged file pass. No other conflicts: benchbox/core/dryrun.py, benchbox/core/tuning/metadata.py, and benchbox/cli/tuning_resolver.py merged cleanly (three-way, no textual overlap with this branch's changes) since develop's edits to those files touch different functions/sections than this branch modified or read from. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
joeharris76
pushed a commit
that referenced
this pull request
Jul 16, 2026
Merges 8 PRs that landed on develop since this branch was cut: #1178 (warn_sql_auto_mode relocated into tuning_resolver.py; run.py slimmed), #1179 (dry-run fixes), #1185 (corpus cleanup), #1187 (drift-detection persistence widening), #1188 (packaged tuning templates: adds TuningSource.PACKAGED_RESOURCE and a last-resort packaged discovery tier in tuning_resolver.py), #1189 (FK-aware load ordering), #1191/#1192 (TODO close-out batches). One real conflict, in benchbox/cli/tuning_resolver.py: both branches added an import line in the same spot (this branch's `from benchbox.core.tuning import modes as tuning_modes`; #1188's `from benchbox.core.tuning.packaged_ templates import list_packaged_templates, packaged_template_path`). Resolved by keeping both -- the rest of the file (this branch's `TuningResolution.canonical_mode` property, #1178's `warn_sql_auto_mode`, and #1188's `TuningSource.PACKAGED_RESOURCE` tier/enum/branches) merged automatically with no further conflicts. Verified the three-way composition holds: `canonical_mode` only special- cases (TUNED, FALLBACK) -> "tuned-fallback" and CUSTOM_FILE -> "custom"; every other (mode, source) pair -- including the new (TUNED, PACKAGED_RESOURCE) -- falls through to the plain mode value, so a packaged- template resolution correctly records as "tuned", not "tuned-fallback" (a packaged template is a genuine curated template, just bundled with the package instead of found under examples/tunings/). Added a dedicated pin for this (test_tuned_via_packaged_resource_is_tuned_not_tuned_fallback) in tests/unit/core/tuning/test_tuning_mode_vocabulary.py. The branch's own TestFallbackLabelingEndToEnd e2e tests needed updating: they previously forced a template-discovery miss for duckdb/tpch via BENCHBOX_TUNING_PATH + an empty cwd, which no longer produces a genuine "no template anywhere" fallback now that the packaged tier (#1188) ships a duckdb/tpch template with the package itself. Switched to duckdb/coffeeshop (no template in examples/tunings/, the packaged tier, or anywhere else), with a dedicated guard test asserting that premise so a future packaged- template addition for that pair fails loudly instead of silently invalidating the fallback-labeling coverage. Bumped _project/config/fast_test_lane_policy.json's max_fast_tests 25000 -> 25050: this branch's merge collects 25022 fast tests (develop alone collects 24995); the added coverage (canonical_mode/tuned-fallback/ official-refusal pins, the PACKAGED_RESOURCE composition test above, and the physical_mechanisms unknown-vs-empty ingest-pipeline regression tests from the prior review round) is genuine fast-lane-appropriate unit coverage, not scope creep. results-explorer/ and Home.tsx were untouched by any of the merged develop commits, so this branch's ADR-2 facet/receipt work carries forward unmodified. Verified post-merge: `BENCHBOX_SKIP_TEST_LOCK=1 pytest tests/unit -q -k tuning -n 0` (1300 passed), `cd results-explorer && npx vitest run` (798 passed), `npx tsc --noEmit` (clean), `make lint` (clean), `make ci-lint` (clean, including the fast-lane guardrail after the ceiling bump above). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
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
Consolidates three late-landing
chatgpt-codex-connectorreview findings on PRs that merged before the sweep could reach their branches:benchbox/core/dryrun.py):_extract_ddl_previewcalledbenchmark.get_schema().keys()unconditionally, but public wrapper benchmarks (e.g.JoinOrder) return a DDL string fromget_schema()rather than a mapping — a shippedexamples/tunings/duckdb/joinorder_tuned.yamltemplate exists, so a tuned JoinOrder dry-run raisedAttributeErrorand silently degraded to a warning instead of showing the preview. Normalizes dict/list schemas the same way_generate_external_schema_sql()already does, falling back to an empty preview for anything else.benchbox/core/tuning/metadata.py): the ClickHouseCREATE TABLESQL usedbase_sql.replace(")", ...)to append theENGINEclause, which rewrote every closing parenthesis in the statement — including eachVARCHAR(N)column width — producing invalid DDL (VARCHAR(255) ENGINE = MergeTree() ... NOT NULL, repeated per column).base_sqlalready ends with theCREATE TABLE's own closing paren, so appending the clause is sufficient and touches nothing else.tests/uat/phases/report.py):release_gate_ordering_violationscompares parsed Dockeruat_lifecycle.logtimestamps (naive local time, written byappend_lifecycle_log) againstnative_stage_completed_at, which is offset-aware in production (orchestrator.py'sdatetime.now().astimezone(), from the feat/uat release gate enforcement #1162 fix). Comparing a naive and an aware datetime raisesTypeError, crashingmake uat-gate-checkwhenever a Docker stage had anaction=uplifecycle entry. Normalizes the naive side ontonative_stage_completed_at's awareness right before comparing, which also preserves the existing naive-to-naive behavior other callers (and existing tests) rely on.Type of Change
Testing
uv run -- python -m pytest tests/unit/core/test_dryrun.py tests/unit/core/tuning/ tests/uat/test_report.py -q— 577 passedgit stashthe source file) and pass with ituv run -- ruff check/ruff format --check/ty checkon touched files — cleanPublic Contract Check
Documentation
Code Quality
Artifact Hygiene
Notes
Bot review findings on PRs #1172, #1174, and #1171, all merged before this sweep ran, consolidated into one branch per the standing PR-review-followup sweep process. None of the touched files are under a CODEOWNERS soundness-critical path, so this PR is eligible for squash auto-merge once CI passes.
Generated by Claude Code