From 2ebbdfdd878bb7f45e60f1686ba37e13822427b9 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:44:01 +0200 Subject: [PATCH 1/2] `git-push`'s 300s budget was a module constant, so a repo whose own pre-push hook runs its suite timed out on every master push (#1631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_PUSH_TIMEOUT = 300` was reachable only from the per-call `:budget=SECONDS` flag, and its own comment argued against itself: what decides the right number "is not visible from here". It is visible from the project's config — whether a pre-push hook runs a suite is a property of the repo. Measured: 12868 passed in 309.86s against the 300s default, so two pushes failed at 302.70s and 302.90s with nothing sent before a third at :budget=1500 landed. Adds `ops.git-push.budget`, merged over the preset entry key-by-key like every other per-op key. Precedence is flag > config > 300, and 300 is still the answer when neither is set. Refused, never clamped: validated against ops.git-push.timeout from the SAME merged entry — both keys from one read, because core exports `budget` to the subprocess but reserves `timeout`, and checking one against the other would be two answers to one question. A value that is not a whole positive number of seconds, is above 1800, is at or above the op timeout, or whose timeout could not be read at all, refuses the push before anything is sent and names both numbers (#399, #1615). Ten seconds of margin is also the argument against a bigger constant: it would move this repo from always-fails to sometimes-fails, and a repo with no hook wants a shorter budget for the same reason. Co-Authored-By: Max --- .supertool.example.json | 4 + .supertool.json | 3 + changelog.d/1631.added.md | 7 + docs/presets/git.md | 26 +- presets/git.json | 2 +- presets/git/push.py | 176 +++++++- .../test_git_push_budget_from_config_1631.py | 381 ++++++++++++++++++ tests/test_op_registry_1356.py | 15 +- 8 files changed, 605 insertions(+), 9 deletions(-) create mode 100644 changelog.d/1631.added.md create mode 100644 tests/test_git_push_budget_from_config_1631.py diff --git a/.supertool.example.json b/.supertool.example.json index 76810709..81a09583 100644 --- a/.supertool.example.json +++ b/.supertool.example.json @@ -340,6 +340,10 @@ } } }, + "git-push": { + "budget": 1500, + "_doc": "The DEFAULT push budget for this repository, in seconds — what `:budget=SECONDS` sets per call, stated once. Reach for it when a pre-push hook runs a test suite: this repo's runs the full suite on a push to master (309.86s measured), against a 300s default, so every such push timed out having sent nothing (#1631). Precedence is :budget=SECONDS > ops.git-push.budget > 300. It must be a whole positive number of seconds, at most 1800, and STRICTLY under ops.git-push.timeout on the same merged entry — a value that is not is REFUSED naming both numbers, before anything is pushed, never clamped: past the op timeout supertool kills the process and a killed push can verify nothing (#399)." + }, "git-commit": { "coauthor": "Max ", "_doc": "Overrides the git preset's git-commit op. 'coauthor' passes through as SUPERTOOL_COAUTHOR — the Co-Authored-By trailer auto-appended when the commit message lacks one (default 'Max '). Set to '' / 'none' / 'off' / 'false' to disable." diff --git a/.supertool.json b/.supertool.json index 7a55efec..43a25a0d 100644 --- a/.supertool.json +++ b/.supertool.json @@ -266,6 +266,9 @@ } }, "ops": { + "git-push": { + "budget": 1500 + }, "dashboard": { "lane_prefix": "lane-" }, diff --git a/changelog.d/1631.added.md b/changelog.d/1631.added.md new file mode 100644 index 00000000..b3fa9cfc --- /dev/null +++ b/changelog.d/1631.added.md @@ -0,0 +1,7 @@ +- **`ops.git-push.budget` — a repository can now state its own push budget, instead of every push to `master` timing out to learn the same fact** ([#1631](https://github.com/Digital-Process-Tools/claude-supertool/issues/1631)). `_PUSH_TIMEOUT = 300` was a module constant reachable from nothing but the per-call `:budget=SECONDS` flag, and its own comment made the case against itself: what decides the right number "is not visible from here". It is visible from the project's config, because *this repo's pre-push hook runs a suite* is a property of the repo. Measured while filing: two pushes of one markdown commit failed at 302.70s and 302.90s with nothing sent, and the third at `:budget=1500` printed `12868 passed, 51 skipped in 309.86s` and landed. Ten seconds over the default — so the failure is total rather than occasional, and a merely bigger constant would have produced the worse regime where the same command sometimes works. A repo with no pre-push hook wants a *shorter* budget for the opposite reason. Per-repo in both directions is what makes it configuration. + + Precedence is **`:budget=SECONDS` > `ops.git-push.budget` > 300**, and 300 is still the answer when neither is set. The key merges over the preset entry key-by-key like every other per-op key, so `{"git-push": {"budget": 1500}}` keeps the op's `cmd` and `timeout`; the receipt names the source of the number actually in force, and a budget the flag overrode is not consulted at all. + + **Refused, never clamped.** The configured budget is validated against `ops.git-push.timeout` **from the same merged entry** and refused when it is not strictly under it, naming both numbers — matching what `_parse_budget` already does for the flag. Past the op-level cap supertool kills the process, and a killed push can verify nothing, so the caller acts on a bare `FAIL (timeout)` for a push that landed ([#399](https://github.com/Digital-Process-Tools/claude-supertool/issues/399)); on the recovery path `_report_recovery_timeout` is the only thing that would have said the worktree is paused mid-rebase ([#1615](https://github.com/Digital-Process-Tools/claude-supertool/issues/1615)). A clamp would convert *the caller asked for a number and got a different one* into a discovery made at the moment a push cannot be verified. + + This value is not an op argument — it arrives from a config file and ends up as `timeout=` on a `subprocess` call, so nothing in `_safe_path` or the op's `paths` declaration stands in front of it. Every shape is checked before anything is pushed: a JSON number, whole (`bool` is an `int` in Python and is refused), positive, at most 1800, and strictly under the op timeout. A `timeout` that itself does not read as a positive whole number is a third state — the budget is refused rather than assumed safe, because a check that could not run must not return the shape of a clean result. Both keys come from one read of the merged entry rather than one from the environment and one from disk: core exports `budget` to the subprocess as `SUPERTOOL_BUDGET` for free, but `timeout` is reserved and deliberately does not, and validating two answers to the same question against each other is not a check. diff --git a/docs/presets/git.md b/docs/presets/git.md index b8a0c57f..3eb05164 100644 --- a/docs/presets/git.md +++ b/docs/presets/git.md @@ -695,13 +695,37 @@ Three calls name their own budget instead, because they are the ones that legiti | call | budget | why | |---|---|---| -| `git push` (`git-push`) | 300s, or `:budget=SECONDS` up to 1800 | The op owns its own timeout so it can verify the remote before reporting; supertool's outer cap must not fire first | +| `git push` (`git-push`) | 300s, or `ops.git-push.budget`, or `:budget=SECONDS` — up to 1800 | The op owns its own timeout so it can verify the remote before reporting; supertool's outer cap must not fire first | | `git fetch` / `git rebase` on `git-push`'s recovery path | 120s, or what is left of the push budget | Can land on a worktree git has already paused ([#640](https://github.com/Digital-Process-Tools/claude-supertool/issues/640)) | | `git commit` (`git-commit`) | 30s | Runs whatever the pre-commit hook chain is | | `git merge` (`git-merge`) | 30s | Runs merge drivers, potentially over the whole tree | **An explicit budget wins; the environment sets the default** ([#704](https://github.com/Digital-Process-Tools/claude-supertool/issues/704)). Setting `SUPERTOOL_GIT_TIMEOUT=5` to tighten `git-status` does not cap `git-push`'s 300s and report a push still in flight as failed. +#### `ops.git-push.budget` — the default your repository chooses + +`:budget=SECONDS` is per-call, and there are repositories where it is the right answer on **every** call: a pre-push hook that runs the suite on a push to `master` cannot finish inside 300s, so the flag has to be retyped every session or the push times out having sent nothing. Set the default once instead ([#1631](https://github.com/Digital-Process-Tools/claude-supertool/issues/1631)): + +```json +{ + "ops": { + "git-push": { "budget": 1500 } + } +} +``` + +Precedence is **`:budget=SECONDS` > `ops.git-push.budget` > 300**, and 300 is still the answer when neither is set. The key merges over the shipped preset entry key-by-key, so writing `budget` alone keeps the op's `cmd`, `timeout` and everything else; `registry:git-push` renders the merged result with the source of each key. + +**It is refused, never clamped, and never silently ignored.** The budget has to stay *strictly* under `ops.git-push.timeout` from the same merged entry — past that cap supertool kills the process, and a killed push cannot ask the remote what landed, which is the verdict this op exists to produce ([#399](https://github.com/Digital-Process-Tools/claude-supertool/issues/399)). A configured value that is not a whole positive number of seconds, is above 1800, is at or above the op timeout, or that could not be checked against the op timeout at all, refuses the push before anything is sent and names both numbers. A push that never happened is recoverable by fixing one line of config; a push under a clock nobody chose is discovered when it cannot be verified. + +A budget the flag overrode is not consulted, so a broken key cannot refuse a push whose clock it does not set. When the config value is the one in force, the receipt says so by name: + +``` +Push budget: 1500s (ops.git-push.budget — default is 300s) +``` + +**Why this is not just a bigger default.** The suite behind this repository's own pre-push hook takes 309.86s against the 300s default — ten seconds, which is well inside normal variance. A raised constant would move a repo like this from *always fails* to *sometimes fails*, from the same command; and a repo with no pre-push hook wants a **shorter** budget, because there the only thing a long one buys is a longer wait before an honest failure. The number is per-repo in both directions. + **`git-push`'s budget is a deadline on its pushing, not a per-call timeout** ([#1615](https://github.com/Digital-Process-Tools/claude-supertool/issues/1615)). `:budget=N` means *this op stops pushing within N seconds of starting*, and the clock covers the initial push, the recovery fetch, the rebase and the re-push between them. It used to mean *each `git push` gets N*, which on the non-fast-forward path spent `2N + 240` — so `:budget=1800` asked for 3840s inside an op capped at 1920, and past that cap supertool kills the process, on the one path where the receipt is the only thing that would say the worktree is paused mid-rebase. The clock opens at the first `git push`, so a run with one push is unchanged. What it costs is on the recovery: a first push that spends most of `N` and is *then* rejected non-fast-forward leaves little or nothing for the rest, and the rest is **declined rather than run short** — `NOT PUSHED - BUDGET SPENT`, naming whether the rebase had already replayed your branch. A `git push` launched on an expired clock is killed before it can verify anything, and on this op the verdict is the whole product. Raise `:budget` and retry; the branch is already rebased, so the retry is a fast-forward. The preamble that picks a remote and the receipt that reads the result stay outside the clock — the receipt deliberately, because an expiring clock past the point of no return must never cost you the answer ([#675](https://github.com/Digital-Process-Tools/claude-supertool/issues/675)). diff --git a/presets/git.json b/presets/git.json index f4b4285d..5efd4e36 100644 --- a/presets/git.json +++ b/presets/git.json @@ -99,7 +99,7 @@ "safety": "acts", "cmd": "{python} {path}git/push.py {args}", "timeout": 1920, - "description": "Push current branch (sets upstream if missing). ALWAYS ends on a one-line `[result]` verdict — PUSHED / NOT PUSHED (already up to date | REJECTED | REBASE PAUSED | UNVERIFIED | no push attempted) with branch → remote/ref @ sha — so the answer survives `| tail -3`. The post-push sha is read back off the real remote via ls-remote and labelled verified/unverified; a sha that was not read is never printed as if it were. Receipt: Repo (which LOCAL repository these commits came from), remote before/after, ahead/behind, MR/PR + pipeline, mergeability, behind-target, uncommitted-leftover COUNT (list: git-status:full), watch cmd. Non-ff auto-rebases (conflict → paused + git-conflicts). Hook amend+push reported as PUSHED. A push that outlasts its budget is verified against the remote ref before any verdict — landed = PUSHED. A fetch/rebase on the non-fast-forward recovery path that outlasts its own budget reports the WORKTREE state — rebase in progress (with continue/abort), not started, or explicitly unknown — instead of a traceback. The stale-base check follows the branch's real upstream remote (not a hardcoded origin) and says `skipped` when the target ref does not resolve, so silence means only `checked, base is fresh`. On a branch with NO upstream the push remote is RESOLVED, not assumed: branch..pushRemote, remote.pushDefault, branch..remote (git's own order), then `origin` if it exists, then the only remote if there is exactly one — so `git clone -o gitlab` and fork/upstream layouts work. Two or more remotes with none named origin and nothing configured is REFUSED (exit 1, nothing pushed) naming the candidates, because creating a branch on a guessed remote is not recoverable by an error message. A resolved remote or ref that begins with `-` is REFUSED by name before any argv is built: those keys are read verbatim (git accepts a URL in them) and `git push -u --receive-pack= HEAD` runs — git eats the option and spawns receive-pack for the local path `HEAD` before failing to find a repository there (observed, git 2.46.2; #818, #1617). Flags: :force-with-lease, :no-verify, :budget=SECONDS (how long this op may spend PUSHING, in place of the 300s default; a DEADLINE for the whole pushing phase, not a per-call timeout — the non-fast-forward recovery's fetch, rebase and re-push all draw from what is left of it, and a phase with nothing left is declined as `NOT PUSHED - BUDGET SPENT` rather than launched on an expired clock (#1615) — the flag to reach for when a pre-push hook runs a test suite, which is where :no-verify is least appropriate; capped at 1800s, and an unreadable, non-positive, contradicted or over-cap value is REFUSED before anything is pushed rather than clamped), :watch (spawns a background pipeline poller; falls back to the running interpreter + supertool.py where the ./supertool wrapper is absent, e.g. a git worktree, and names the reason if it cannot start). An UNKNOWN flag is REFUSED before anything is pushed (exit 2) — never silently dropped. An upstream that resolves to a DIFFERENT branch — the default outcome of `git worktree add -b `, so every st-wt/NNN branch starts here — is still refused rather than guessed through, but both ways out are now flags on this op instead of raw `git push` lines the caller's own hook may forbid: `:set-upstream` pushes the branch under its own name and retargets tracking to / (the usual first push), `:to-upstream` pushes onto the tracked ref on purpose with an explicit refspec. Asking for both is REFUSED (exit 2) naming the two targets — they are different refs and precedence would be the guess the refusal exists to prevent.", + "description": "Push current branch (sets upstream if missing). ALWAYS ends on a one-line `[result]` verdict — PUSHED / NOT PUSHED (already up to date | REJECTED | REBASE PAUSED | UNVERIFIED | no push attempted) with branch → remote/ref @ sha — so the answer survives `| tail -3`. The post-push sha is read back off the real remote via ls-remote and labelled verified/unverified; a sha that was not read is never printed as if it were. Receipt: Repo (which LOCAL repository these commits came from), remote before/after, ahead/behind, MR/PR + pipeline, mergeability, behind-target, uncommitted-leftover COUNT (list: git-status:full), watch cmd. Non-ff auto-rebases (conflict → paused + git-conflicts). Hook amend+push reported as PUSHED. A push that outlasts its budget is verified against the remote ref before any verdict — landed = PUSHED. A fetch/rebase on the non-fast-forward recovery path that outlasts its own budget reports the WORKTREE state — rebase in progress (with continue/abort), not started, or explicitly unknown — instead of a traceback. The stale-base check follows the branch's real upstream remote (not a hardcoded origin) and says `skipped` when the target ref does not resolve, so silence means only `checked, base is fresh`. On a branch with NO upstream the push remote is RESOLVED, not assumed: branch..pushRemote, remote.pushDefault, branch..remote (git's own order), then `origin` if it exists, then the only remote if there is exactly one — so `git clone -o gitlab` and fork/upstream layouts work. Two or more remotes with none named origin and nothing configured is REFUSED (exit 1, nothing pushed) naming the candidates, because creating a branch on a guessed remote is not recoverable by an error message. A resolved remote or ref that begins with `-` is REFUSED by name before any argv is built: those keys are read verbatim (git accepts a URL in them) and `git push -u --receive-pack= HEAD` runs — git eats the option and spawns receive-pack for the local path `HEAD` before failing to find a repository there (observed, git 2.46.2; #818, #1617). Flags: :force-with-lease, :no-verify, :budget=SECONDS (how long this op may spend PUSHING, in place of the 300s default; a DEADLINE for the whole pushing phase, not a per-call timeout — the non-fast-forward recovery's fetch, rebase and re-push all draw from what is left of it, and a phase with nothing left is declined as `NOT PUSHED - BUDGET SPENT` rather than launched on an expired clock (#1615) — the flag to reach for when a pre-push hook runs a test suite, which is where :no-verify is least appropriate; capped at 1800s, and an unreadable, non-positive, contradicted or over-cap value is REFUSED before anything is pushed rather than clamped; the DEFAULT itself is settable per repository as ops.git-push.budget in .supertool.json — precedence :budget > ops.git-push.budget > 300 — validated against ops.git-push.timeout FROM THE SAME MERGED ENTRY and refused naming both numbers, before anything is pushed, when it is not strictly under it (#1631)), :watch (spawns a background pipeline poller; falls back to the running interpreter + supertool.py where the ./supertool wrapper is absent, e.g. a git worktree, and names the reason if it cannot start). An UNKNOWN flag is REFUSED before anything is pushed (exit 2) — never silently dropped. An upstream that resolves to a DIFFERENT branch — the default outcome of `git worktree add -b `, so every st-wt/NNN branch starts here — is still refused rather than guessed through, but both ways out are now flags on this op instead of raw `git push` lines the caller's own hook may forbid: `:set-upstream` pushes the branch under its own name and retargets tracking to / (the usual first push), `:to-upstream` pushes onto the tracked ref on purpose with an explicit refspec. Asking for both is REFUSED (exit 2) naming the two targets — they are different refs and precedence would be the guess the refusal exists to prevent.", "syntax": "git-push[:force-with-lease][:no-verify][:watch][:budget=SECONDS][:set-upstream|:to-upstream]", "replaces": [ { "argv": "git push", "unless_flag": ["--tags", "--follow-tags", "--delete", "-d", "--mirror", "--all", "--prune", "--force", "-f", "--dry-run", "-n"], "use": "git-push" }, diff --git a/presets/git/push.py b/presets/git/push.py index 06a132d3..70dd2f5d 100644 --- a/presets/git/push.py +++ b/presets/git/push.py @@ -31,6 +31,11 @@ push to a protected branch. Refused rather than clamped if it is unreadable, non-positive, contradicted by a second `budget=`, or above `_PUSH_TIMEOUT_MAX`. + The *default* it replaces is itself settable per repository, as + `ops.git-push.budget` in `.supertool.json` (#1631) — precedence is + flag > config > 300 — because whether a pre-push hook runs a suite is a + property of the repo, and a repo without one wants a shorter budget for + the same reason. See `_config_budget`, which refuses on the same terms. It is a **deadline, not a per-call timeout** (#1615): the clock opens at the first `git push` and the non-fast-forward recovery's fetch, rebase and re-push all draw from what is left of it. See `_open_push_deadline` @@ -64,6 +69,7 @@ """ from __future__ import annotations +import json import os import re import subprocess @@ -127,6 +133,13 @@ # finish inside the budget, ever, and the only flag that helped was # `:no-verify` — which skips the gate the hook exists to be. # +# Since #1631 the number is also a *repository's*, through `ops.git-push.budget` +# in its own `.supertool.json` — see `_config_budget`. That is not a retreat +# from the paragraph above: what a repo knows is whether its own pre-push hook +# runs a suite, which is a fact about the repo and not an inference the op is +# making. What the op still refuses to do is guess. This constant remains the +# answer when nobody has stated one. +# # The number stays a caller's, not the op's. `_prepush_hook_state` can see that # a hook would run and this op knows the destination ref, so it could size # itself from "protected branch + a hook exists" — but it cannot see what any @@ -451,6 +464,144 @@ def _parse_budget(argv: list[str]) -> tuple[Optional[int], str]: return seen[0][1], "" +# The op entry this run was dispatched from, read back off disk (#1631). +# +# `_PUSH_TIMEOUT`'s own comment says what decides the right number "is not +# visible from here", and it is right about the caller's machine. What it misses +# is that a REPOSITORY knows something the tool cannot: whether its own pre-push +# hook runs a suite, and roughly how long that takes. Here it is 309.86s against +# a 300s default, so every master push times out on a healthy machine with a +# green suite. Ten seconds is well inside variance, which is also why raising +# the constant is the wrong fix — it would move this repo from "always fails" to +# "sometimes fails" — and a repo with no pre-push hook wants a SHORTER budget, +# because there a long one only delays an honest failure. Per-repo in both +# directions is what makes it configuration rather than a better constant. +# +# Two keys are needed and they must come from ONE read. Core exports every +# non-reserved op key to the subprocess as `SUPERTOOL_`, so `budget` would +# arrive in the environment for free — but `timeout` is in core's +# `_RESERVED_KEYS` and deliberately does not (tests/test_custom_ops.py pins its +# absence). A budget out of the environment validated against a timeout off disk +# is two answers to one question checked against each other, so both come from +# the same merged entry instead. +# +# `git.json` is located from this file rather than searched for: core resolved +# the preset through `_find_preset_file` and substituted its directory into +# `{path}`, so the script now running came out of the directory the winning +# `git.json` sits in — whichever of the three candidates that was. +_OP_NAME = "git-push" +_CONFIG_BUDGET_KEY = "budget" +_PRESET_JSON = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "git.json") + +#: "this file did not answer", which is not "this file answered with nothing". +_UNREADABLE = object() + + +def _read_json(path: str) -> object: + """Parsed JSON from `path`, or `_UNREADABLE`. + + The distinction is load-bearing one caller down. A file that is absent, + unreadable or not JSON never answered; a file holding `[]` answered with a + document that carries no ops. `_load_config` stops walking at the second and + keeps walking past the first, and collapsing them here would resolve a + budget out of a parent config core never read. + """ + try: + with open(path, encoding="utf-8") as fh: + return json.load(fh) + except (OSError, ValueError): + return _UNREADABLE + + +def _ops_entry(data: object) -> dict: + """`ops["git-push"]` out of one parsed config document, or `{}`.""" + if not isinstance(data, dict): + return {} + ops = data.get("ops") + if not isinstance(ops, dict): + return {} + entry = ops.get(_OP_NAME) + return dict(entry) if isinstance(entry, dict) else {} + + +def _merged_op_entry() -> dict: + """`ops.git-push` as core merged it — the preset entry, then the project's. + + Mirrors `_merge_presets` / `_merge_op_def` for this one op: dict over dict + merges key-by-key, so a project supplying `budget` alone keeps the preset's + `timeout` rather than replacing the entry with a stub (#1356). The walk + stops at the first `.supertool.json` that PARSES, which is what + `_load_config` does; one that does not parse is skipped and the walk goes on. + """ + entry = _ops_entry(_read_json(_PRESET_JSON)) + directory = os.path.abspath(os.getcwd()) + while True: + candidate = os.path.join(directory, ".supertool.json") + if os.path.isfile(candidate): + data = _read_json(candidate) + if data is not _UNREADABLE: + entry.update(_ops_entry(data)) + return entry + parent = os.path.dirname(directory) + if parent == directory: + return entry + directory = parent + + +def _config_budget() -> tuple[Optional[int], str]: + """`ops.git-push.budget` from the merged entry. `(seconds, refusal)`. + + The same three states as `_parse_budget`, and the same refusal discipline: + absent, a number, or unusable and named. **Never clamped** — the caller + wrote a number in a file, and a different one quietly taking effect is + discovered at the moment a push cannot be verified. + + This value is not an op argument. It comes out of a project config file, so + nothing in `_safe_path` and nothing in the op's `paths` declaration stands in + front of it, and it ends up as `timeout=` on a `subprocess` call. Every shape + it could arrive in is therefore checked here: present, a JSON number, a whole + one (`bool` is an `int` in Python and is not one), positive, at most + `_PUSH_TIMEOUT_MAX`, and strictly under this op's own `timeout` — read from + the same merged entry, because a ceiling from a different read is not the + ceiling that will kill this process. + """ + entry = _merged_op_entry() + raw = entry.get(_CONFIG_BUDGET_KEY) + if raw is None: + return None, "" + key = f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}" + cap_key = f"ops.{_OP_NAME}.timeout" + if isinstance(raw, bool) or not isinstance(raw, int): + return None, (f"{key} is {_untrusted.flat(repr(raw))} — the budget must " + f"be a whole number of seconds, written as a JSON number.") + if raw <= 0: + return None, (f"{key} is {raw} — the budget must be a positive number " + f"of seconds.") + if raw > _PUSH_TIMEOUT_MAX: + return None, (f"{key} is {raw}s — the most this op can wait is " + f"{_PUSH_TIMEOUT_MAX}s. It is not clamped to that: past " + f"{cap_key} supertool kills this process, and a killed " + f"push can verify nothing (#399). Raise both, or lower " + f"this one.") + cap = entry.get("timeout") + if isinstance(cap, bool) or not isinstance(cap, int) or cap <= 0: + return None, (f"{key} is {raw}s, but {cap_key} did not read as a " + f"positive whole number of seconds " + f"({_untrusted.flat(repr(cap))}), so the budget could not " + f"be checked against it. Refused rather than assumed " + f"safe: a budget that is not strictly under the op " + f"timeout is killed by supertool's outer cap, and a " + f"killed push can verify nothing (#399).") + if raw >= cap: + return None, (f"{key} is {raw}s and {cap_key} is {cap}s — the budget " + f"must be strictly UNDER the op timeout, because past " + f"that cap supertool kills this process and a killed push " + f"can verify nothing (#399). Raise {cap_key} above " + f"{raw}s, or lower {key} below {cap}s.") + return raw, "" + + def _st_hint(arg: str) -> str: """A runnable `supertool` invocation for `arg`. For printed remedies. @@ -1793,7 +1944,11 @@ def _budget_advice() -> str: f"NOT ops.git-push.timeout: that op-level cap bounds the whole " f"process, raising it alone will not move this one, and this budget " f"has to stay strictly under it or a push killed by the outer cap can " - f"verify nothing (#399).") + f"verify nothing (#399). A repository whose own pre-push hook runs a " + f"suite can state the number once instead of retyping it every " + f"session: `\"git-push\": {{\"budget\": SECONDS}}` under `ops` in " + f".supertool.json (ops.git-push.budget), which the flag still " + f"overrides.") def _report_push_timeout(branch: str, head_before: str, @@ -2314,6 +2469,23 @@ def _push_op() -> int: _result(f"NOT PUSHED - no push attempted (unusable :budget — " f"{budget_why})") return 2 + budget_source = ":budget" + if budget is None: + # Precedence is flag > config > `_PUSH_TIMEOUT` (#1631). The config is + # not consulted at all when the flag decided — that is what precedence + # means, and it keeps a broken key from refusing a push whose clock it + # does not set. + budget, config_why = _config_budget() + if config_why: + print(f"ERROR: unusable push budget in .supertool.json — " + f"{config_why}") + print(f"Default is {_PUSH_TIMEOUT}s; the most this op can wait is " + f"{_PUSH_TIMEOUT_MAX}s. Nothing was pushed.") + _result(f"NOT PUSHED - no push attempted (unusable " + f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY} — {config_why})") + return 2 + if budget is not None: + budget_source = f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}" _BUDGET["seconds"] = budget upstream, upstream_why = _upstream_ref() @@ -2423,7 +2595,7 @@ def _push_op() -> int: # was dropped read identically from the receipt (#647). The default is # not printed: it is documented, and every receipt carrying a line # about a number nobody chose is a line nobody reads. - print(f"Push budget: {_push_budget()}s (:budget — default is " + print(f"Push budget: {_push_budget()}s ({budget_source} — default is " f"{_PUSH_TIMEOUT}s)") # --porcelain is what makes the non-fast-forward decision trustworthy: it diff --git a/tests/test_git_push_budget_from_config_1631.py b/tests/test_git_push_budget_from_config_1631.py new file mode 100644 index 00000000..a43b4cf9 --- /dev/null +++ b/tests/test_git_push_budget_from_config_1631.py @@ -0,0 +1,381 @@ +"""#1631 — the push budget was a module constant, so the repo that knows the +answer could not state it. + +`.githooks/pre-push` runs the full suite when the destination is `master`/`main` +(#1242, #894). Measured on 2026-08-13: **12868 passed, 51 skipped in 309.86s**, +against `_PUSH_TIMEOUT = 300`. Ten seconds. So every master push in this +repository times out on a healthy machine with a green suite — twice measured at +302.70s and 302.90s with nothing sent — and only the third, at `:budget=1500`, +landed. + +That ten-second margin is also the argument against raising the constant. A +bumped default would put this repo in the *worse* regime — sometimes landing, +sometimes not, from the same command — and a repo with no pre-push hook wants a +*shorter* budget, because there the only thing a long one buys is a longer wait +before an honest failure. The number is per-repo in both directions, which is +what makes it configuration rather than a better constant. The constant own +comment already said so: what decides the right number "is not visible from +here". + +So `ops.git-push.budget` in `.supertool.json`, merged over the preset entry +key-by-key by core the way every other per-op key is. Precedence is +flag > config > 300, and 300 is still the answer when neither is set. + +**The invariant this must not break.** `_PUSH_TIMEOUT` and `_PUSH_TIMEOUT_MAX` +exist under a documented constraint: the push budget stays **strictly** below +`ops.git-push.timeout`, or a process killed by supertool outer cap can verify +nothing and the caller acts on a bare `FAIL (timeout)` for a push that landed +(#399) — and on the recovery path `_report_recovery_timeout` is the only thing +that would have said the worktree is paused mid-rebase (#1615). A configured +budget is therefore validated against the op timeout **from the same merged +entry**, and **refused rather than clamped** when it is not strictly under, the +way `_parse_budget` already refuses the flag. A clamp converts "the caller asked +for a number and got a different one" into a discovery made at the moment a push +cannot be verified. + +**Why one entry read off disk rather than the env.** Core exports every +non-reserved op key to the subprocess as `SUPERTOOL_`, so `budget` arrives +that way for free — but `timeout` is in core `_RESERVED_KEYS` and deliberately +does not (`tests/test_custom_ops.py` pins its absence). Validating a budget that +came from the environment against a timeout that came from disk is checking two +answers to the same question against each other. Both come from one read. +""" +from __future__ import annotations + +import importlib.util +import io +import json +import os +import shutil +import subprocess +import sys +import tempfile +from contextlib import redirect_stdout +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent)) +from _changelog_findable import assert_change_is_findable # noqa: E402 + +ROOT = Path(__file__).parent.parent +PRESET = ROOT / "presets" / "git" / "push.py" +_spec = importlib.util.spec_from_file_location("git_push_1631", PRESET) +assert _spec is not None and _spec.loader is not None +push = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(push) + +#: The op-level cap this budget has to stay strictly under, read from the same +#: file the op is dispatched from rather than restated here. +OP_TIMEOUT = json.loads( + (ROOT / "presets" / "git.json").read_text(encoding="utf-8") +)["ops"]["git-push"]["timeout"] + + +_HERMETIC_ENV = { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_AUTHOR_NAME": "Mate", + "GIT_AUTHOR_EMAIL": "mate@t", + "GIT_COMMITTER_NAME": "Mate", + "GIT_COMMITTER_EMAIL": "mate@t", + "GIT_TERMINAL_PROMPT": "0", +} + + +def _run(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + return subprocess.run(["git"] + args, cwd=cwd, env=_HERMETIC_ENV, + capture_output=True, text=True, timeout=60, + encoding="utf-8", errors="replace") + + +class _Sandbox: + """Bare remote + `mine`, the clone the op is driven in.""" + + def __init__(self) -> None: + self.tmp = tempfile.mkdtemp(prefix="st1631_") + self.remote = os.path.join(self.tmp, "remote.git") + self.mine = os.path.join(self.tmp, "mine") + assert _run(["init", "--bare", "-b", "feature", "remote.git"], + self.tmp).returncode == 0 + assert _run(["clone", self.remote, "mine"], self.tmp).returncode == 0 + assert _run(["checkout", "-b", "feature"], self.mine).returncode == 0 + Path(self.mine, "a.txt").write_text("base", encoding="utf-8") + assert _run(["add", "a.txt"], self.mine).returncode == 0 + assert _run(["commit", "-m", "base"], self.mine).returncode == 0 + + def configure(self, entry: object) -> None: + """Write `.supertool.json` with `ops.git-push` set to `entry`.""" + Path(self.mine, ".supertool.json").write_text( + json.dumps({"ops": {"git-push": entry}}), encoding="utf-8") + + def remote_has_feature(self) -> bool: + return _run(["rev-parse", "--verify", "refs/heads/feature"], + self.remote).returncode == 0 + + def drive_push(self, *argv: str) -> tuple[int, str]: + prev_cwd = os.getcwd() + prev_argv = sys.argv[:] + prev_env = {k: os.environ.get(k) for k in _HERMETIC_ENV} + os.chdir(self.mine) + os.environ.update({k: v for k, v in _HERMETIC_ENV.items() + if v is not None}) + sys.argv = ["push.py", *argv] + buf = io.StringIO() + try: + with redirect_stdout(buf): + rc = push.main() + finally: + os.chdir(prev_cwd) + sys.argv = prev_argv + for k, v in prev_env.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + return rc, buf.getvalue() + + def close(self) -> None: + shutil.rmtree(self.tmp, ignore_errors=True) + + +@pytest.fixture +def box(): + s = _Sandbox() + try: + yield s + finally: + s.close() + + +@pytest.fixture(autouse=True) +def _reset_budget(): + """Module state, reset in `main()` prologue. Several tests here reach past + `main()`, so it is reset around every one.""" + push._BUDGET["seconds"] = None + yield + push._BUDGET["seconds"] = None + + +@pytest.fixture +def entry(monkeypatch: pytest.MonkeyPatch): + """Pin the merged op entry `_config_budget` reads.""" + def _set(**keys: object) -> None: + monkeypatch.setattr(push, "_merged_op_entry", lambda: dict(keys)) + return _set + + +def _verdict(out: str) -> str: + lines = [ln for ln in out.splitlines() if ln.strip()] + assert lines, "no output at all:" + os.linesep + out + assert lines[-1].startswith("[result] "), ( + "the receipt does not end on a verdict:" + os.linesep + out) + return lines[-1] + + +# --------------------------------------------------------------------------- +# absence — 300 is still the answer when nothing is configured +# --------------------------------------------------------------------------- + +def test_no_configured_budget_leaves_the_300s_default_in_force(entry) -> None: + entry(timeout=OP_TIMEOUT) + assert push._config_budget() == (None, "") + assert push._push_budget() == push._PUSH_TIMEOUT == 300 + + +def test_an_empty_entry_is_not_a_configured_budget(entry) -> None: + """No preset on disk, no config: absence, not a refusal. A repo that never + asked for this must not be refused a push by the machinery that serves it.""" + entry() + assert push._config_budget() == (None, "") + + +# --------------------------------------------------------------------------- +# a usable value +# --------------------------------------------------------------------------- + +def test_a_configured_budget_strictly_under_the_op_timeout_is_taken(entry) -> None: + entry(timeout=OP_TIMEOUT, budget=1500) + assert push._config_budget() == (1500, "") + + +def test_the_shipped_op_timeout_leaves_room_for_this_repos_own_suite() -> None: + """The measurement that filed this: 309.86s of pre-push suite. A budget + covering it has to fit under both ceilings, or the key cannot answer the + issue it was added for.""" + assert 310 < push._PUSH_TIMEOUT_MAX < OP_TIMEOUT + + +# --------------------------------------------------------------------------- +# the #399 invariant — refused, never clamped, with both numbers named +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("budget,cap", [(600, 600), (900, 600)]) +def test_a_configured_budget_not_strictly_under_the_op_timeout_is_refused( + entry, budget: int, cap: int) -> None: + entry(timeout=cap, budget=budget) + seconds, why = push._config_budget() + assert seconds is None, "clamped instead of refused" + assert str(budget) in why, why + assert str(cap) in why, why + assert "ops.git-push.timeout" in why, why + + +def test_the_refusal_names_the_config_key_that_has_to_change(entry) -> None: + entry(timeout=600, budget=600) + why = push._config_budget()[1] + assert "ops.git-push.budget" in why, why + + +def test_a_configured_budget_over_the_op_ceiling_is_refused(entry) -> None: + entry(timeout=OP_TIMEOUT, budget=push._PUSH_TIMEOUT_MAX + 1) + seconds, why = push._config_budget() + assert seconds is None + assert str(push._PUSH_TIMEOUT_MAX) in why, why + + +@pytest.mark.parametrize("cap", [None, "1920", 0, -1, True, 19.2, [1920]]) +def test_a_configured_budget_is_refused_when_the_op_timeout_cannot_be_read( + entry, cap: object) -> None: + """Three states, not two. A check that cannot run declines; it does not + hand back the shape of a clean result (docs/validators.md).""" + keys: dict = {"budget": 900} + if cap is not None: + keys["timeout"] = cap + entry(**keys) + seconds, why = push._config_budget() + assert seconds is None, "a budget nothing could be checked against was taken" + assert "ops.git-push.timeout" in why, why + + +# --------------------------------------------------------------------------- +# shape — this value reaches subprocess as a timeout, and arrives from a file +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("raw", ["1500", "", 1500.0, True, False, [1500], + {"seconds": 1500}]) +def test_a_configured_budget_that_is_not_a_whole_number_is_refused( + entry, raw: object) -> None: + entry(timeout=OP_TIMEOUT, budget=raw) + seconds, why = push._config_budget() + assert seconds is None, "a non-integer budget reached the push clock" + assert why + + +@pytest.mark.parametrize("raw", [0, -30]) +def test_a_non_positive_configured_budget_is_refused(entry, raw: int) -> None: + entry(timeout=OP_TIMEOUT, budget=raw) + seconds, why = push._config_budget() + assert seconds is None + assert why + + +def test_a_refusal_never_spans_lines(entry) -> None: + """The value is somebody config file text. It is rendered flat, so it cannot + forge a second receipt line around itself.""" + entry(timeout=OP_TIMEOUT, budget="900\nStatus: pushed ✓") + why = push._config_budget()[1] + assert why + assert "\n" not in why, repr(why) + + +# --------------------------------------------------------------------------- +# the merge itself — one entry, both keys, read the way core reads it +# --------------------------------------------------------------------------- + +def test_the_entry_merges_the_project_config_over_the_preset_key_by_key( + box) -> None: + """The project supplies `budget` alone; `timeout` still comes from the + preset. Both keys out of one read is the whole point — the pair is + validated against itself.""" + box.configure({"budget": 1500}) + prev = os.getcwd() + os.chdir(box.mine) + try: + merged = push._merged_op_entry() + finally: + os.chdir(prev) + assert merged.get("budget") == 1500 + assert merged.get("timeout") == OP_TIMEOUT + + +def test_a_project_timeout_override_is_the_one_the_budget_is_checked_against( + box) -> None: + """A project that lowers the op timeout lowers the ceiling with it. Reading + the preset 1920 here would authorise a budget the outer cap kills.""" + box.configure({"budget": 900, "timeout": 600}) + prev = os.getcwd() + os.chdir(box.mine) + try: + seconds, why = push._config_budget() + finally: + os.chdir(prev) + assert seconds is None, "checked against the preset timeout, not the merged one" + assert "600" in why, why + + +# --------------------------------------------------------------------------- +# precedence, end to end: flag > config > 300 +# --------------------------------------------------------------------------- + +def test_the_flag_wins_over_the_configured_budget(box) -> None: + box.configure({"budget": 900}) + rc, out = box.drive_push("budget=1200") + assert rc == 0, out + assert "Push budget: 1200s (:budget" in out, out + + +def test_the_configured_budget_applies_when_no_flag_is_given(box) -> None: + box.configure({"budget": 900}) + rc, out = box.drive_push() + assert rc == 0, out + assert "Push budget: 900s (ops.git-push.budget" in out, out + + +def test_an_unconfigured_repo_pushes_on_the_default_and_says_nothing( + box) -> None: + """The default is not printed — a receipt line about a number nobody chose + is a line nobody reads.""" + rc, out = box.drive_push() + assert rc == 0, out + assert "Push budget:" not in out, out + + +def test_an_unusable_configured_budget_refuses_before_anything_is_pushed( + box) -> None: + box.configure({"budget": "soon"}) + rc, out = box.drive_push() + assert rc == 2, out + assert not box.remote_has_feature(), "pushed under a budget it refused" + assert "ops.git-push.budget" in out, out + assert "no push attempted" in _verdict(out), out + + +def test_a_broken_configured_budget_is_inert_when_the_flag_decides(box) -> None: + """Precedence means the config is not consulted, so it cannot refuse a push + whose clock it does not set.""" + box.configure({"budget": "soon"}) + rc, out = box.drive_push("budget=1200") + assert rc == 0, out + assert "Push budget: 1200s (:budget" in out, out + + +# --------------------------------------------------------------------------- +# findable by someone who did not build it +# --------------------------------------------------------------------------- + +def test_the_change_is_findable() -> None: + assert_change_is_findable(1631) + + +def test_the_config_key_is_documented_where_a_user_would_look() -> None: + doc = (ROOT / "docs" / "presets" / "git.md").read_text(encoding="utf-8") + assert "ops.git-push.budget" in doc, ( + "a config key nobody can find out about is not shipped") + + +def test_the_timeout_receipt_points_at_the_config_key_too() -> None: + """The receipt a timed-out push prints is where this is read. Naming only + the flag sends the caller back to retyping the number every session.""" + assert "ops.git-push.budget" in push._budget_advice() diff --git a/tests/test_op_registry_1356.py b/tests/test_op_registry_1356.py index 7294ae2b..1a572a17 100644 --- a/tests/test_op_registry_1356.py +++ b/tests/test_op_registry_1356.py @@ -313,10 +313,15 @@ class TestThisRepoIsTheInstance: The set is a register, not a limit: it grows when this repo deliberately adds a config key to a shipped op. `watches` and `channel` joined on 2026-08-12 carrying `watch_name`, which is how #1477's channel name reaches - those two surfaces at all (`presets/watch/README.md`). Update the list with - the reason; never relax the assertion, because a shadowing entry that - nobody registered is exactly the stub-replacing-a-definition defect this - file exists for. + those two surfaces at all (`presets/watch/README.md`). `git-push` joined on + 2026-08-13 carrying `budget: 1500` (#1631): this repo's own pre-push hook + runs the full suite on a push to `master` — 309.86s measured — against a + 300s default, so every such push timed out having sent nothing. This entry + is also the live instance of the key-by-key merge, since it supplies + `budget` alone and the op's `timeout` has to survive it or the budget is + validated against nothing. Update the list with the reason; never relax the + assertion, because a shadowing entry that nobody registered is exactly the + stub-replacing-a-definition defect this file exists for. """ def test_the_shadowed_ops_are_still_the_registered_set(self) -> None: @@ -332,7 +337,7 @@ def test_the_shadowed_ops_are_still_the_registered_set(self) -> None: shadowed = sorted(n for n, e in config["ops"].items() if isinstance(e, dict) and n in preset_ops) assert shadowed == [ - "channel", "dashboard", "git-diff", "radar", "watches", + "channel", "dashboard", "git-diff", "git-push", "radar", "watches", ], shadowed def test_a_naive_walk_loses_git_diff_from_the_path_naming_set(self) -> None: From 85fe0d16bce9b33900ab6d460d25a6cbbdd55a6c Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:56:48 +0200 Subject: [PATCH 2/2] The timeout receipt named `_PUSH_TIMEOUT` while a configured budget was the clock, and two doc sites still described the flag as the only lever (#1631) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the independent review of the first commit. `_budget_advice` is printed by three receipts and opened with "That budget is _PUSH_TIMEOUT in presets/git/push.py". With `ops.git-push.budget` in force that sends the caller to raise a number that did not cut — #1530's own defect one indirection further in. `_BUDGET` gains a `source`, reset in `main()`'s prologue like the rest of it, and the advice names the lever that was actually pulled; every arm still hands back a runnable `git-push:budget=SECONDS`. `tests/test_git_push_budget_1530.py`'s reset fixture cleared `seconds` and not `source`, so a driven push decided which remedy a later receipt offered. docs/presets/git.md: the new section called `:budget=SECONDS` "per-call" two paragraphs above the #1615 section stating it is NOT a per-call timeout; and the hook-relay paragraph still named `_PUSH_TIMEOUT` as the clock a push outlasts. Co-Authored-By: Max --- docs/presets/git.md | 4 +- presets/git/push.py | 47 +++++++++++++------ tests/test_git_push_budget_1530.py | 5 ++ .../test_git_push_budget_from_config_1631.py | 41 +++++++++++++++- 4 files changed, 79 insertions(+), 18 deletions(-) diff --git a/docs/presets/git.md b/docs/presets/git.md index 3eb05164..5088dcaf 100644 --- a/docs/presets/git.md +++ b/docs/presets/git.md @@ -530,7 +530,7 @@ The line above the relay has three states, and it is a claim about configuration `ran` with nothing after it gets its own sentence — *it printed nothing, so this receipt cannot say which arm it took* — because a silent hook and an absent one otherwise render identically. -One arm carries no relay and says so. A push that outlasts `_PUSH_TIMEOUT` is killed and its captured output dies with it, so the timeout receipt states that the hook's words were never captured rather than leaving a blank that reads as a hook with nothing to say. +One arm carries no relay and says so. A push that outlasts its push budget — `_PUSH_TIMEOUT`, `ops.git-push.budget` or `:budget=SECONDS`, whichever was in force — is killed and its captured output dies with it, so the timeout receipt states that the hook's words were never captured rather than leaving a blank that reads as a hook with nothing to say. **The rebase-recovery route carries all of it too, and carried none of it until [#1490](https://github.com/Digital-Process-Tools/claude-supertool/issues/1490).** A non-fast-forward hands the push to `_recover_by_rebase`, which runs its **own** `git push` and prints its own receipts — and neither the disclosure above nor the head/tail bound followed it there. So `Status: pushed ✓ (rebased onto remote)` was the one landed-push receipt in this op that said nothing about the hook at all, which is #1448's premise turned back on it: a push that lands after a rebase is precisely a push whose hook has just run. Both of that route's `--- git output ---` dumps are bounded now as well, on the same 5/30 as the straight route, and the rejected-after-rebase arm is where the transcript is largest for exactly the same reason. The `rebase could not start` arm prints no hook line, deliberately: no push of that route's own has run yet, so there is nothing it could say about a hook that would be about the failure it is reporting. @@ -704,7 +704,7 @@ Three calls name their own budget instead, because they are the ones that legiti #### `ops.git-push.budget` — the default your repository chooses -`:budget=SECONDS` is per-call, and there are repositories where it is the right answer on **every** call: a pre-push hook that runs the suite on a push to `master` cannot finish inside 300s, so the flag has to be retyped every session or the push times out having sent nothing. Set the default once instead ([#1631](https://github.com/Digital-Process-Tools/claude-supertool/issues/1631)): +`:budget=SECONDS` is per *invocation* — see the deadline section below for what it means within one — and there are repositories where it is the right answer on **every** invocation: a pre-push hook that runs the suite on a push to `master` cannot finish inside 300s, so the flag has to be retyped every session or the push times out having sent nothing. Set the default once instead ([#1631](https://github.com/Digital-Process-Tools/claude-supertool/issues/1631)): ```json { diff --git a/presets/git/push.py b/presets/git/push.py index 70dd2f5d..ee2cdd00 100644 --- a/presets/git/push.py +++ b/presets/git/push.py @@ -170,8 +170,12 @@ # opened at the first `git push`; `allowed` is the clock the most recent push # was actually launched with, which the timeout receipt has to name rather than # the budget it was cut from. +# `source` is #1631: with three places a budget can come from, a receipt that +# names the wrong one sends the caller to raise a number that did not cut. It +# is module state for the same reason `seconds` is — `_budget_advice` is called +# from three receipts, none of which has the parse site in scope. _BUDGET: dict[str, object] = {"seconds": None, "deadline": None, - "allowed": None} + "allowed": None, "source": ""} def _push_budget() -> int: @@ -1936,19 +1940,33 @@ def _budget_advice() -> str: which left `:no-verify` — skipping the gate — as the one flag that helped a push that could not fit. `.githooks/pre-push` runs the full suite when the destination is master/main, and that suite has measured 530.71s. + + #1631 is why it names the source that was actually in force. There are now + three, and a receipt that pointed at `_PUSH_TIMEOUT` while a configured + `ops.git-push.budget` was the clock would send the caller to edit a number + that did not cut — which is #1530's own defect one indirection further in. """ + source = _BUDGET["source"] + if source == f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}": + where = (f"That budget is `ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}` in " + f"this repository's .supertool.json — raise it there, or " + f"override this one call with `git-push:budget=SECONDS`") + elif source == ":budget": + where = ("That budget is the `git-push:budget=SECONDS` this call " + "passed — pass a bigger one") + else: + where = (f"That budget is _PUSH_TIMEOUT in presets/git/push.py — ask " + f"for more of it with `git-push:budget=SECONDS`, or state " + f"this repository's own default once as " + f"`ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}` in .supertool.json " + f"(#1631), which the flag still overrides") return ( - f"That budget is _PUSH_TIMEOUT in presets/git/push.py — ask for more " - f"of it with `git-push:budget=SECONDS` (up to {_PUSH_TIMEOUT_MAX}s), " + f"{where} (up to {_PUSH_TIMEOUT_MAX}s), " f"which is the right lever when a pre-push hook runs a suite. It is " f"NOT ops.git-push.timeout: that op-level cap bounds the whole " f"process, raising it alone will not move this one, and this budget " f"has to stay strictly under it or a push killed by the outer cap can " - f"verify nothing (#399). A repository whose own pre-push hook runs a " - f"suite can state the number once instead of retyping it every " - f"session: `\"git-push\": {{\"budget\": SECONDS}}` under `ops` in " - f".supertool.json (ops.git-push.budget), which the flag still " - f"overrides.") + f"verify nothing (#399).") def _report_push_timeout(branch: str, head_before: str, @@ -2421,7 +2439,8 @@ def main() -> int: use_utf8_stdout() _RUN.update({"phase": "not-attempted", "branch": "", "remote": "", "ref": "", "target": "", "verdict": False}) - _BUDGET.update({"seconds": None, "deadline": None, "allowed": None}) + _BUDGET.update({"seconds": None, "deadline": None, "allowed": None, + "source": ""}) try: return _push_op() except Exception as exc: # noqa: BLE001 — deliberate; see _crash_receipt @@ -2469,7 +2488,7 @@ def _push_op() -> int: _result(f"NOT PUSHED - no push attempted (unusable :budget — " f"{budget_why})") return 2 - budget_source = ":budget" + _BUDGET["source"] = ":budget" if budget is None: # Precedence is flag > config > `_PUSH_TIMEOUT` (#1631). The config is # not consulted at all when the flag decided — that is what precedence @@ -2484,8 +2503,8 @@ def _push_op() -> int: _result(f"NOT PUSHED - no push attempted (unusable " f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY} — {config_why})") return 2 - if budget is not None: - budget_source = f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}" + _BUDGET["source"] = ("" if budget is None + else f"ops.{_OP_NAME}.{_CONFIG_BUDGET_KEY}") _BUDGET["seconds"] = budget upstream, upstream_why = _upstream_ref() @@ -2595,8 +2614,8 @@ def _push_op() -> int: # was dropped read identically from the receipt (#647). The default is # not printed: it is documented, and every receipt carrying a line # about a number nobody chose is a line nobody reads. - print(f"Push budget: {_push_budget()}s ({budget_source} — default is " - f"{_PUSH_TIMEOUT}s)") + print(f"Push budget: {_push_budget()}s ({_BUDGET['source']} — default " + f"is {_PUSH_TIMEOUT}s)") # --porcelain is what makes the non-fast-forward decision trustworthy: it # moves git's per-ref status onto stdout in a machine-readable grammar, diff --git a/tests/test_git_push_budget_1530.py b/tests/test_git_push_budget_1530.py index 5c35d999..0c363345 100644 --- a/tests/test_git_push_budget_1530.py +++ b/tests/test_git_push_budget_1530.py @@ -142,8 +142,13 @@ def _reset_budget(): every one — otherwise a case that sets a budget decides the next one's clock.""" push._BUDGET["seconds"] = None + # `source` too, since #1631: a run that reaches past `main()` leaves the + # lever it took behind, and `_budget_advice` reads it. Resetting only + # `seconds` let a driven push decide which remedy a later receipt offered. + push._BUDGET["source"] = "" yield push._BUDGET["seconds"] = None + push._BUDGET["source"] = "" def _verdict(out: str) -> str: diff --git a/tests/test_git_push_budget_from_config_1631.py b/tests/test_git_push_budget_from_config_1631.py index a43b4cf9..ab9f3127 100644 --- a/tests/test_git_push_budget_from_config_1631.py +++ b/tests/test_git_push_budget_from_config_1631.py @@ -154,8 +154,10 @@ def _reset_budget(): """Module state, reset in `main()` prologue. Several tests here reach past `main()`, so it is reset around every one.""" push._BUDGET["seconds"] = None + push._BUDGET["source"] = "" yield push._BUDGET["seconds"] = None + push._BUDGET["source"] = "" @pytest.fixture @@ -375,7 +377,42 @@ def test_the_config_key_is_documented_where_a_user_would_look() -> None: "a config key nobody can find out about is not shipped") -def test_the_timeout_receipt_points_at_the_config_key_too() -> None: +# --------------------------------------------------------------------------- +# the timeout receipt names the source that was actually in force +# --------------------------------------------------------------------------- + +def test_the_advice_offers_the_config_key_when_nothing_was_configured() -> None: """The receipt a timed-out push prints is where this is read. Naming only the flag sends the caller back to retyping the number every session.""" - assert "ops.git-push.budget" in push._budget_advice() + advice = push._budget_advice() + assert "_PUSH_TIMEOUT in presets/git/push.py" in advice, advice + assert "ops.git-push.budget" in advice, advice + + +def test_the_advice_names_the_config_key_when_the_config_is_the_clock() -> None: + """A receipt pointing at `_PUSH_TIMEOUT` while a configured budget was the + clock sends the caller to raise a number that did not cut — #1530's own + defect one indirection further in.""" + push._BUDGET["source"] = "ops.git-push.budget" + advice = push._budget_advice() + assert "ops.git-push.budget" in advice, advice + assert ".supertool.json" in advice, advice + assert "_PUSH_TIMEOUT in presets/git/push.py" not in advice, advice + + +def test_the_advice_does_not_send_a_flag_caller_to_edit_a_constant() -> None: + push._BUDGET["source"] = ":budget" + advice = push._budget_advice() + assert "_PUSH_TIMEOUT in presets/git/push.py" not in advice, advice + assert ":budget=SECONDS" in advice, advice + + +def test_the_budget_source_is_reset_in_the_main_prologue() -> None: + """Module state on the same terms as `seconds` — a source surviving a run + would name the previous call's lever on this one's receipt. Asserted on the + source of `main()` rather than by running it: `conftest`'s + PRESET_SELF_CLEARING_GLOBALS guard credits only a literal item assignment + in the prologue, and driving `main()` from this checkout would push it.""" + src = PRESET.read_text(encoding="utf-8") + prologue = src.split("def main()", 1)[1].split("def ", 1)[0] + assert '"source": ""' in prologue, prologue