defaults guard: a shared helper name is no tunable source (6182 -> 449); an unchecked BLOCK still counts

Merged
LA
3d ago
+234-24

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, make and points are Tunable helpers. The old guard selected any declaration with a token of that name. For example, registry.bind("toggleSidebar") { ...model.toggle() } in AppActions.bindApp was selected, and the dependency walk then pulled in every handler's static func bind. Thus almost every app PR "changed a default". #32983 (OnboardingHandlers.swift:bind) and #32979 (NewTabPage.swift:paletteActions through HistoryHandlers.bind's closure make) were flagged only because of this.

Now a helper call counts only in two forms:

  • Type.helper, where Type is declared in the helper's file
  • a bare helper, in a file that declares one of the types of the helper's file (the same type or an extension of it)

x.helper and .helper are other members. The dependency walk is not changed.

Fingerprints on feat-cmux-next 38369c1a886: 6182 before, 449 after. No entry is added. All real helper callers stay, for example ChromeTunables.swift:space1, TabTunables.swift:compactTabWidth, MetricTunables.swift:sidebarWidth and every DropOverlayTunables entry. The bot-owned snapshot schemas/defaults/cmux-next.json is 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>):

PRold guardthis guard
#32983tunable-source.CmuxNextApp/Handlers/OnboardingHandlers.swift:bindno default change
#32979NewTabPage+Open.swift:fillPaletteActions, NewTabPage.swift:paletteActionsno default change

2. Review findings from #33111

  • If the write-access lookup for a receipt author fails, that author now counts for a BLOCK, never for a SHIP. Thus an unchecked receipt can only keep the guard red. Before, the receipt was dropped, so an unchecked BLOCK lost to an owner's earlier SHIP.
  • If a lookup fails at the earlier head of a carried SHIP, the guard now names it in the error, as it does at the head. Before, it gave only a warning.

Regression command: python3 tests/test_defaults_guard.py

  • Test commit 9094a6552df: 4 failures.
    • test_a_member_that_only_shares_a_helpers_name_is_not_a_tunable_source reproduces the bindApp/bind/make shapes.
    • 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
    • test_a_receipt_whose_lookup_failed_is_named_in_the_error, for its new wording.
  • Head: 72 tests OK.
  • Real default changes are still caught:
    • The existing fixtures (helper, helper body, cross-file Factory.number, symbol, external symbol, dynamic, backticked) still fail without a review.
    • The new test_a_helper_called_from_an_extension_or_by_its_type_is_still_a_tunable_source covers an extension caller and a Type.helper caller in another module.
    • The name-collision test also checks that a real helper default change is still caught.

Observed / Not run

Observed:

  • The fingerprint counts before and after on the feat tree.
  • Both PR diffs, run through the old and the new guard.
  • The test red and green results above.
  • scripts/cmux-next/prepush-gate.py is clean.

Not run: a live pull_request_target run. The guard runs from the PR base, so this change is first used by PRs whose base contains it.

Changelog

none


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with 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, including Self.helper, typealiases of Tunable, and subclass or protocol-inherited helpers. This drops the fingerprint count on feat-cmux-next from 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.

  • A call owner's receipt is trusted without the write-access REST lookup, which was exhausting the shared API quota (#32983).
  • Failed lookups are now named in the error, including at the earlier head of a carried SHIP, whose author's SHIP no longer counts but whose BLOCK does.
  • Calls through a generic parameter (e.g., T.points) are still not matched.