Repository navigation
defaults guard: a shared helper name is no tunable source (6182 -> 449); an unchecked BLOCK still counts - #33161
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).
…nchecked 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).
…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.
|
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)
web and size checks: green Default changesThis pull request changes 1 default at
Signed off on this commit by Merge receipt: merged as Regenerate the snapshot with Shipped in:
|
|
Found 1 test failure on Blacksmith runners: Failure
|
…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.
…alls 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).
|
Review (independent-subagent, correctness first): SHIP at 3a0a2e2
|
…s-guard-tunable-helpers # Conflicts: # scripts/ci/defaults_guard.py # tests/test_defaults_guard.py
|
Review (independent-subagent, correctness first): SHIP at 65e1e63
|
…oes not see (#33321) #33161 matches a Tunable helper by the call that can reach it. Its review found five forms that this match misses and the old name match caught. No helper in the tree is called in these ways today. The lead decided to leave them and to write them down where the match is. Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
Conflicts kept both sides: Package.swift deps (ChiefMemory and Icons), Chief BUILTIN skills (10 entries), PageFactory routes (registry and the new panels route), AppsClient state (changedFiles and eventListeners), manifest fixture tests, and production wrangler vars with the app registry left unbound. defaults_guard.py takes base's type-aware helper matching (#33161), which supersedes this branch's name-only fix; the three tests that asserted the old conservative name matching are dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Follow-up to #33111. This branch contains the two commits of #33111. After #33111 lands, the diff here is only the last two commits.
1. A shared name is not a call of a Tunable helper.
toggle,makeandpointsare Tunable helpers. The old guard selected any declaration with a token of that name. For example,registry.bind("toggleSidebar") { ...model.toggle() }inAppActions.bindAppwas selected, and the dependency walk then pulled in every handler'sstatic func bind. Thus almost every app PR "changed a default". #32983 (OnboardingHandlers.swift:bind) and #32979 (NewTabPage.swift:paletteActionsthroughHistoryHandlers.bind's closuremake) were flagged only because of this.Now a helper call counts only in two forms:
Type.helper, whereTypeis declared in the helper's filehelper, in a file that declares one of the types of the helper's file (the same type or an extension of it)x.helperand.helperare other members. The dependency walk is not changed.Fingerprints on feat-cmux-next 38369c1: 6182 before, 449 after. No entry is added. All real helper callers stay, for example
ChromeTunables.swift:space1,TabTunables.swift:compactTabWidth,MetricTunables.swift:sidebarWidthand everyDropOverlayTunablesentry. The bot-owned snapshotschemas/defaults/cmux-next.jsonis regenerated by base autofix; this PR does not edit it.Results on the real diffs (
defaults_guard.py check --base <merge base> --head <pr head>):tunable-source.CmuxNextApp/Handlers/OnboardingHandlers.swift:bindNewTabPage+Open.swift:fillPaletteActions,NewTabPage.swift:paletteActions2. Review findings from #33111
Regression command:
python3 tests/test_defaults_guard.pytest_a_member_that_only_shares_a_helpers_name_is_not_a_tunable_sourcereproduces thebindApp/bind/makeshapes.test_an_unchecked_block_after_an_owners_ship_keeps_the_guard_redtest_an_unchecked_receipt_one_base_merge_back_is_named_in_the_errortest_a_receipt_whose_lookup_failed_is_named_in_the_error, for its new wording.Factory.number, symbol, external symbol, dynamic, backticked) still fail without a review.test_a_helper_called_from_an_extension_or_by_its_type_is_still_a_tunable_sourcecovers an extension caller and aType.helpercaller in another module.Observed / Not run
Observed:
scripts/cmux-next/prepush-gate.pyis clean.Not run: a live
pull_request_targetrun. The guard runs 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 matching any token that shares a Tunable helper's name (
toggle,make,points) as a helper call, which flagged nearly every app PR as changing a default; a helper now counts only for calls that can actually reach it, includingSelf.helper, typealiases of Tunable, and subclass or protocol-inherited helpers. This drops the fingerprint count onfeat-cmux-nextfrom 6182 to 449, and #32983 and #32979 no longer report default changes. Also makes an unchecked receipt whose author's write-access lookup fails count for a BLOCK only, so it can keep the guard red but never green.T.points) are still not matched.Written for commit ef33467. Summary will update on new commits.