fix: consolidated bot review follow-up sweep (10 findings across 9 merged PRs)#1116
Merged
Conversation
…image ref field (#1065 review) _inventory_resources/_list_images/_list_containers/_list_volumes ignored the caller's project_prefix and always classified against the hard-coded DEFAULT_UAT_PROJECT_PREFIX, so `--prefix foo` left foo-owned leftovers uncleaned while still targeting default benchbox-uat resources. Thread project_prefix through all four functions. Also: current `container image ls --format json` renders the image reference at the top-level displayReference field, not configuration.name (which is empty on those rows), so owned-mode image reclaim never matched. Read displayReference first, falling back to configuration.name for older schema variants. Also corrects throughput-timeout-leak-and-success-gates.yaml's root-cause diagnosis (#1068 review): StreamRunner.execute's timeout enforcement is dead code, not just unable to cancel a running thread -- as_completed() only ever yields an already-done future, so the subsequent future.result(timeout=...) can never itself raise TimeoutError. w1 is corrected to fix detection (a wait()-with-deadline poll loop) before surfacing leaked streams. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
#1069 review) #1069 removed normalize_tbl_trailing_delimiters() entirely, reasoning all bundled dbgen binaries are now compiled with -DEOL_HANDLING. But TPCCompiler.needs_compilation() only checks whether a binary file already exists -- it has no staleness/CFLAGS check against current source -- so a pre-existing, locally-built _sources/tpc-h/dbgen/dbgen predating that fix (or a source compile on a platform outside the precompiled matrix) is reused as-is and would silently reintroduce the trailing pipe with no safety net. Restored the normalization pass in _finalize_generation. The O(1) tail probe keeps it a no-op for the now-clean bundled binaries (matching the original design rationale), while still normalizing the fallback case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
test-installation's post-publish smoke test installed unqualified
`benchbox`, so PyPI/Test PyPI propagation lag or a pre-existing Test PyPI
version could install a different release than the one this workflow run
just built and published, silently missing a broken current wheel. Add
`build` to test-installation's needs and pin both install steps to
benchbox==${{ needs.build.outputs.version }}.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…eview) benchbox/experimental/__init__.py's subsystem list and the _EXPERIMENTAL_SUBSYSTEMS no-registry-leak guard in test_cli_package_exports.py still said "concurrency" after the concurrency -> load_testing rename, so the guard checked a name that no longer exists (vacuously passing) instead of the real module it's meant to protect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…review) Unconditional import tomllib raises ModuleNotFoundError on Python 3.10 (stdlib tomllib is 3.11+), which the repo still advertises as supported (requires-python >=3.10). Falls back to the already-declared tomli backport, matching the pattern in benchbox/core/data_fetch/manifest.py and friends. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…D needs no test changes (#1079 review) compare_ruleset's enforce_review_rule parameter defaults to the module constant (enforce_review_rule: bool = DEVELOP_REVIEW_RULE_ENFORCED), so flipping the constant also flips that default. test_missing_review_enforcement_is_a_non_blocking_warning_by_default calls compare_ruleset() without passing enforce_review_rule explicitly, relying on the current False default to exercise the non-blocking path -- after the flip it would exercise the blocking path instead and fail its own blocking_findings(findings) == [] assertion. Corrected the runbook to say the maintainer must update that test alongside the flip, rather than claiming no test changes are required. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…andoff w2 (#1080 review) w2 had two notes: keys back to back. YAML loaders either silently drop one value or, in stricter loaders such as the ruamel-based structured TODO editor used by todo_cli.py done, reject duplicate mapping keys, so future updates to this item could fail or lose the original implementation note. Merged the completion text into the existing notes block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…test (#1074 review) test_no_cross_stream_result_bleed only checked len(query_ids) == 103 per stream. A stream that duplicates one query while omitting another, or collapses a variant pair like 14a/14b into the same id, keeps the same length but changes which ids are present -- the length check alone can't catch it. Compare against the exact expected query_id Counter (derived independently from query range 1-99 and MULTI_PART_QUERY_IDS, matching how throughput_test.py builds query_display_id). Verified against a real DuckDB run (SF 0.01) that the new assertion passes on genuine production output, and synthetically that a variant-collapse corruption keeping the same length is caught by the Counter comparison while the old length-only check would have missed it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…g to 2 (#1077 review) _resolve_requested_stream_count() (benchbox/platforms/base/execution.py) floors any resolved stream count to the TPC throughput minimum of 2, because it has no wire-level way to distinguish an explicit user request of 1 from the schema's own default of 1 at that deep boundary -- that trade-off is intentional and documented there. But run-official --streams 1 is one of the two reachable paths (the other being the interactive configurator) where the request IS still known to be explicit at the point it's captured. Reject it there instead of letting it silently run a different stream count than what the user typed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
…efer extension probe (#1082 review) Three findings from the late-landing bot review on merged PR #1082: 1. from_config() read metadata_path/data_path via flat config.get(...) only. The real `benchbox run --platform ducklake --platform-option metadata_path=... --platform-option data_path=...` path nests those values under config["options"] (DuckLake has no registered PlatformHookRegistry config builder to promote them to top-level flat keys - the same reason _resolve_option() already exists in this function for catalog/pg_*/s3_* options). User-specified catalog/data locations were silently ignored in favor of generated defaults. 2. force_recreate was read from a flat "force" key, but the real --force CLI flag threads through as force_recreate nested in config["options"] via the same default config builder. A real --force request was silently reset to False, so an existing catalog was reused instead of wiped for a clean rebuild. Now checks both the legacy flat "force" key (config_utils.py's build_config() path) and force_recreate (flat or nested). 3. tests/integration/test_ducklake_integration.py probed DuckDB extension availability (INSTALL ducklake / INSTALL sqlite) at module level, so `@pytest.mark.skipif` class decorators triggered a real, potentially networked INSTALL during test COLLECTION - which pytest always does regardless of which markers later deselect the tests from execution. Moved the probe into setup_method (only reached by a test pytest has already decided to run), so a fast/offline collection never pays for it. 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: fab0e4e5b7
ℹ️ 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".
…1116 review) test-installation's windows-latest leg used the default PowerShell shell for the two install steps, so `${BENCHBOX_VERSION}` (bash env syntax) resolved to empty and `pip install "benchbox=="` would have run instead of installing the pinned version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6
joeharris76
enabled auto-merge (squash)
July 17, 2026 02:37
Resolves 4 conflicts from this stale review-followup-sweep branch: - throughput-timeout-leak-and-success-gates.yaml: our planning-doc edit is superseded by #1106, which already completed and archived this TODO to DONE/ with the same root-cause finding recorded; took develop's deletion. - benchbox/platforms/ducklake.py: composed our force_recreate resolution fix with develop's independently-added tuning-provenance passthrough keys. - tests/uat/container_cleanup.py: kept develop's added network inventory call (unrelated addition, no conflict of intent). - tests/uat/test_container_cleanup.py: kept both this branch's 2 new regression tests (#1065 prefix-classification, displayReference field) and develop's 7 new w4/w5/w6 network-routing tests; no overlapping coverage between the two sets. Catches this branch up with develop so CI can run again (was mergeable_state=dirty with zero CI runs since 2026-07-10).
# Conflicts: # docs/operations/repo-admin-settings.md
…t-lane bump 25190->25220 The #1079 finding's runbook correction is superseded by the 2026-07-18 require_code_owner_review retirement: the DEVELOP_REVIEW_RULE_ENFORCED constant, the flip-order paragraph it corrected, and the test it named were all removed on develop, so the hunk is dropped as moot. Fast-lane ceiling bumped once for the whole open-PR merge queue with byte-identical content across queued branches; this merge ref collects 25191. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018y41mPzRqiLqvZLZCxwS6j
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, batch-retriggered after the account's usage limit reset. Each PR listed below was already merged before its review completed (or the review only landed after merge), so all fixes are consolidated into this one sweep branch offdevelopper the established review-followup pattern..github/workflows/release.yml(CODEOWNERS-gated) — auto-merge intentionally NOT enabled. This PR needs your review before merge.Findings fixed
tests/uat/container_cleanup.py:project_prefixwasn't threaded through inventory (custom--prefixleftovers weren't cleaned); image reference read from the wrong JSON field (configuration.namevs. currentcontainer image ls's top-leveldisplayReference).throughput-timeout-leak-and-success-gates.yaml: corrected the root-cause diagnosis —StreamRunner.execute's timeout enforcement is dead code (as_completed()only yields already-done futures, so the subsequentfuture.result(timeout=...)can never itself raiseTimeoutError), not just "can't cancel a running thread." w1 now targets fixing detection first.benchbox/core/tpch/generator.py: restorednormalize_tbl_trailing_delimiters()as a safety net.TPCCompiler.needs_compilation()has no staleness check, so a pre-existing local dbgen build predating-DEOL_HANDLINGis never recompiled and would silently reintroduce trailing-pipe rows with no normalization pass..github/workflows/release.yml: the post-publish smoke test installed unqualifiedbenchboxinstead ofbenchbox==${{ needs.build.outputs.version }}, so PyPI propagation lag could mask a broken current wheel.benchbox/experimental/__init__.py+test_cli_package_exports.py: theconcurrency→load_testingrename left the no-registry-leak guard checking a name that no longer exists.tests/integration/test_throughput_real_duckdb.py:test_no_cross_stream_result_bleedonly checked row count (103); now compares the exact expected query/variant multiset, catching a variant-collapse (14a/14b) bug the count alone would miss._project/scripts/build_joinorder_data.py: added the Python 3.10tomlifallback (stdlibtomllibis 3.11+, but the repo advertises>=3.10).benchbox/cli/commands/run_official.py:run-official --streams 1is now rejected outright instead of being silently floored to 2 by_resolve_requested_stream_count()'s documented (and still-intentional) floor.docs/operations/repo-admin-settings.md: corrected the runbook's claim that flippingDEVELOP_REVIEW_RULE_ENFORCEDneeds no test changes — it does (the default-valued test parameter is bound to that constant).explorer-pipeline-frontend-handoff.yaml: merged a duplicatenotes:key on w2 that a stricter YAML loader could reject or silently drop.benchbox/platforms/ducklake.py(3 findings):metadata_path/data_pathandforce_recreateweren't resolved from the nestedconfig["options"]shape the real CLI path uses; the live-integration test file's extension-availability probe ran at import time (during collection, before marker filtering applies), spending a real networkINSTALLeven in offline/fast lanes.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/typecheck, then targeted + broader suite runs. See individual commit messages for per-fix detail.uv run -- python -m pytest tests/unit/(24191 passed, 6 failed — all 6 confirmed pre-existing/environment-specific: 5 are the sandbox's root-user bypassing filesystem permission checks the tests expect to fail, matching known pre-existing failures; 1 is test-isolation flakiness in a file untouched by this branch, passing cleanly standalone). Zero diff overlap between this branch and any of the 6 failing files.tests/integration/test_throughput_stream_count.py,tests/uat/test_container_cleanup.py,tests/unit/platforms/test_ducklake_adapter.py,tests/unit/core/tpch/,tests/unit/test_release_infrastructure.py,tests/unit/scripts/test_build_joinorder_data.pyall pass in full.Public Contract Check
Documentation
docs/operations/repo-admin-settings.mdcorrected (docs(runbook): make the review-enforcement flag flip's ordering explicit #1079).Code Quality
ruff check/ruff format --check/ty checkclean on every changed file.Artifact Hygiene
Notes
Closes out 11 outstanding P2/P3 bot review threads across #1065, #1068 (partial — 2 of 3 sub-findings were already adequately resolved by concurrent work, verified and will be replied+resolved separately), #1069, #1072, #1073, #1074, #1076, #1077, #1079, #1080, #1082.
🤖 Generated with Claude Code
Generated by Claude Code