fix: consolidated bot review follow-up sweep (7 findings across 6 merged PRs)#1144
Merged
Conversation
…1128 review) archive_dir's broad `except Exception` converted a checksum/manifest integrity failure into a silent pytest.skip, defeating the point of a suite that exists to catch corrupted canonical data. Narrowed to DownloadError/ImportError/OSError (true availability failures); a ChecksumMismatchError now propagates and fails the test. DuckDB's `~` operator is regexp_full_match, not a contains-match, so the UTF-8 fidelity check only selected rows consisting entirely of one non-ASCII character - a manual DuckDB check confirmed it matched zero rows even on data containing "Amélie". Switched to regexp_matches() and recomputed the pinned UTF8_FIDELITY counts/hashes against the real archive with the corrected query (title.title: 5->155075 rows, etc.). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…aceholder-rows (#1130 review) The primary path (w0/w1, actually taken) landed #1117 as-is rather than the fallback test-only rewrite, so this completed item's scope_limit only naming the fallback's test file understated what was actually authorized/touched. Added the two paths #1117 changed (connection_wrappers.py + its unit test file) for an accurate audit trail; no scope reopened, both were already merged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…in develop-post-merge (#1136 review) Two gaps in the signature-aware auto-revert decision: 1. The current-run artifact check only required at least one signature.json to exist (compgen -G glob match), not all four expected gate artifacts. If one gate crashed/timed out before its always()-run signature step could upload, the diff still ran on the remaining three - a missing gate's real failure was silently excluded instead of forcing fail-open. Now checks each of the four expected signature-<job> directories explicitly. 2. fast-test/medium-test's signature emission trusted the junit XML unconditionally: a job can fail for a reason junit never records (e.g. pytest-cov's --cov-fail-under gate - every testcase passes, the step still fails), producing an empty-but-"clean" signature that made the diff see no new failures and silently suppress a real revert. Added a --job-failed flag to scripts/post_merge_signature.py's build command: when the job failed and the junit-derived signature is empty, it falls back to a job-failure descriptor instead of trusting the false-clean junit. A job that failed WITH real testcase failures keeps the more precise junit signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
… review) TPC-H/TPC-DS power test harnesses called cursor.fetchall() solely to count/discard rows (result_count is the only consumer), which now trips PlatformAdapterCursor's placeholder-materialization warning on every query for a normal validation-mode run - noise that defeats the warning's purpose of flagging genuine VALUE-dependent placeholder consumption. - tpch/power_test.py, tpcds/power_test.py: `_query_result_count` checks platform_result["rows_returned"] first (never materializes) and only falls back to fetchall() for cursors with no reported count (raw DB-API cursors, test doubles). - tpcds/power_test.py's warm-up loop drains via row_count() when available, falling back to fetchall() only for cursors without it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
#1138 review) Both verification commands defined success as "no grep hits" but grep exits 1 on no match and 0 on a match - the gates had the pass/fail logic backwards, so a future implementer's scripted verification would report success/failure exactly opposite of the real outcome. - uat-execute-path-unification.yaml: `grep ... && echo FOUND || echo CLEAN` always exits 0 regardless of which branch prints - the FOUND branch now exits 1. - uat-resume-retirement-artifact-durability.yaml: both "no hits = success" pipelines now invert their exit status with `!`, matching the `! grep -q ...` idiom already used elsewhere in this TODO tree. Verified empirically against the real repo (dead code still present - correctly reports FOUND, exit 1) and simulated clean/dirty/filtered cases for the resume-reference gates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…ocking shutdown (#1141 review) executor.shutdown(wait=False) left cancel_futures at its default False, so when max_workers < num_streams, a future still QUEUED (never dispatched to a worker) at the timeout deadline would start executing once a worker eventually freed up - well after execute() had already returned and counted that stream as timed out. That created extra, unexpected DB work overlapping with whatever phase ran next. cancel_futures=True closes the gap: it cancels only futures that have not started running (Python's documented ThreadPoolExecutor.shutdown semantics), leaving already-running (leaked/abandoned) futures untouched - the can't-forcibly-cancel-a-running-thread invariant is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
joeharris76
enabled auto-merge (squash)
July 11, 2026 20:07
![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: 7ecb2848ea
ℹ️ 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".
#1137's fix routed count-only power-test paths through row_count()/rows_returned to avoid a placeholder-materialization warning, but for raw DB-API cursors without platform_result, _query_result_count still falls back to fetchall() - and that fallback call had moved to after connection.commit(). Some drivers with unbuffered SELECT results reject or invalidate commit() while rows are still unread, which would misrecord successful power queries as failures. Restores the pre-#1137 drain-before-commit ordering in both tpch/power_test.py and tpcds/power_test.py::_execute_one_query while keeping #1137's benefit (no eager fetchall() when rows_returned is already available). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
Catches this stale review-followup-sweep branch up with develop so CI can run again (was mergeable_state=dirty with zero CI runs).
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
Late-landing
chatgpt-codex-connectorbot review follow-ups. Each PR listed below was already merged before its review completed, so all fixes are consolidated into this one sweep branch offdevelopper the established review-followup pattern. (A 7th finding, on PR #1139, was found already fixed by concurrent work on that PR's own branch — verified adequate and replied+resolved there directly, not duplicated here.)Findings fixed
tests/integration/test_joinorder_full_archive.py: a broadexcept Exceptionconverted a checksum/manifest integrity failure into a silentpytest.skip, defeating the point of a suite that exists to catch corrupted canonical data — narrowed to true availability failures (DownloadError/ImportError/OSError). Also fixed the UTF-8 fidelity check: DuckDB's~operator isregexp_full_match, not a contains-match, so it only matched rows consisting entirely of one non-ASCII character (empirically confirmed it matched zero rows even on data containing "Amélie"); switched toregexp_matches()and recomputed the pinned counts/hashes against the real archive (title.title: 5 → 155075 rows)._project/DONE/auto-revert-incident/active/fix-session-isolation-placeholder-rows.yaml: this completed item'sscope_limitonly named the fallback path's test file, but the primary path actually taken (landing fix(throughput): preserve first_row when padding cursor rows; count raw-cursor rows inside the timed window #1117 as-is) touchedconnection_wrappers.py+ its unit test file — added for an accurate audit trail..github/workflows/develop-post-merge.yml+scripts/post_merge_signature.py: (a) the current-run signature check only required at least onesignature.jsonto exist, not all four expected gate artifacts — a crashed/cancelled gate's missing artifact no longer silently diffs a partial signature; (b) a gate that fails for a reason junit never records (e.g.pytest-cov's--cov-fail-undergate — every testcase passes, the job still fails) produced an empty-but-"clean" signature that could silently suppress a real revert — added a--job-failedflag that falls back to a job-failure descriptor when the job failed and junit found nothing.benchbox/core/tpch/power_test.py,benchbox/core/tpcds/power_test.py: the TPC power-test harnesses calledcursor.fetchall()solely to count/discard rows, trippingPlatformAdapterCursor's new placeholder-materialization warning on every query for a normal run. Routed all four call sites (both power tests' per-query counting, TPC-DS warm-up, TPC-DS_execute_query) throughplatform_result["rows_returned"]/row_count()first, falling back tofetchall()only for cursors with no reported count._project/TODO/uat-hardening/planning/*.yamlfiles: verification commands defined success as "no grep hits", but grep exits 1 on no match and 0 on a match — both gates had the pass/fail logic backwards. Fixed the exit-code inversion in both.benchbox/core/throughput/runner.py:executor.shutdown(wait=False)leftcancel_futuresat its defaultFalse, so whenmax_workers < num_streams, a still-queued (never dispatched) stream could start executing once a worker freed up — well afterexecute()had already returned and counted it as timed out. Addedcancel_futures=True, which only cancels not-yet-started futures (already-running/leaked ones are untouched, preserving the documented can't-forcibly-cancel-a-running-thread invariant).Testing
Every fix follows the same discipline: read the actual bug, add/extend a regression test, verify it fails without the fix (
git stashthe fix, confirm failure,git stash pop, confirm pass), lint/format, then targeted + broader suite runs.tests/integration/test_joinorder_full_archive.py— 117 passed against the real ~GB canonical archive (locally cached), including a new fixture-level test proving the exception narrowing and reproducing the exact bug (assert cursor.fetchall() == [(7,)]type reproduction of the old buggy~match returning zero rows on "Amélie").tests/unit/test_post_merge_signature.py— 28 passed (25 original + 3 new), including simulating both the missing-artifact and coverage-only-failure scenarios end-to-end.tests/integration/test_tpc_power_tests.py+tests/integration/test_tpcds_power_test.py— 43 passed, including newcaplog-based tests proving zero placeholder warnings fire on the count-only paths.tests/unit/core/throughput/test_runner.py— 25 passed, including a new test withmax_workers=1, num_streams=2proving a queued stream never starts even after its worker frees up (confirmed the old code did start it, ~2s later, without the fix).uv run -- python -m pytest tests/unit/— 24238 passed, 5 failed (all 5 confirmed pre-existing: the sandbox's root-user bypassing filesystem permission checks the tests expect to fail; zero diff overlap with this branch).Public Contract Check
Documentation
docs/development/throughput-result-alignment.mdupdated to describecancel_futures=Truesemantics (fix(throughput): return from the executor without blocking on a hung stream #1141).Code Quality
ruff check/ruff format --checkclean on every changed file.Artifact Hygiene
Notes
No CODEOWNERS-gated paths touched — enabling squash auto-merge.
🤖 Generated with Claude Code
Generated by Claude Code