Sync ruff pin to v0.15.12 (uv.lock + pre-commit) and apply formatter sweep - #2297
Conversation
…r sweep The pre-commit ruff pin (v0.15.0) and the venv-installed ruff (v0.14.14 from uv.lock) had drifted across a major formatter change: ruff 0.15 added support for PEP 758 (Python 3.14's relaxed ``except A, B:`` syntax without parens). The repo moved to Python 3.14 in #2279, so ruff 0.15 correctly drops the now-unnecessary tuple parens — but the older venv ruff (0.14.14) didn't, so ``make lint`` looked clean locally while the pre-commit hook flagged 85+ files. That mismatch silently bit the #2170 PR (and would bite anyone else whose ``.venv`` was provisioned before this fix). Three coupled changes to align the toolchain: - ``pyproject.toml``: tighten the dev ruff specifier from ``>=0.1.0`` to ``>=0.15.12,<0.16`` so the floor matches the pre-commit minor and the ceiling prevents a future 0.16 from drifting again. - ``.pre-commit-config.yaml``: bump the ruff pin from ``v0.15.0`` to ``v0.15.12`` (the latest 0.15.x at time of writing). - ``uv.lock``: regenerated via ``uv lock --upgrade-package ruff``; installs ``ruff==0.15.12`` on the next ``uv sync``. Apply the resulting formatter sweep across 86 files in the same commit (mostly PEP 758 paren drops on ``except (A, B):`` sites). Splitting would have left main red on the very next commit, since pre-commit would then enforce a format the bumped ruff produces.
There was a problem hiding this comment.
Review (post-merge)
The PR is mechanically correct: PEP 758 is a real, accepted Python 3.14 feature; target-version = "py314" + requires-python = ">=3.14" makes the unparenthesized form valid; CI is on 3.14; uv.lock pins mypy==1.19.1, ruff==0.15.12, bandit==1.9.3 — all of which parse the new syntax (verified locally). Bundling the pin bump and the format sweep is the right call — splitting them would leave main red between commits.
A few non-blocking observations the formatter sweep surfaced but didn't address:
1. Pre-existing redundant exception classes in catch lists (non-blocking)
The mechanical paren-drop touched 13 sites where the original except (A, B): listed two exception classes in a subclass relationship — i.e., the more-specific class is dead code in the catch list and the handler is functionally except <base>:. Since the PR rewrote every one of these lines, this was the natural moment to clean them up.
except <X>, Exception: (5 sites) — JSONDecodeError, TypeError, ValueError, etc. are all subclasses of Exception, so the leading specific class is dead:
-
gateway/commit_registry_client.py:86—except json.JSONDecodeError, Exception: -
sandbox/egg_agent_tools/handlers/_gateway.py:128—except json.JSONDecodeError, Exception: -
sandbox/egg_lib/contract_cli.py:306—except json.JSONDecodeError, Exception: -
shared/egg_contracts/checkpoint_cli.py:354—except json.JSONDecodeError, Exception: -
shared/egg_contracts/checkpoint_cli.py:368—except json.JSONDecodeError, Exception:These should be
except Exception:(and ideally narrowed further — catching all ofExceptionto setparsed = Noneafter a failed JSON parse is too broad and will swallow programming errors likeAttributeErroron aNoneexc.read()).
except ImportError, ModuleNotFoundError: (5 sites) — ModuleNotFoundError extends ImportError, so the second class is dead:
-
orchestrator/tests/test_infra_error_escalation.py:673and:678 -
orchestrator/tests/test_overseer_issue_filing_integration.py:47 -
orchestrator/tests/test_two_tier_integration.py:65(×2)These should be
except ImportError:.
except subprocess.TimeoutExpired, subprocess.SubprocessError: (2 sites) — TimeoutExpired extends SubprocessError:
-
shared/egg_contracts/agent_recovery.py:298 -
shared/egg_git/default_branch.py:36Should be
except subprocess.SubprocessError:.
except FileNotFoundError, OSError: (1 site) — FileNotFoundError extends OSError:
-
sandbox/egg_lib/sdlc_hitl.py:324Should be
except OSError:.
Note that ruff's B014 rule flags exact duplicates but does not catch subclass redundancy, which is why the lint stayed clean. Worth a follow-up sweep — doesn't need to block this PR or carry its own.
2. Readability cost of the unparenthesized form
Every except A, B: site now visually mirrors the deprecated Python 2 except A, name: binding syntax (removed in 3.0 by PEP 3110). PEP 758's as-only binding rule prevents semantic ambiguity, but for anyone with Python-2-era muscle memory the eye still has to translate "this is not a name binding" on every read. This is a tooling-imposed cost (ruff's UP rules), not something this PR can opt out of without fighting the formatter — flagging it for visibility, not as a fix request.
If the team finds this confusing in practice, the escape hatch is to drop UP from select in pyproject.toml (or lint.ignore = ["UP024"] for just the parenless except rule — verify the exact code) and reformat back to parens. Cheap to revisit later.
3. Floor pin is good
ruff>=0.15.12,<0.16 matching the pre-commit minor is exactly the right defensive shape — prevents the same drift that bit #2296. The only nit is that the <0.16 ceiling will require a manual bump when 0.16 ships; that's intentional and consistent with how the pre-commit rev: is pinned to a specific tag, so no objection.
4. uv.lock change is clean
Single-package upgrade (ruff only); transitive deps unchanged; sha256 hashes match upstream wheels; dates are within the 0.15.12 release window (2026-04-24). No surprises.
— Authored by egg
|
egg review completed. View run logs |
Post-merge ruff sweep (#2297 bumped ruff to v0.15.12) flagged the quoted forward reference. PEP 649 lazy evaluation lands in py3.14 (the project target), so the runtime quote is no longer required.
…env var Two blocking issues from the review on `a5bdee1` / `d1ba140`: 1. Unit tests in test_global_slice_admit.py were importing the bare `global_slice_admit` module while production uses `orchestrator.global_slice_admit`. These resolve to different module objects with separate `_singleton` instances — so the unit tests passed on internal consistency but never exercised the production singleton. Switch to `from orchestrator import global_slice_admit`. The orchestrator conftest doesn't add the repo root to sys.path before top-level imports run, so the test module adds it explicitly before the import. 2. `_resolve_cap()` swallowed ImportError with a bare `except Exception:` and silently returned 4, ignoring an operator-set `EGG_ORCH_GLOBAL_MAX_PARALLEL_SLICES=8` on the bare-import path. Match the dual-path import pattern used in routes/pipelines.py: `try: from orchestrator import env_config / except ImportError: import env_config`. If neither path resolves we now raise rather than silently return 4 — that's a deployment bug worth surfacing. Also addressed two non-blocking items: - Drop `reset_for_testing` from `__all__` so the package contract is `try_admit` / `release` / `snapshot` only. Tests still reach the function via attribute access. - Drop the unnecessary `_run_concurrent_phase` patch in `test_release_called_on_integration_branch_failure` — the integration-branch-failure path returns before reaching that function, so the patch was dead. Skipped #4 (PEP 758 except rewrites): main has since been ruff-swept to PEP 758 (#2297), so the form in this PR is now consistent with surrounding code. Reverting would create new churn.
* Fix #2241 (gap 1): global slice concurrency cap across pipelines Per-pipeline EGG_ORCH_MAX_PARALLEL_SLICES (default 5) doesn't bound slice spawns across pipelines. Two pipelines kicked off via submit_task can each fan out their wave concurrently and exceed the host's safe budget (~4 slices, each spawning ~8 containers). Adds an orchestrator-process-wide admission counter: - EGG_ORCH_GLOBAL_MAX_PARALLEL_SLICES (default 4) — process-wide cap across ALL running pipelines. - orchestrator/global_slice_admit.py — manual counter+lock with idempotent admit/release keyed on (pipeline_id, slice_id). Idempotent release means duplicate calls in finally blocks are safe; never over-releases into negatives. - Run loop wires try_admit before mark_spawned so the per-pipeline iter_ready accounting stays honest. Rejected slices stay READY and re-yield next tick. Release fires from a finally block in _run_one_slice so every exit path (consensus, phase failure, raised exception, integration-branch failure) frees the slot. - Status endpoint and get_pipeline_snapshot expose the {cap, admitted, admitted_keys} view so operators can see when slices are queued behind the cap rather than wedged. Caveat documented: in-process counter only — HA replicas get one cap per replica. Closes gap 1 of #2241; gap 2 (in-progress / has-open-PR child handling for epic reassess) tracked separately. * Address PR #2288 review feedback: fix singleton-identity + bare-path env var Two blocking issues from the review on `a5bdee1` / `d1ba140`: 1. Unit tests in test_global_slice_admit.py were importing the bare `global_slice_admit` module while production uses `orchestrator.global_slice_admit`. These resolve to different module objects with separate `_singleton` instances — so the unit tests passed on internal consistency but never exercised the production singleton. Switch to `from orchestrator import global_slice_admit`. The orchestrator conftest doesn't add the repo root to sys.path before top-level imports run, so the test module adds it explicitly before the import. 2. `_resolve_cap()` swallowed ImportError with a bare `except Exception:` and silently returned 4, ignoring an operator-set `EGG_ORCH_GLOBAL_MAX_PARALLEL_SLICES=8` on the bare-import path. Match the dual-path import pattern used in routes/pipelines.py: `try: from orchestrator import env_config / except ImportError: import env_config`. If neither path resolves we now raise rather than silently return 4 — that's a deployment bug worth surfacing. Also addressed two non-blocking items: - Drop `reset_for_testing` from `__all__` so the package contract is `try_admit` / `release` / `snapshot` only. Tests still reach the function via attribute access. - Drop the unnecessary `_run_concurrent_phase` patch in `test_release_called_on_integration_branch_failure` — the integration-branch-failure path returns before reaching that function, so the patch was dead. Skipped #4 (PEP 758 except rewrites): main has since been ruff-swept to PEP 758 (#2297), so the form in this PR is now consistent with surrounding code. Reverting would create new churn. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
…t) (#2267) * Fix #2263: per-phase consensus timeout defaults (refine/plan/implement) A single 30-min `consensus_timeout_minutes` was calibrated against refine (smallest fan-out, ~1 pass) and forced implement (5 reviewers, 2-3 NACK iterations common) to either burn the budget or trip the auto-decision / force-kill boundary. This adds three per-phase override fields and a phase-aware fallback chain at the consensus polling read site: 1. `consensus_timeout_minutes_<phase>` if explicitly set, else 2. legacy `consensus_timeout_minutes` if explicitly set (preserves the back-compat clause that pipelines passing only the global behave identically across all three phases), else 3. PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN — refine 30, plan 60, implement 90. The legacy global default flips from `30` to `None` so its "is it set?" state is unambiguous; existing pipelines that explicitly pass a value still see that value applied uniformly. Companion to #2243's progress gate (which defers the decision while progress signals are fresh) and #2245's post-timeout per-iteration clock — different layers, same goal. * Bump file-size allowlist baseline for pipelines.py / peer_consensus.py PR #2250 (file-size lint) merged after #2254 (progress gate) but its allowlist baseline wasn't updated to reflect #2254's growth. Combined with this PR's +1 line in pipelines.py the lint now fails. Update the baselines to the post-merge state (15515 lines / 677159 bytes for pipelines.py; 2003 lines / 85965 bytes for peer_consensus.py). Issue #2248 still tracks the underlying decomposition work. * Fix file-size lint: bump select_tests.py baseline to match main (1850 lines / 73711 bytes) * Address reviewer suggestions on per-phase consensus timeout - Drop stale 'consensus_timeout_minutes: 30' from JSON example in docs/guides/sdlc-pipeline.md. With the new per-phase defaults (refine 30 / plan 60 / implement 90), copy-pasting that value would actively regress plan and implement back to 30. Replaced with prose explaining the unset-default behaviour and a worked example showing per-phase override precedence. - Replace hardcoded '30' fallback in resolve_consensus_timeout_minutes with PHASE_CONSENSUS_TIMEOUT_DEFAULTS_MIN['refine'] so the unknown- phase branch tracks the constant if the floor is ever recalibrated. - Update test_unknown_phase_falls_back_to_30 to reference the constant rather than the magic number 30, matching the production code's source of truth. * Drop forward-ref quotes on PipelineConfig per ruff UP037 Post-merge ruff sweep (#2297 bumped ruff to v0.15.12) flagged the quoted forward reference. PEP 649 lazy evaluation lands in py3.14 (the project target), so the runtime quote is no longer required. * Annotate resolver override to clear mypy no-any-return getattr returns Any, so without a hint mypy flags the return on line 42. Reviewer-suggested non-blocking observation on PR #2267 — orchestrator isn't on the mypy frontier yet, but cheap insurance for when it is. --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Summary
The pre-commit
ruff-formatpin (v0.15.0) and the venv-installed ruff (v0.14.14 fromuv.lock) had drifted across a major formatter change: ruff 0.15 added support for PEP 758 (Python 3.14's relaxedexcept A, B:syntax without parens). The repo moved to Python 3.14 in #2279, so ruff 0.15 correctly drops the now-unnecessary tuple parens — but the older venv ruff (0.14.14) didn't, somake lintlooked clean locally while the pre-commit hook flagged 85+ files. That mismatch silently bit #2296 (and would bite anyone whose.venvwas provisioned before this fix).Changes
pyproject.toml: tighten the devruffspecifier from>=0.1.0to>=0.15.12,<0.16so the floor matches the pre-commit minor and the ceiling prevents a future 0.16 from drifting again..pre-commit-config.yaml: bump the ruff pin fromv0.15.0tov0.15.12(latest 0.15.x).uv.lock: regenerated viauv lock --upgrade-package ruff; installsruff==0.15.12on the nextuv sync.except (A, B):).Why bundle the format sweep
Splitting the pin bump from the format sweep would leave main red on the very next commit — pre-commit would then enforce a format the bumped ruff produces, but the codebase wouldn't yet be in that shape. Bundling them is the cleanest landing.
Test plan
uv sync --extra devinstalls ruff 0.15.12make lint— all checks passed.venv/bin/pytest sandbox/tests/test_egg_orch_overseer_consult_advisor.py shared/tests/test_overseer_advisor.py— 71 passedpre-commit run --all-files— clean (executed via the commit hook)Follow-up
After this lands, all 17 open PRs need to merge main (or rebase) and run
.venv/bin/ruff formatto absorb the same paren drops on any sites their changes touch. Most should be conflict-free.