From 5802ba675105bcf313deb5bdea6d39259abea48e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 26 Oct 2026 11:53:28 -0400 Subject: [PATCH] fix(ci): hold rate-limit default changes for a person The limits.api defaults that stop a lane before GitHub rate-limits the account lived in pr-pipeline.schema.json, which is not on the merge hold floor, so a pull request raising or removing them could merge on the AI review lanes alone. Flooring the whole schema would hold most of its frequent changes for a person. Move the defaults, unchanged, into .github/actions/log-api-budget/rate-limits.json, their only copy: rate-guard.mjs and babysit_gh.py read it (babysit_gh.py's GITHUB_TOKEN_LIMIT_CAP constant now comes from it too), the schema drops its defaults, and every hold list floors the file (HOLD_PATH_FLOOR, babysit_risk.HIGH_FLOOR, ci-status's hold_patterns, both review lanes' hold-paths). A missing or invalid file still leaves rate-guard warning that the read floor is unchecked while stops hold, and still refuses every babysit_gh call; both now also reject a malformed value. Co-Authored-By: Claude Opus 5.5 --- .../fix-floor-rate-limit-defaults-a0b4a200.md | 7 ++ .../fix-floor-rate-limit-defaults-ab3c71bb.md | 8 ++ .../actions/log-api-budget/api-budget.bash | 2 +- .../log-api-budget/log-api-budget.test.mjs | 6 +- .github/actions/log-api-budget/rate-guard.mjs | 56 ++++++++----- .../log-api-budget/rate-guard.test.mjs | 76 ++++++++++++++---- .../actions/log-api-budget/rate-limits.json | 7 ++ .github/actions/resolve-config/resolve.mjs | 2 +- .../pr-merge/enable-auto-merge.test.mjs | 1 + .github/scripts/pr-merge/merge-holds.mjs | 1 + .github/scripts/pr-merge/merge-holds.test.mjs | 1 + .../workflows/intake-label-needs-triage.yml | 1 - .github/workflows/pr-require-checks.yml | 10 +-- .github/workflows/pr-review-hosted.yml | 1 + .../workflows/pr-review-security-hosted.yml | 1 + docs/conventions/pr-pipeline.yaml | 6 +- docs/conventions/pr-pipeline/README.md | 5 +- .../pr-pipeline/pr-pipeline.schema.json | 13 +--- .../skills/babysit-prs/scripts/babysit_gh.py | 60 +++++++------- .../babysit-prs/scripts/babysit_risk.py | 1 + .../tests/test_babysit_gh_rate_guard.py | 78 +++++++++++++++---- .../scripts/tests/test_babysit_risk.py | 4 + .../scripts/tests/test_merge_holds_sync.py | 14 +++- scripts/ci-lanes-pr-hold.test.sh | 1 + 24 files changed, 251 insertions(+), 111 deletions(-) create mode 100644 .changes/pr-pipeline/fix-floor-rate-limit-defaults-a0b4a200.md create mode 100644 .changes/source-control/fix-floor-rate-limit-defaults-ab3c71bb.md create mode 100644 .github/actions/log-api-budget/rate-limits.json diff --git a/.changes/pr-pipeline/fix-floor-rate-limit-defaults-a0b4a200.md b/.changes/pr-pipeline/fix-floor-rate-limit-defaults-a0b4a200.md new file mode 100644 index 0000000000..1a0a9c7984 --- /dev/null +++ b/.changes/pr-pipeline/fix-floor-rate-limit-defaults-a0b4a200.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- The `limits.api` defaults (each pool's `plan`, `writer-pause`, `breaker`, `read-floor`, github-token's `limit-cap`, content-creation's `per-minute` and `per-hour`) moved out of `pr-pipeline.schema.json` into [`.github/actions/log-api-budget/rate-limits.json`](https://github.com/melodic-software/claude-code-plugins/blob/main/.github/actions/log-api-budget/rate-limits.json), their only copy, with the same values. The schema states no default for them now. The merge path floor (`merge-holds.mjs` `HOLD_PATH_FLOOR`, `babysit_risk.HIGH_FLOOR`, ci-status's `hold_patterns` and both review lanes' `hold-paths`) holds that file, so a change to a rate-limit default waits for a person, while a schema change does not. diff --git a/.changes/source-control/fix-floor-rate-limit-defaults-ab3c71bb.md b/.changes/source-control/fix-floor-rate-limit-defaults-ab3c71bb.md new file mode 100644 index 0000000000..fb08315b08 --- /dev/null +++ b/.changes/source-control/fix-floor-rate-limit-defaults-ab3c71bb.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Changed + +- `babysit_gh.py` reads its `limits.api` defaults (`writer-pause`, `read-floor`, and github-token's `limit-cap`, which replaces the `GITHUB_TOKEN_LIMIT_CAP` constant) from `.github/actions/log-api-budget/rate-limits.json` instead of the pipeline schema, with the same values. A missing or invalid file still refuses every call, and a default that is not a number now does too. +- `babysit_risk.HIGH_FLOOR` also holds `.github/actions/log-api-budget/rate-limits.json`, the rate-limit defaults `rate-guard.mjs` reads, so a change to one waits for a person. diff --git a/.github/actions/log-api-budget/api-budget.bash b/.github/actions/log-api-budget/api-budget.bash index 6a05af2e32..f777f289a7 100644 --- a/.github/actions/log-api-budget/api-budget.bash +++ b/.github/actions/log-api-budget/api-budget.bash @@ -59,7 +59,7 @@ # answer is a 2xx; under # API_BUDGET_READ_FLOOR of the planned budget # left, warns once (unset: no floor; ci-status -# sets it to the schema's read-floor default); +# sets it to the read-floor default); # the plan is the header's limit, at most # API_BUDGET_LIMIT_CAP (unset: no cap; ci-status # sets github-token's limit-cap default) diff --git a/.github/actions/log-api-budget/log-api-budget.test.mjs b/.github/actions/log-api-budget/log-api-budget.test.mjs index e839d4fa04..5eb6e18f7b 100644 --- a/.github/actions/log-api-budget/log-api-budget.test.mjs +++ b/.github/actions/log-api-budget/log-api-budget.test.mjs @@ -657,9 +657,9 @@ test("ci-status sets the shell limit cap to github-token's limit-cap default", ( assert.equal(Number(cap[1]), apiLimits({ resolvedPath: "" })["github-token"]["limit-cap"]); }); -// ci-status has no checkout of the schema, so its wrapper's floor is set in -// the job env, pinned here to the schema default the Node guard reads. -test("ci-status sets the shell read floor to the schema's read-floor default", () => { +// ci-status has no checkout of rate-limits.json, so its wrapper's floor is set in +// the job env, pinned here to the rate-limits.json default the Node guard reads. +test("ci-status sets the shell read floor to the read-floor default", () => { const floor = /\n {6}API_BUDGET_READ_FLOOR: "?([0-9.]+)"?\n/.exec(CI); assert.ok(floor, "ci-status sets API_BUDGET_READ_FLOOR"); assert.equal(Number(floor[1]), apiLimits({ resolvedPath: "" })["github-token"]["read-floor"]); diff --git a/.github/actions/log-api-budget/rate-guard.mjs b/.github/actions/log-api-budget/rate-guard.mjs index 67c7ffbfea..008e25e2e8 100644 --- a/.github/actions/log-api-budget/rate-guard.mjs +++ b/.github/actions/log-api-budget/rate-guard.mjs @@ -15,8 +15,9 @@ // the header's remaining and GitHub's refusals, never the plan. // // The margins are limits.api in pr-pipeline.yaml, read from resolve-config's -// output (RESOLVED_CONFIG), over the defaults in pr-pipeline.schema.json, -// which this module reads at load: the schema holds the only copy. +// output (RESOLVED_CONFIG), over the defaults in rate-limits.json beside this +// module: that file holds the only copy, and every hold list floors it, so a +// change to a default waits for a person. // rate-guard.vectors.json pins classify's decisions for every port. import { appendFileSync, mkdirSync, readFileSync } from "node:fs"; @@ -46,24 +47,39 @@ const MAX_RESET = 999_999_999_999; export const API_POOLS = Object.freeze(["github-token", "lanes-app", "graphql", "owner-token", "content-creation"]); -const SCHEMA = new URL("../../../docs/conventions/pr-pipeline/pr-pipeline.schema.json", import.meta.url); +const DEFAULTS = new URL("./rate-limits.json", import.meta.url); -const defaultsOf = (properties) => - Object.fromEntries( - Object.entries(properties) - .filter(([, property]) => "default" in property) - .map(([key, property]) => [key, property.default]), - ); +// Each field's check: the shares, then the counts. per-minute and per-hour +// belong to content-creation alone. +const SHARE = (value) => typeof value === "number" && value >= 0 && value <= 1; +const COUNT = (value) => Number.isInteger(value) && value >= 1; +const FIELDS = { plan: (value) => SHARE(value) && value > 0, "writer-pause": SHARE, breaker: SHARE, "read-floor": SHARE, "limit-cap": COUNT }; +const CONTENT_FIELDS = { ...FIELDS, "per-minute": COUNT, "per-hour": COUNT }; +const REQUIRED = ["plan", "writer-pause", "breaker", "read-floor"]; + +// rate-limits.json, checked: every pool in API_POOLS and no other, each with +// the four shares, and every field in its range. Throws on anything else. +function readDefaults(path) { + const defaults = JSON.parse(readFileSync(path, "utf8")); + const pools = Object.keys(defaults ?? {}); + if (Array.isArray(defaults) || pools.length !== API_POOLS.length || !API_POOLS.every((name) => pools.includes(name))) { + throw new Error(`${path}: the pools must be ${API_POOLS.join(", ")}`); + } + for (const name of API_POOLS) { + const fields = name === "content-creation" ? CONTENT_FIELDS : FIELDS; + const entry = defaults[name]; + const bad = [...REQUIRED.filter((field) => !(field in (entry ?? {}))), ...Object.entries(entry ?? {}).filter(([field, value]) => !fields[field]?.(value)).map(([field]) => field)]; + if (typeof entry !== "object" || entry === null || Array.isArray(entry) || bad.length) { + throw new Error(`${path}: ${name} has a missing or invalid ${bad.join(", ") || "entry"}`); + } + } + return defaults; +} -// limits.api with every unset field filled from the schema's defaults: the -// pool's $defs entry, then the entry's own default (github-token's -// limit-cap). Throws when the schema cannot be read. -export function apiLimits({ resolvedPath = process.env.RESOLVED_CONFIG, schemaPath = SCHEMA } = {}) { - const schema = JSON.parse(readFileSync(schemaPath, "utf8")); - const defs = schema.$defs; - const entries = schema.properties.limits.properties.api.properties; - const pool = defaultsOf(defs["api-pool"].properties); - const content = { ...pool, ...defaultsOf(defs["api-content-pool"].properties) }; +// limits.api with every unset field filled from rate-limits.json. Throws when +// that file cannot be read or fails readDefaults' check. +export function apiLimits({ resolvedPath = process.env.RESOLVED_CONFIG, defaultsPath = DEFAULTS } = {}) { + const defaults = readDefaults(defaultsPath); let set = {}; if (resolvedPath) { try { @@ -73,7 +89,7 @@ export function apiLimits({ resolvedPath = process.env.RESOLVED_CONFIG, schemaPa } } return Object.fromEntries( - API_POOLS.map((name) => [name, { ...(name === "content-creation" ? content : pool), ...entries[name]?.default, ...(set[name] ?? {}) }]), + API_POOLS.map((name) => [name, { ...defaults[name], ...(set[name] ?? {}) }]), ); } @@ -311,7 +327,7 @@ export function createRateLimitLatch({ config = limits(); } catch { config = null; - log("::warning::rate guard: limits.api and its schema defaults could not be read; the read floor is not checked"); + log("::warning::rate guard: limits.api and its defaults (rate-limits.json) could not be read; the read floor is not checked"); } } const value = config?.[entryOf(pool, token)]?.[key]; diff --git a/.github/actions/log-api-budget/rate-guard.test.mjs b/.github/actions/log-api-budget/rate-guard.test.mjs index c0ae40abc2..ad095ff44d 100644 --- a/.github/actions/log-api-budget/rate-guard.test.mjs +++ b/.github/actions/log-api-budget/rate-guard.test.mjs @@ -46,23 +46,69 @@ test("classify reads a Headers object as it reads a plain one", () => { assert.deepEqual(classify({ ...vector.input, headers }, { now: NOW, readFloor: READ_FLOOR }), vector.expect); }); -test("defaults come from the schema: a changed schema default changes the module's", () => +const DEFAULTS_PATH = new URL("./rate-limits.json", import.meta.url); + +test("defaults come from rate-limits.json: a changed default changes the module's", () => withDir((dir) => { - const schema = JSON.parse(readFileSync(SCHEMA_PATH, "utf8")); - const live = apiLimits({ resolvedPath: "", schemaPath: SCHEMA_PATH }); - schema.$defs["api-pool"].properties["read-floor"].default = 0.25; - schema.$defs["api-content-pool"].properties["per-minute"].default = 7; - const path = join(dir, "schema.json"); - writeFileSync(path, JSON.stringify(schema)); - const changed = apiLimits({ resolvedPath: "", schemaPath: path }); - for (const name of ["github-token", "lanes-app", "graphql", "owner-token", "content-creation"]) { - assert.equal(changed[name]["read-floor"], 0.25, name); - assert.notEqual(live[name]["read-floor"], 0.25, name); - } + const defaults = JSON.parse(readFileSync(DEFAULTS_PATH, "utf8")); + const live = apiLimits({ resolvedPath: "", defaultsPath: DEFAULTS_PATH }); + defaults["lanes-app"]["read-floor"] = 0.25; + defaults["content-creation"]["per-minute"] = 7; + const path = join(dir, "rate-limits.json"); + writeFileSync(path, JSON.stringify(defaults)); + const changed = apiLimits({ resolvedPath: "", defaultsPath: path }); + assert.equal(changed["lanes-app"]["read-floor"], 0.25); + assert.notEqual(live["lanes-app"]["read-floor"], 0.25); assert.equal(changed["content-creation"]["per-minute"], 7); - assert.equal(changed["github-token"]["per-minute"], undefined); + assert.deepEqual(changed["github-token"], live["github-token"]); + })); + +test("a missing or invalid rate-limits.json throws, so the latch warns that the floor is unchecked", () => + withDir((dir) => { + const valid = JSON.parse(readFileSync(DEFAULTS_PATH, "utf8")); + const edits = { + "not JSON": "{", + "not an object": "[]", + "a pool missing": JSON.stringify({ ...valid, graphql: undefined }), + "an unknown pool": JSON.stringify({ ...valid, search: valid.graphql }), + "a share missing": JSON.stringify({ ...valid, "lanes-app": { ...valid["lanes-app"], "read-floor": undefined } }), + "a share past 1": JSON.stringify({ ...valid, "lanes-app": { ...valid["lanes-app"], "read-floor": 1.5 } }), + "a share as text": JSON.stringify({ ...valid, "lanes-app": { ...valid["lanes-app"], breaker: "0.2" } }), + "a plan of 0": JSON.stringify({ ...valid, graphql: { ...valid.graphql, plan: 0 } }), + "a fractional limit-cap": JSON.stringify({ ...valid, "github-token": { ...valid["github-token"], "limit-cap": 10.5 } }), + "a limit-cap of 0": JSON.stringify({ ...valid, "github-token": { ...valid["github-token"], "limit-cap": 0 } }), + "per-minute off content-creation": JSON.stringify({ ...valid, "owner-token": { ...valid["owner-token"], "per-minute": 80 } }), + "a per-hour of 0": JSON.stringify({ ...valid, "content-creation": { ...valid["content-creation"], "per-hour": 0 } }), + "an unknown field": JSON.stringify({ ...valid, graphql: { ...valid.graphql, floor: 0.1 } }), + }; + assert.throws(() => apiLimits({ resolvedPath: "", defaultsPath: join(dir, "missing.json") }), "missing"); + for (const [name, text] of Object.entries(edits)) { + const path = join(dir, "rate-limits.json"); + writeFileSync(path, text); + assert.throws(() => apiLimits({ resolvedPath: "", defaultsPath: path }), name); + } })); +test("the schema states no limits.api default: rate-limits.json holds the only copy, for the schema's pools and fields", () => { + const schema = JSON.parse(readFileSync(SCHEMA_PATH, "utf8")); + const defaults = JSON.parse(readFileSync(DEFAULTS_PATH, "utf8")); + const entries = schema.properties.limits.properties.api.properties; + const pools = [schema.$defs["api-pool"], schema.$defs["api-content-pool"]]; + for (const [name, entry] of Object.entries(entries)) { + assert.equal("default" in entry, false, name); + } + for (const pool of pools) { + for (const [field, property] of Object.entries(pool.properties)) { + assert.equal("default" in property, false, field); + } + } + assert.deepEqual(Object.keys(defaults).sort(), Object.keys(entries).sort()); + for (const [name, fields] of Object.entries(defaults)) { + const allowed = Object.keys(schema.$defs[name === "content-creation" ? "api-content-pool" : "api-pool"].properties); + assert.deepEqual(Object.keys(fields).filter((field) => !allowed.includes(field)), [], name); + } +}); + // GitHub documents GITHUB_TOKEN at 1,000 requests per hour per repository // (https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-github_token-in-github-actions); // the other pools plan to the limit their header reports. @@ -84,7 +130,7 @@ test("a resolved limit-cap caps another pool, and another github-token field kee assert.equal(limits["lanes-app"]["limit-cap"], 4000); })); -test("a resolved config's limits.api overrides the schema default for that field only", () => +test("a resolved config's limits.api overrides the rate-limits.json default for that field only", () => withDir((dir) => { const path = join(dir, "resolved.json"); writeFileSync(path, JSON.stringify({ limits: { api: { "lanes-app": { "read-floor": 0.3 } } } })); @@ -226,7 +272,7 @@ test("graphql 200 with RATE_LIMITED latches graphql", async () => { assert.equal(gh.sent.length, 1); }); -test("without a readable schema the latch still stops, and says the floor is unchecked", async () => { +test("without readable rate-limit defaults the latch still stops, and says the floor is unchecked", async () => { const lines = []; const latch = createRateLimitLatch({ now: () => NOW, diff --git a/.github/actions/log-api-budget/rate-limits.json b/.github/actions/log-api-budget/rate-limits.json new file mode 100644 index 0000000000..ac6652ddd6 --- /dev/null +++ b/.github/actions/log-api-budget/rate-limits.json @@ -0,0 +1,7 @@ +{ + "github-token": { "plan": 0.7, "writer-pause": 0.4, "breaker": 0.2, "read-floor": 0.1, "limit-cap": 1000 }, + "lanes-app": { "plan": 0.7, "writer-pause": 0.4, "breaker": 0.2, "read-floor": 0.1 }, + "graphql": { "plan": 0.7, "writer-pause": 0.4, "breaker": 0.2, "read-floor": 0.1 }, + "owner-token": { "plan": 0.7, "writer-pause": 0.4, "breaker": 0.2, "read-floor": 0.1 }, + "content-creation": { "plan": 0.7, "writer-pause": 0.4, "breaker": 0.2, "read-floor": 0.1, "per-minute": 80, "per-hour": 500 } +} diff --git a/.github/actions/resolve-config/resolve.mjs b/.github/actions/resolve-config/resolve.mjs index c1e5259a90..a8a749cefa 100644 --- a/.github/actions/resolve-config/resolve.mjs +++ b/.github/actions/resolve-config/resolve.mjs @@ -960,7 +960,7 @@ const limitsOf = (limits = {}) => ({ "fix-docs-per-run": limits["fix-docs-per-run"] ?? 2, "admission-factor": limits["admission-factor"] ?? 0.8, "admission-min-per-hour": limits["admission-min-per-hour"] ?? 1, - // As set: rate-guard.mjs fills it from the schema's defaults. + // As set: rate-guard.mjs fills it from rate-limits.json beside it. api: limits.api ?? {}, }); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 240bcb1612..2b070a7228 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -317,6 +317,7 @@ test("a floor path no configured glob names is commented on and labeled human-ga ".github/actions/check-trusted-trigger/check-trusted-trigger.mjs", ".github/actions/check-kill-switch/check-kill-switch.mjs", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/CODEOWNERS", "vendor/.gitmodules", ".github/scripts/pr-merge/merge-holds.mjs", diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 6991eab475..14d1d91e6a 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -243,6 +243,7 @@ export const HOLD_PATH_FLOOR = Object.freeze([ ".github/actions/check-kill-switch/**", ".github/actions/request-lane-token/**", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/scripts/pr-merge/merge-holds.mjs", ".github/scripts/pr-merge/enable-auto-merge.mjs", ".github/scripts/pr-merge/superseded.mjs", diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 7c3b922234..60bdbb1d1d 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -304,6 +304,7 @@ const FLOOR_FILES = [ ".github/actions/check-trusted-trigger/check-trusted-trigger.mjs", ".github/actions/check-kill-switch/check-kill-switch.mjs", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/CODEOWNERS", "plugins/foo/codeowners", ".gitmodules", diff --git a/.github/workflows/intake-label-needs-triage.yml b/.github/workflows/intake-label-needs-triage.yml index b0724ce65b..eac6cd2cbc 100644 --- a/.github/workflows/intake-label-needs-triage.yml +++ b/.github/workflows/intake-label-needs-triage.yml @@ -85,7 +85,6 @@ jobs: /.github/actions/log-api-budget/ /.github/standards/trusted-actors/ /.node-version - /docs/conventions/pr-pipeline/pr-pipeline.schema.json sparse-checkout-cone-mode: false - name: Set up Node diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 799aa2902f..8ebad735fa 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3104,9 +3104,9 @@ jobs: # wrapper and the job-end api-budget notice, since this job has no # checkout of that file. The read helper step below puts it in # gh-read.sh, so each step that sources that file reads through it. - # Its read floor and limit cap are pr-pipeline.schema.json's read-floor - # and github-token limit-cap defaults (log-api-budget.test.mjs checks), - # as this job has no schema checkout. + # Its read floor and limit cap are github-token's read-floor and + # limit-cap defaults in .github/actions/log-api-budget/rate-limits.json + # (log-api-budget.test.mjs checks), as this job has no checkout of it. API_BUDGET_READ_FLOOR: "0.1" API_BUDGET_LIMIT_CAP: "1000" API_BUDGET_SH: | @@ -3171,7 +3171,7 @@ jobs: # answer is a 2xx; under # API_BUDGET_READ_FLOOR of the planned budget # left, warns once (unset: no floor; ci-status - # sets it to the schema's read-floor default); + # sets it to the read-floor default); # the plan is the header's limit, at most # API_BUDGET_LIMIT_CAP (unset: no cap; ci-status # sets github-token's limit-cap default) @@ -4281,7 +4281,7 @@ jobs: } if [[ $AUTHOR_ID =~ ^[0-9]+$ && " $exempt_author_ids " == *" $AUTHOR_ID "* ]]; then # The trust-gate paths, as bash patterns (`*` crosses `/`). - hold_patterns=('.github/standards/trusted-actors/*' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/request-lane-token/*' '.github/actions/check-kill-switch/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-merge.yml' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' 'docs/conventions/pr-pipeline.yaml' 'CODEOWNERS' '*/CODEOWNERS' '.gitmodules' '*/.gitmodules') + hold_patterns=('.github/standards/trusted-actors/*' '.github/actions/check-trusted-trigger/*' '.github/actions/resolve-config/*' '.github/actions/request-lane-token/*' '.github/actions/check-kill-switch/*' '.github/actions/log-api-budget/rate-guard.mjs' '.github/actions/log-api-budget/rate-limits.json' '.github/scripts/pr-merge/merge-holds.mjs' '.github/scripts/pr-merge/enable-auto-merge.mjs' '.github/scripts/pr-merge/superseded.mjs' '.github/scripts/pr-merge/clear-escalations.mjs' '.github/scripts/pr-unblock/address-feedback.mjs' '.github/scripts/pr-unblock/gate-source.mjs' '.github/scripts/pr-unblock/thread-scope.mjs' '.github/scripts/work-resolve-issue/report-outcome.mjs' '.github/scripts/work-resolve-issue/limits.mjs' '.github/workflows/pr-require-checks.yml' '.github/workflows/pr-merge.yml' 'plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_check_inputs.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_checks.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_classify.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_dependabot.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_feedback.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_repo_config.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_review_trigger.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_run_cache.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_state.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_util.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py' 'plugins/source-control/skills/babysit-prs/scripts/babysit_whole_review.py' 'docs/conventions/pr-pipeline.yaml' 'CODEOWNERS' '*/CODEOWNERS' '.gitmodules' '*/.gitmodules') if [[ ! $PR_NUMBER =~ ^[1-9][0-9]*$ ]]; then echo "::error::the pull request number '$PR_NUMBER' is not a number." exit 1 diff --git a/.github/workflows/pr-review-hosted.yml b/.github/workflows/pr-review-hosted.yml index 323ec9c2bc..e372521048 100644 --- a/.github/workflows/pr-review-hosted.yml +++ b/.github/workflows/pr-review-hosted.yml @@ -206,6 +206,7 @@ jobs: .github/actions/request-lane-token/** .github/actions/check-kill-switch/** .github/actions/log-api-budget/rate-guard.mjs + .github/actions/log-api-budget/rate-limits.json .github/scripts/pr-merge/merge-holds.mjs .github/scripts/pr-merge/enable-auto-merge.mjs .github/scripts/pr-merge/superseded.mjs diff --git a/.github/workflows/pr-review-security-hosted.yml b/.github/workflows/pr-review-security-hosted.yml index 2a72694005..9e5d21468c 100644 --- a/.github/workflows/pr-review-security-hosted.yml +++ b/.github/workflows/pr-review-security-hosted.yml @@ -206,6 +206,7 @@ jobs: .github/actions/request-lane-token/** .github/actions/check-kill-switch/** .github/actions/log-api-budget/rate-guard.mjs + .github/actions/log-api-budget/rate-limits.json .github/scripts/pr-merge/merge-holds.mjs .github/scripts/pr-merge/enable-auto-merge.mjs .github/scripts/pr-merge/superseded.mjs diff --git a/docs/conventions/pr-pipeline.yaml b/docs/conventions/pr-pipeline.yaml index ce6cbcd458..e72b97730d 100644 --- a/docs/conventions/pr-pipeline.yaml +++ b/docs/conventions/pr-pipeline.yaml @@ -787,14 +787,14 @@ limits: admission-min-per-hour: 1 # GitHub API margins per pool (`api:` with github-token, lanes-app, # graphql, owner-token, content-creation). limit-cap sets each pool's planned - # budget, the header's x-ratelimit-limit at most the cap (github-token: 1000, + # budget, the header's x-ratelimit-limit at most the cap (github-token: # GitHub's documented GITHUB_TOKEN limit); plan and read-floor are shares of # it, writer-pause and breaker shares of the header's limit; content-creation # adds per-minute and per-hour. The rate guard reads only read-floor and # limit-cap, to warn; the others are thresholds meant for # the hub's limits sampler, which nothing here enforces. Their - # defaults live in pr-pipeline.schema.json ($defs/api-pool, - # $defs/api-content-pool); add an `api:` entry here only to override one. + # defaults live in .github/actions/log-api-budget/rate-limits.json, held + # for a person like this file; add an `api:` entry here only to override one. wip: # Cap = lane PRs merged per hour over the window times this target. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index eed5df5d4c..1cacc2d007 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -792,7 +792,8 @@ and its queued entries leave; two different identities, or an unreadable head co unattended: `.github/actions/resolve-config/**`, whose code the token broker runs from main in its signer process, `.github/actions/request-lane-token/**`, which mints the lanes App token, `.github/actions/check-trusted-trigger/**` and the rate guard it sends every call through - (`.github/actions/log-api-budget/rate-guard.mjs`), `.github/actions/check-kill-switch/**`, + (`.github/actions/log-api-budget/rate-guard.mjs`) with its rate-limit defaults + (`.github/actions/log-api-budget/rate-limits.json`), `.github/actions/check-kill-switch/**`, whose output writer both actions use, `**/CODEOWNERS`, `**/.gitmodules`, and the files that enforce the floor: `.github/scripts/pr-merge/merge-holds.mjs`, `enable-auto-merge.mjs`, `superseded.mjs` and `clear-escalations.mjs` there, @@ -2172,7 +2173,7 @@ with defaults. | `fix-docs-per-run` | 2 | fix-docs dispatches one pr-unblock-dispatch run sends; at most 20. | | `admission-factor` | 0.8 | Share of the lane's hourly merge rate admitted per hour for the queued cap ([WIP cap](#wip-cap)); greater than 0, at most 1. | | `admission-min-per-hour` | 1 | Admissions per hour the queued cap never falls below, so a day with no lane merges cannot hold admissions shut; 0 to 100, and 0 removes the floor. | -| `api` | schema | GitHub API margins per pool (`github-token`, `lanes-app`, `graphql`, `owner-token`, `content-creation`). `limit-cap` sets the pool's planned budget: planned = min(header limit, `limit-cap`), used = header limit - header remaining, left = planned - used. `github-token` defaults it to 1000, GitHub's documented GITHUB_TOKEN limit, so a higher header limit is headroom, never a target; the other pools have no cap and plan to their header limit. `plan` and `read-floor` are shares of the planned budget; `writer-pause` and `breaker` are shares of the header's limit, by its remaining; `content-creation` adds `per-minute` and `per-hour`. Their defaults are in `pr-pipeline.schema.json` (`$defs/api-pool`, `$defs/api-content-pool`, and the `github-token` entry). The rate guard (`.github/actions/log-api-budget/rate-guard.mjs`, `planOf`) reads only `read-floor` and `limit-cap`, to warn once while calls continue, and stops only on the header's remaining and GitHub's refusals; the others are thresholds meant for the hub's limits sampler, and nothing in this repository enforces them. resolve-config passes the block through as set. | +| `api` | schema | GitHub API margins per pool (`github-token`, `lanes-app`, `graphql`, `owner-token`, `content-creation`). `limit-cap` sets the pool's planned budget: planned = min(header limit, `limit-cap`), used = header limit - header remaining, left = planned - used. `github-token` defaults it to GitHub's documented GITHUB_TOKEN limit, so a higher header limit is headroom, never a target; the other pools have no cap and plan to their header limit. `plan` and `read-floor` are shares of the planned budget; `writer-pause` and `breaker` are shares of the header's limit, by its remaining; `content-creation` adds `per-minute` and `per-hour`. Their defaults are in `.github/actions/log-api-budget/rate-limits.json`, the only copy, which every merge hold list floors, so a change to a default waits for a person. The rate guard (`.github/actions/log-api-budget/rate-guard.mjs`, `planOf`) reads only `read-floor` and `limit-cap`, to warn once while calls continue, and stops only on the header's remaining and GitHub's refusals; the others are thresholds meant for the hub's limits sampler, and nothing in this repository enforces them. resolve-config passes the block through as set. | report-outcome and the work-unblock sweep read them from resolve-config's output; pr-feedback-dispatch, pr-unblock-dispatch and work-resolve-issue's WIP cap check resolve the base diff --git a/docs/conventions/pr-pipeline/pr-pipeline.schema.json b/docs/conventions/pr-pipeline/pr-pipeline.schema.json index 66717d3ba7..81c1ccada8 100644 --- a/docs/conventions/pr-pipeline/pr-pipeline.schema.json +++ b/docs/conventions/pr-pipeline/pr-pipeline.schema.json @@ -160,12 +160,11 @@ "api": { "type": "object", "additionalProperties": false, - "description": "GitHub API margins per pool. The pool's planned budget is the limit its x-ratelimit-limit header reports, at most its limit-cap; plan and read-floor are shares of that planned budget, writer-pause and breaker shares of the header's limit. A graphql call reads `graphql`; any other call reads the entry its token names. Unset fields take the entry's own default, then the defaults in $defs/api-pool and $defs/api-content-pool.", + "description": "GitHub API margins per pool. The pool's planned budget is the limit its x-ratelimit-limit header reports, at most its limit-cap; plan and read-floor are shares of that planned budget, writer-pause and breaker shares of the header's limit. A graphql call reads `graphql`; any other call reads the entry its token names. Unset fields take their defaults from .github/actions/log-api-budget/rate-limits.json, the only copy, which every merge hold list floors so a change to one waits for a person.", "properties": { "github-token": { "$ref": "#/$defs/api-pool", - "default": { "limit-cap": 1000 }, - "description": "GITHUB_TOKEN. GitHub documents 1,000 requests per hour per repository for it (https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-github_token-in-github-actions), so the lanes plan to 1,000 and a higher header limit is headroom, never a target." + "description": "GITHUB_TOKEN. GitHub documents 1,000 requests per hour per repository for it (https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-github_token-in-github-actions), so its default limit-cap plans to that and a higher header limit is headroom, never a target." }, "lanes-app": { "$ref": "#/$defs/api-pool" }, "graphql": { "$ref": "#/$defs/api-pool" }, @@ -315,34 +314,30 @@ "type": "number", "exclusiveMinimum": 0, "maximum": 1, - "default": 0.7, "description": "Share of the pool's planned budget the lanes plan to spend. A threshold meant for the hub's limits sampler; the rate guard does not enforce it." }, "writer-pause": { "type": "number", "minimum": 0, "maximum": 1, - "default": 0.4, "description": "Share of the header's limit left, by the header's remaining, at which writers should pause. A threshold meant for the hub's limits sampler; the rate guard does not enforce it." }, "breaker": { "type": "number", "minimum": 0, "maximum": 1, - "default": 0.2, "description": "Share of the header's limit left, by the header's remaining, at which the repository kill switch should be set. A threshold meant for the hub's limits sampler; the rate guard does not enforce it." }, "read-floor": { "type": "number", "minimum": 0, "maximum": 1, - "default": 0.1, "description": "Share of the planned budget left below which the rate guard warns once per pool; calls continue." }, "limit-cap": { "type": "integer", "minimum": 1, - "description": "Requests per window the pool plans to, whatever higher limit its header reports: planned = min(header limit, limit-cap), used = header limit - header remaining, left = planned - used. Unset: planned is the header's limit. The rate guard's stop still reads only the header's remaining and GitHub's refusals." + "description": "Requests per window the pool plans to, whatever higher limit its header reports: planned = min(header limit, limit-cap), used = header limit - header remaining, left = planned - used. Unset in both the config and rate-limits.json: planned is the header's limit. The rate guard's stop still reads only the header's remaining and GitHub's refusals." } } }, @@ -359,13 +354,11 @@ "per-minute": { "type": "integer", "minimum": 1, - "default": 80, "description": "Content-creating requests GitHub allows per minute. A threshold meant for the hub's limits sampler; the rate guard does not enforce it." }, "per-hour": { "type": "integer", "minimum": 1, - "default": 500, "description": "Content-creating requests GitHub allows per hour. A threshold meant for the hub's limits sampler; the rate guard does not enforce it." } } diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 6b49f65d8c..0abede5193 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -325,7 +325,8 @@ def _send( # the process exits red naming the reset, with no retry and no wait. The # signals are the documented ones below; the shares of each pool's limit # (`writer-pause`, `read-floor`) are `limits.api` in pr-pipeline.yaml, unset -# fields filled from the schema's defaults, and an unreadable block or schema +# fields filled from .github/actions/log-api-budget/rate-limits.json (with +# github-token's `limit-cap`), and an unreadable block or defaults file # refuses every call. With no pr-pipeline.yaml at all (an installed plugin, a # consumer repo) there are no margins, and the signals alone still latch. `evaluate_rate_response` is the shared decision table's # port: .github/actions/log-api-budget/rate-guard.vectors.json tests it here @@ -371,23 +372,15 @@ def _send( _REPO_ROOT = _PARENTS[5] if len(_PARENTS) > 5 else None _CONVENTIONS = None if _REPO_ROOT is None else _REPO_ROOT / "docs" / "conventions" LIMITS_CONFIG = None if _CONVENTIONS is None else _CONVENTIONS / "pr-pipeline.yaml" -LIMITS_SCHEMA = ( +LIMITS_DEFAULTS = ( None - if _CONVENTIONS is None - else _CONVENTIONS / "pr-pipeline" / "pr-pipeline.schema.json" + if _REPO_ROOT is None + else _REPO_ROOT / ".github" / "actions" / "log-api-budget" / "rate-limits.json" ) LIMITS_FIELDS = ("writer-pause", "read-floor") # The shares with no repo config: no margin, so only a signal stops a call. NO_MARGINS = dict.fromkeys(LIMITS_FIELDS, 0.0) TOKEN_POOLS = frozenset({"github-token", "lanes-app", "owner-token"}) -# GITHUB_TOKEN's documented ceiling, whatever x-ratelimit-limit reports: "The -# rate limit for GITHUB_TOKEN is 1,000 requests per hour per repository." The -# read floor counts against min(header limit, this); the writer pause and the -# exhausted stop keep the header's own numbers. -# Pointer: -# As-of: 2026-10-25. -# Recheck trigger: that page changes the GITHUB_TOKEN figure. -GITHUB_TOKEN_LIMIT_CAP = 1000 # The YAML reader's runner, bound at import: a test that fakes `gh` through # `run_command` still reads the real limits. _reader_run = run_command @@ -652,8 +645,8 @@ def _limits_pool(args: list[str], env_overrides: Mapping[str, str] | None) -> st def pool_limits(pool: str) -> dict[str, float] | str: - """`limits.api.`'s shares, unset ones from the schema's defaults, or - why they cannot be read. Read through the pipeline config's own reader; + """`limits.api.`'s shares, unset ones from rate-limits.json, with the + pool's default `limit-cap` when it has one, or why they cannot be read. Read through the pipeline config's own reader; only a read that succeeded is kept, so a failure is read again next call.""" limits = _limits.get(pool) or _read_pool_limits(pool) if not isinstance(limits, str): @@ -662,27 +655,25 @@ def pool_limits(pool: str) -> dict[str, float] | str: def warm_rate_guard() -> None: - """Read every `limits.api` pool the schema names now, before a test fakes - the subprocess seam the reader runs through.""" - if LIMITS_SCHEMA is None: + """Read every `limits.api` pool rate-limits.json names now, before a test + fakes the subprocess seam the reader runs through.""" + if LIMITS_DEFAULTS is None: return try: - schema = json.loads(LIMITS_SCHEMA.read_text(encoding="utf-8")) + defaults = json.loads(LIMITS_DEFAULTS.read_text(encoding="utf-8")) except (OSError, ValueError): return - pools = dig(schema, "properties", "limits", "properties", "api", "properties") - for pool in json_object(pools): + for pool in json_object(defaults): pool_limits(str(pool)) def _read_pool_limits(pool: str) -> dict[str, float] | str: import babysit_merge_policy as merge_policy # noqa: PLC0415 - imports this module - if LIMITS_CONFIG is None or LIMITS_SCHEMA is None or not LIMITS_CONFIG.exists(): + if LIMITS_CONFIG is None or LIMITS_DEFAULTS is None or not LIMITS_CONFIG.exists(): return dict(NO_MARGINS) try: - schema = json.loads(LIMITS_SCHEMA.read_text(encoding="utf-8")) - fields = dig(schema, "$defs", "api-pool", "properties") + defaults = json.loads(LIMITS_DEFAULTS.read_text(encoding="utf-8")) text = LIMITS_CONFIG.read_text(encoding="utf-8") keys = [(f"limits.api.{pool}.{name}", False) for name in LIMITS_FIELDS] values = merge_policy._concern_values(text, keys, _reader_run) @@ -692,14 +683,22 @@ def _read_pool_limits(pool: str) -> dict[str, float] | str: return f"{LIMITS_CONFIG.name} does not parse" shares: dict[str, float] = {} for name, (key, _) in zip(LIMITS_FIELDS, keys, strict=True): - raw = values[key][0] if values[key] else dig(fields, name, "default") + # The config reader gives text; the defaults file must hold a number. + raw = values[key][0] if values[key] else dig(defaults, pool, name) try: share = float(raw) except (TypeError, ValueError): share = math.nan - if isinstance(raw, bool) or not 0 <= share <= 1: + typed = bool(values[key]) or isinstance(raw, (int, float)) + if isinstance(raw, bool) or not typed or not 0 <= share <= 1: return f"limits.api.{pool}.{name} is not a share: {raw!r}" shares[name] = share + # The default cap alone, as before any config could set one. + cap = dig(defaults, pool, "limit-cap") + if cap is not None: + if not isinstance(cap, int) or isinstance(cap, bool) or cap < 1: + return f"{LIMITS_DEFAULTS.name}: {pool}.limit-cap is not a count: {cap!r}" + shares["limit-cap"] = cap return shares @@ -807,7 +806,7 @@ def _below(pool: str | None, field: str) -> bool: if not reading: continue remaining, limit = reading - cap = _limit_cap(name) if field == "read-floor" else None + cap = limits.get("limit-cap") if field == "read-floor" else None if cap is not None and cap < limit: remaining, limit = max(0, cap - (limit - remaining)), cap if remaining < limits[field] * limit: @@ -815,15 +814,10 @@ def _below(pool: str | None, field: str) -> bool: return False -def _limit_cap(pool: str) -> int | None: - """The absolute ceiling under a pool's header limit, or None.""" - return GITHUB_TOKEN_LIMIT_CAP if pool == "github-token" else None - - def low_headroom(pool: str | None = None) -> bool: """Whether `pool` (any pool read so far, when None) last read below its - `read-floor` share (for `github-token`, of `GITHUB_TOKEN_LIMIT_CAP` when - the header reports more), or its limits are unreadable. Report-only reads + `read-floor` share (of the pool's default `limit-cap` in rate-limits.json + when the header reports more), or its limits are unreadable. Report-only reads are skipped then.""" return _below(pool, "read-floor") diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py index a0a9583129..f2796bb0a3 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py @@ -206,6 +206,7 @@ class RiskResult: ".github/actions/check-kill-switch/**", ".github/actions/request-lane-token/**", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/scripts/pr-merge/merge-holds.mjs", ".github/scripts/pr-merge/enable-auto-merge.mjs", ".github/scripts/pr-merge/superseded.mjs", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_rate_guard.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_rate_guard.py index 4c44dd91e1..06d97a9213 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_rate_guard.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_rate_guard.py @@ -3,7 +3,8 @@ Expected values come from two sources the code does not compute: the shared decision vectors (.github/actions/log-api-budget/rate-guard.vectors.json, run by the JavaScript client too) and the `limits.api` shares in -docs/conventions/pr-pipeline.yaml with their schema defaults. Canned gh output +docs/conventions/pr-pipeline.yaml with their defaults in +.github/actions/log-api-budget/rate-limits.json. Canned gh output follows GitHub's documented signals and `gh api -i`'s observed shape. """ @@ -32,12 +33,11 @@ VECTORS = ( REPO_ROOT / ".github" / "actions" / "log-api-budget" / "rate-guard.vectors.json" ) -SCHEMA = REPO_ROOT / "docs" / "conventions" / "pr-pipeline" / "pr-pipeline.schema.json" +RATE_LIMITS = REPO_ROOT / ".github" / "actions" / "log-api-budget" / "rate-limits.json" -def schema_default(field: str) -> float: - schema = json.loads(SCHEMA.read_text(encoding="utf-8")) - return schema["$defs"]["api-pool"]["properties"][field]["default"] +def file_default(field: str, pool: str = "lanes-app") -> float: + return json.loads(RATE_LIMITS.read_text(encoding="utf-8"))[pool][field] def head(status: int, **headers: str) -> str: @@ -59,7 +59,7 @@ def core(remaining: int, limit: int = 5000, **extra: str) -> dict[str, str]: class SharedVectors(unittest.TestCase): def test_every_vector_in_the_shared_file(self) -> None: table = json.loads(VECTORS.read_text(encoding="utf-8")) - floor = schema_default("read-floor") + floor = file_default("read-floor") self.assertGreaterEqual(len(table["vectors"]), 26) for vector in table["vectors"]: with self.subTest(vector["name"]): @@ -279,7 +279,7 @@ def shares(self, pool: str) -> dict[str, float]: assert not isinstance(limits, str), limits return limits - def test_a_set_share_is_read_and_an_unset_one_is_the_schema_default(self) -> None: + def test_a_set_share_is_read_and_an_unset_one_is_the_file_default(self) -> None: with tempfile.TemporaryDirectory() as tmp: config = pathlib.Path(tmp, "pr-pipeline.yaml") config.write_text( @@ -289,7 +289,7 @@ def test_a_set_share_is_read_and_an_unset_one_is_the_schema_default(self) -> Non with mock.patch.object(gh, "LIMITS_CONFIG", config): shares = self.shares("lanes-app") self.assertEqual(shares["writer-pause"], 0.3) - self.assertEqual(shares["read-floor"], schema_default("read-floor")) + self.assertEqual(shares["read-floor"], file_default("read-floor")) def test_below_the_read_floor_report_only_reads_are_skipped(self) -> None: floor = self.shares("github-token")["read-floor"] @@ -329,8 +329,23 @@ def test_github_token_read_floor_counts_against_the_per_repo_cap(self) -> None: # The writer pause still reads the header's own numbers. self.assertFalse(gh.writes_paused("github-token")) + def test_the_github_token_cap_is_read_from_the_defaults_file(self) -> None: + defaults = json.loads(RATE_LIMITS.read_text(encoding="utf-8")) + defaults["github-token"]["limit-cap"] = 2000 + floor = self.shares("github-token")["read-floor"] + # Under the file's 1000 cap this reads low; under 2000 it does not. + remaining = 5000 - 1000 + int(floor * 1000) - 1 + with tempfile.TemporaryDirectory() as tmp: + path = pathlib.Path(tmp, "rate-limits.json") + path.write_text(json.dumps(defaults), encoding="utf-8") + with mock.patch.object(gh, "LIMITS_DEFAULTS", path): + gh.reset_rate_guard() + self.answer(0, head(200, **core(remaining)) + "{}") + gh.gh_capture(["api", "repos/o/r/pulls/1"]) + self.assertFalse(gh.low_headroom("github-token")) + def test_the_cap_is_github_token_only(self) -> None: - cap = 1000 + cap = file_default("limit-cap", "github-token") floor = self.shares("lanes-app")["read-floor"] os.environ["API_BUDGET_TOKEN"] = "lanes-app" self.answer(0, head(200, **core(5000 - cap + int(floor * cap) - 1)) + "{}") @@ -374,7 +389,7 @@ def test_the_module_imports_with_no_repo_root_and_no_margins(self) -> None: sys.modules["babysit_gh"] = module path = {str(source)!r} exec(compile(open(path, encoding="utf-8").read(), module.__file__, "exec"), module.__dict__) - print(module.LIMITS_CONFIG, module.LIMITS_SCHEMA) + print(module.LIMITS_CONFIG, module.LIMITS_DEFAULTS) module.warm_rate_guard() print(module.pool_limits("core") == module.NO_MARGINS) """ @@ -391,11 +406,12 @@ def test_the_module_imports_with_no_repo_root_and_no_margins(self) -> None: class NoRepoConfig(GuardCase): - """The installed-plugin layout: no pr-pipeline.yaml or schema beside it.""" + """The installed-plugin layout: no pr-pipeline.yaml or rate-limits.json + beside it.""" def setUp(self) -> None: super().setUp() - for name in ("LIMITS_CONFIG", "LIMITS_SCHEMA"): + for name in ("LIMITS_CONFIG", "LIMITS_DEFAULTS"): patcher = mock.patch.object(gh, name, REPO_ROOT / f"no-such-{name}") patcher.start() self.addCleanup(patcher.stop) @@ -465,10 +481,42 @@ def no_bash( self.assertIn("fail closed", str(raised.exception)) self.assertEqual(self.sent, []) - def test_an_unreadable_schema_sends_nothing(self) -> None: - with mock.patch.object(gh, "LIMITS_SCHEMA", REPO_ROOT / "no-such-schema.json"): - with self.assertRaises(gh.RateLimitExhausted): + def test_missing_rate_limit_defaults_send_nothing(self) -> None: + with mock.patch.object( + gh, "LIMITS_DEFAULTS", REPO_ROOT / "no-such-rate-limits.json" + ): + with self.assertRaises(gh.RateLimitExhausted) as raised: gh.gh_capture(["api", "repos/o/r/pulls/1"]) + self.assertIn("fail closed", str(raised.exception)) + self.assertEqual(self.sent, []) + + def test_invalid_rate_limit_defaults_send_nothing(self) -> None: + valid = json.loads(RATE_LIMITS.read_text(encoding="utf-8")) + token = valid["github-token"] + cases = { + "not JSON": "{", + "the pool missing": json.dumps({"lanes-app": valid["lanes-app"]}), + "a share missing": json.dumps( + {"github-token": {k: v for k, v in token.items() if k != "read-floor"}} + ), + "a share past 1": json.dumps({"github-token": token | {"writer-pause": 2}}), + "a share as text": json.dumps( + {"github-token": token | {"read-floor": "0.1"}} + ), + "a fractional limit-cap": json.dumps( + {"github-token": token | {"limit-cap": 1.5}} + ), + "a limit-cap of 0": json.dumps({"github-token": token | {"limit-cap": 0}}), + } + for name, text in cases.items(): + with self.subTest(name), tempfile.TemporaryDirectory() as tmp: + gh.reset_rate_guard() + path = pathlib.Path(tmp, "rate-limits.json") + path.write_text(text, encoding="utf-8") + with mock.patch.object(gh, "LIMITS_DEFAULTS", path): + with self.assertRaises(gh.RateLimitExhausted) as raised: + gh.gh_capture(["api", "repos/o/r/pulls/1"]) + self.assertIn("fail closed", str(raised.exception)) self.assertEqual(self.sent, []) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py index a4ee86348e..4249f643c3 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_risk.py @@ -285,6 +285,7 @@ def test_floor_paths_are_high_whatever_the_switch(self): ".github/actions/check-kill-switch/**", ".github/actions/request-lane-token/**", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/scripts/pr-merge/merge-holds.mjs", ".github/scripts/pr-merge/enable-auto-merge.mjs", ".github/scripts/pr-merge/superseded.mjs", @@ -326,6 +327,8 @@ def test_floor_paths_are_high_whatever_the_switch(self): (".github/actions/check-kill-switch/check-kill-switch.mjs", ".github/actions/check-kill-switch/**"), (".github/actions/request-lane-token/action.yml", ".github/actions/request-lane-token/**"), (".github/actions/log-api-budget/rate-guard.mjs", ".github/actions/log-api-budget/rate-guard.mjs"), + (".github/actions/log-api-budget/rate-limits.json", ".github/actions/log-api-budget/rate-limits.json"), + (".github/actions/log-api-budget/Rate-Limits.JSON", ".github/actions/log-api-budget/rate-limits.json"), (".github/scripts/pr-merge/merge-holds.mjs", ".github/scripts/pr-merge/merge-holds.mjs"), (".github/scripts/pr-merge/enable-auto-merge.mjs", ".github/scripts/pr-merge/enable-auto-merge.mjs"), (".github/scripts/pr-merge/superseded.mjs", ".github/scripts/pr-merge/superseded.mjs"), @@ -1180,6 +1183,7 @@ def test_the_floor_holds_with_the_switch_off(self): ".github/actions/check-kill-switch/action.yml", ".github/actions/request-lane-token/action.yml", ".github/actions/log-api-budget/rate-guard.mjs", + ".github/actions/log-api-budget/rate-limits.json", ".github/scripts/pr-merge/merge-holds.mjs", ".github/scripts/pr-merge/enable-auto-merge.mjs", ".github/scripts/pr-merge/superseded.mjs", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index be4e813b2c..548ca8de20 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -59,7 +59,12 @@ ".github/scripts/pr-merge/superseded.mjs", ".github/scripts/pr-merge/clear-escalations.mjs", ) -IMPORT = re.compile(r"""(?:from|import)\s*\(?\s*["'](\.{1,2}/[^"']+)["']""") +# A relative import, or a module-relative file a module reads at load +# (`new URL("./x.json", import.meta.url)`): data such as a routing table +# decides as much as the code that reads it. +IMPORT = re.compile( + r"""(?:(?:from|import)\s*\(?|new\s+URL\()\s*["'](\.{1,2}/[^"']+)["']""" +) def import_closure(entries: tuple[str, ...]) -> list[str]: @@ -157,6 +162,8 @@ def python_closure(js_files: list[str]) -> list[str]: ".github/actions/log-api-budget/Rate-Guard.MJS", ".github/actions/log-api-budget/action.yml", ".github/actions/log-api-budget/rate-guard.test.mjs", + ".github/actions/log-api-budget/rate-limits.json", + ".github/actions/log-api-budget/Rate-Limits.JSON", ".github/CODEOWNERS", ".github/codeowners", "CODEOWNERS", @@ -202,7 +209,7 @@ def test_the_cost_paths_are_the_same(self) -> None: def test_the_floor_is_the_same_on_both_sides(self) -> None: answer = module(FLOOR_PATHS) self.assertEqual(answer["floor"], list(risk.HIGH_FLOOR)) - self.assertEqual(len(risk.HIGH_FLOOR), 37) + self.assertEqual(len(risk.HIGH_FLOOR), 38) for path, held, configured in zip( FLOOR_PATHS, answer["held"], answer["configured"], strict=True ): @@ -222,6 +229,9 @@ def test_the_floor_holds_what_its_actions_load(self) -> None: files = import_closure(FLOOR_ENTRIES) self.assertIn(".github/actions/check-kill-switch/check-kill-switch.mjs", files) self.assertIn(".github/actions/log-api-budget/rate-guard.mjs", files) + # The rate-limit defaults rate-guard.mjs reads decide when the lanes + # stop short of GitHub's limits, so they are held like the module. + self.assertIn(".github/actions/log-api-budget/rate-limits.json", files) answer = module(files) for path, held in zip(files, answer["held"], strict=True): with self.subTest(path): diff --git a/scripts/ci-lanes-pr-hold.test.sh b/scripts/ci-lanes-pr-hold.test.sh index 449eb1d6d0..83d80876e3 100755 --- a/scripts/ci-lanes-pr-hold.test.sh +++ b/scripts/ci-lanes-pr-hold.test.sh @@ -299,6 +299,7 @@ expect "a dot-prefixed file under a denied tree fails" 1 3 "it changes docs/held for path in CODEOWNERS docs/CODEOWNERS .gitmodules .github/actions/resolve-config/resolve.mjs \ .github/actions/check-trusted-trigger/.x.mjs .github/actions/request-lane-token/action.yml \ .github/actions/check-kill-switch/check-kill-switch.mjs .github/actions/log-api-budget/rate-guard.mjs \ + .github/actions/log-api-budget/rate-limits.json \ .github/scripts/pr-merge/merge-holds.mjs .github/scripts/pr-merge/enable-auto-merge.mjs \ .github/scripts/pr-merge/superseded.mjs .github/scripts/pr-merge/clear-escalations.mjs \ plugins/source-control/skills/babysit-prs/scripts/babysit_risk.py .github/workflows/pr-require-checks.yml \