Repository navigation
defaults guard: a call owner's SHIP counts when the REST quota is spent (#32983) - #33111
Merged
Merged
Conversation
…is spent (failing, #32983) A private org member reads as CONTRIBUTOR to the workflow's token. The guard then asks the REST API for their write access. On #32983 that call got "API rate limit exceeded for installation", so the guard trusted nobody and stayed red on a valid SHIP from lawrencecchen, a call owner. Red on the base: python3 tests/test_defaults_guard.py fails test_a_call_owners_receipt_ships_with_the_rest_quota_spent and test_a_receipt_whose_lookup_failed_is_named_in_the_error.
…okup (#32983) A private org member reads as CONTRIBUTOR to the workflow's token, so the guard asked the REST API for each receipt author's write access. That call spends the installation's REST quota, which other jobs share. On #32983 the quota was spent ("API rate limit exceeded for installation"), the lookup trusted nobody, and the guard stayed red on lawrencecchen's valid SHIP; the merge service left the PR out as "defaults guard red". The call owners come from the trusted base's .github/defaults-call-owners.json, the same file that lets them approve a default outright, so their receipts now need no lookup. Any other member is still looked up; when that lookup fails for a reason other than 404, the error now names the receipt that was not counted and says to re-run when the quota resets. python3 tests/test_defaults_guard.py: red on the base (2 failures: test_a_call_owners_receipt_ships_with_the_rest_quota_spent, test_a_receipt_whose_lookup_failed_is_named_in_the_error), green on this commit (67 tests OK).
Contributor
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on e8b1b7d to compare; do not merge until green; see lane-rules Default changesThis pull request changes 1 default at
Signed off on this commit by Merge receipt: merged as Regenerate the snapshot with Shipped in:
|
Contributor
Author
|
Review (independent-subagent, correctness first): SHIP at e0d7252
|
lawrencecchen
added
the
ready-to-land
|
Found 6 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
lawrencecchen
added a commit
that referenced
this pull request
Nov 3, 2026
…9); an unchecked BLOCK still counts (#33161) * test(defaults guard): a call owner's SHIP counts when the REST quota is spent (failing, #32983) A private org member reads as CONTRIBUTOR to the workflow's token. The guard then asks the REST API for their write access. On #32983 that call got "API rate limit exceeded for installation", so the guard trusted nobody and stayed red on a valid SHIP from lawrencecchen, a call owner. Red on the base: python3 tests/test_defaults_guard.py fails test_a_call_owners_receipt_ships_with_the_rest_quota_spent and test_a_receipt_whose_lookup_failed_is_named_in_the_error. * fix(defaults guard): trust a call owner's receipt without the REST lookup (#32983) A private org member reads as CONTRIBUTOR to the workflow's token, so the guard asked the REST API for each receipt author's write access. That call spends the installation's REST quota, which other jobs share. On #32983 the quota was spent ("API rate limit exceeded for installation"), the lookup trusted nobody, and the guard stayed red on lawrencecchen's valid SHIP; the merge service left the PR out as "defaults guard red". The call owners come from the trusted base's .github/defaults-call-owners.json, the same file that lets them approve a default outright, so their receipts now need no lookup. Any other member is still looked up; when that lookup fails for a reason other than 404, the error now names the receipt that was not counted and says to re-run when the quota resets. python3 tests/test_defaults_guard.py: red on the base (2 failures: test_a_call_owners_receipt_ships_with_the_rest_quota_spent, test_a_receipt_whose_lookup_failed_is_named_in_the_error), green on this commit (67 tests OK). * test(defaults guard): a shared helper name is no tunable source; an unchecked BLOCK still counts (failing) #32983 and #32979 were flagged only because `toggle` and `make` are Tunable helpers: any token with that name selected its declaration, and through AppActions.bindApp every handler's `bind` (6182 tunable-source entries). Two findings from the review of #33111: a BLOCK whose author's lookup failed was dropped, so an owner's earlier SHIP passed; and a failed lookup at the earlier head of a carried SHIP gave only a warning. python3 tests/test_defaults_guard.py on this commit: 4 failures (test_a_member_that_only_shares_a_helpers_name_is_not_a_tunable_source, test_an_unchecked_block_after_an_owners_ship_keeps_the_guard_red, test_an_unchecked_receipt_one_base_merge_back_is_named_in_the_error, and test_a_receipt_whose_lookup_failed_is_named_in_the_error for its new wording). * fix(defaults guard): match a Tunable helper by the call that reaches it; an unchecked BLOCK still counts A Tunable helper (points, make, toggle) selected every declaration that had a token of that name, so `registry.bind { model.toggle() }` in AppActions.bindApp, and through it every handler's `bind`, was a tunable-source default. That is 6182 fingerprints on feat-cmux-next. Now a helper counts only as `Type.helper` where the helper's file declares Type, or as a bare `helper` in a file that declares one of the helper file's types (the same type or an extension of it). `x.helper` and `.helper` are other members. The dependency walk does not change. On feat-cmux-next (38369c1) the count goes from 6182 to 449, and no fingerprint is added. On the diffs of #32983 and #32979 the old guard reports 1 and 2 tunable-source changes, and this one reports none. Two findings from the review of #33111: a user whose write-access lookup failed now counts for a BLOCK (never for a SHIP), so an unchecked receipt can only keep the guard red. A failed lookup at the earlier head of a carried SHIP is now named in the error too. python3 tests/test_defaults_guard.py: 4 failures on the test commit, 72 tests OK on this one. The real-default fixtures (helper, helper body, cross-file Factory.number, symbol, external symbol, dynamic) still fail without a review. * test(defaults guard): Self., typealias, subclass and protocol helper calls stay tunable sources (failing) The review of #33161 found forms that reach a Tunable or its helper with no Tunable token and no Type.helper of a type in the helper's file: Self.points(...), a typealias of Tunable (TD.number, `: TD = .number`), a subclass calling an inherited helper (bare or Sub.helper), and a type calling a helper of a protocol extension it conforms to. On this commit all 6 subtests fail. On the guard before #33161, the 4 Self, subclass and protocol subtests pass and the 2 typealias subtests fail: that guard missed a typealias too. * fix(defaults guard): Self., typealias, subclass and protocol helper calls reach a Tunable helper The review of #33161 found Tunable forms the exact helper match missed. Self.points(...) was skipped because Self is no declared type, and the guard before #33161 caught it only by the shared-name match. Now: - Self.helper resolves like a bare helper: the caller's own types. - A caller's types also include the superclasses and protocols its types name, so a subclass reaches its base's helper and a conforming type a protocol extension's helper. Only the caller side takes inheritance, so a helper file that conforms to Sendable does not reach every Sendable type. - A typealias of a Tunable, or an alias of such an alias, counts as a Tunable token (TD.number, `: TD = .number`). The guard before #33161 missed this too. - `class func` / `class var` no longer read as types named func / var. feat-cmux-next tree: still 449 fingerprints, the same set, all of them in the old 6182. #32983 and #32979 still report no tunable-source change. python3 tests/test_defaults_guard.py: 6 subtests red on the test commit, 73 tests OK here. Not resolved: a helper reached through a generic parameter (T.points). --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
defaults guardstayed red on #32983 with a valid receipt (Review (independent-subagent): SHIP at 8af92c61...by lawrencecchen). The merge service then left the PR out as "defaults guard red".Cause: lawrencecchen's org membership is private. The workflow's token sees a private member as
CONTRIBUTOR. The guard then asks the REST API (collaborators/<login>/permission) for write access. That call uses the installation's REST quota, and other jobs use the same quota. In run 41576329148 the call gotAPI rate limit exceeded for installation. A failed lookup trusts nobody, so the guard rejected the receipt. The error message gave no reason ("needs a comment or review whose first line is ..."). It was not an old guard version:scripts/ci/defaults_guard.pyis the same at the PR base c2933eb and at feat tip.Fix:
.github/defaults-call-owners.jsonis trusted without the REST lookup. The guard reads that file from the trusted base, and the same file already lets these owners approve a default directly.Regression command:
python3 tests/test_defaults_guard.pytest_a_call_owners_receipt_ships_with_the_rest_quota_spentreproduces Chief: import browser cookies from Chrome on request #32983 (owner receipt, CONTRIBUTOR association, rate-limited lookup) and gives the same warning and error as the CI log.test_a_receipt_whose_lookup_failed_is_named_in_the_erroralso fails.Changes to existing tests: the private-member fixtures used
austinywang, who is a call owner. They now use a non-owner login, so they still test the lookup path. The "outsider" CONTRIBUTOR receipt now uses the loginoutsider, not the call ownerteamleaderleo.Observed / Not run
Observed:
::warning ... lawrencecchen: gh: API rate limit exceeded for installation, followed by the "needs a review" error.orgs/manaflow-ai/public_members/lawrencecchenreturns 404 (the membership is private).('ship', 'review (independent-subagent)'), with no lookup.python3 tests/test_defaults_guard.pyred then green as above;scripts/cmux-next/prepush-gate.pyclean.Not run: a live
pull_request_targetrun of this guard. The workflow checks out the guard from the PR base, so this change is first used by PRs whose base contains it.Changelog
none
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes the defaults guard so a call owner's SHIP receipt counts even when the GitHub REST quota is spent (#32983), and errors now name the receipt when a lookup fails for any reason other than 404.
Written for commit e0d7252. Summary will update on new commits.