Repository navigation
fix(guards): the router sweep was wrong four ways, and three guards swept at IMPORT (#16187, #16188) - #16189
Conversation
…1s to everyone (#16188) Measured: importing the full guard set took 67s, of which 63.6s was three modules doing work at module scope before a single assertion ran. no_duplicate_dict_keys_test 19.5s _sweep() parses 37,561 dict literals + _trees_holding_python() walks the tree man_page_indexer_path_resolves 24.5s files_naming_the_indexer() sweep model_pricing_tables_agree 11.4s discover_pricing_tables() + cross-index Now lazy behind cached accessors. Import: 67s -> 5.9s. The sweeps still run, in the tests that read them, and are cached so a module's several tests pay once. WHY IT MATTERED BEYOND TIDINESS. reach_declarations_test imports every guard module to enumerate declare() calls, so ANY change to repo_tests/_reach.py paid all 67s before running one assertion — against a 128s pre-push budget. Three pushes of the #15928 mechanism timed out with DID NOT RUN, and I attributed it to the budget scaling with changed-file count (#16182). That was half right. #16182 is real and unchanged: a 2-file changeset still buys 128s where a 22-file one buys 208s. But the reason THIS changeset could not fit was 61 seconds of work no importer asked for, and I reported one external blocker where there were two independent local causes underneath it. The shape is worth keeping: import time is a cost every consumer pays whether or not it uses the result. A module-scope fixture charges the pre-push hook, every importer, and collection itself — including runs that deselect all its tests. Behaviour is unchanged. Each accessor returns exactly what the module-scope name held, and the tests read through them.
… is now declared (#16187) Found while verifying #15738's premise, which turned out to be wrong in my favour and against the sweep: create/update/delete_user ARE authenticated, two hops down. Four distinct escapes, each a gate the resolver could not reach: 0 hops api/service_messages.py _check_admin defined locally 1 hop api/user_provider_credentials.py imported from a non-auth path 2 hops api/user_management/users.py route -> get_user_service -> get_tenant_context -> get_current_user wrong file api/user_management/router.py 24 lines, mounts four sub-routers, declares no routes at all The first two were caught in review and fixed with one-hop resolution. That fix did not reach the third — a narrower repair to a defect about narrow reach. The fourth is the worst of them: the sweep reported UNGATED for a file containing zero routes, so the verdict was about the wrong FILE rather than the wrong depth. _resolve_gate_chain follows Depends() to MAX_DEPENDENCY_HOPS, with a CYCLE GUARD. Not optional: these graphs are recursive in practice, and an unbounded walk hangs the guard rather than failing it — a guard that never returns is worse than one that returns wrong, because nothing reports it. _included_modules an aggregator is judged by what it MOUNTS, and its evidence names them: "aggregator: 4/4 mounted routers gated (organizations, password_change, teams, users)" 98 routers · 86 gated · 9 ungated · 2 documented-open · 1 unreadable KNOWN_UNGATED 10 -> 9 THE DEPTH IS DECLARED, NOT MERELY RAISED. UNGATED now means "not gated within MAX_DEPENDENCY_HOPS", and the module docstring says so. Raising the number four times would treat the symptom; a reach nobody can read is the defect this module exists to detect, and it had it. A test asserts the docstring states the limit — and caught me omitting it on the first pass of this very commit.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…tion about routes (#16187) Two of the three defects found by the code-review pass on this PR. The first is the same family the PR exists to fix, reintroduced one level up. The aggregator branch triggered on `not _per_route(...) and not _router_level(...)` -- a question about GATES, used to decide whether a module has ROUTES. Those coincide for `api/user_management/router.py`, the 24-line mount-only module that motivated the branch, and come apart in both directions for anything mixed: * mounts + its own UNGATED route -> no gates found, so it looked like a pure aggregator, was judged by what it mounts, and reported GATED while its own route stood open. False GATED is the direction that hides a hole. * mounts + its own GATED route -> the branch was skipped entirely and the mounted set was computed and discarded, so a sub-router reachable ONLY via the mount left the sweep. That is live: core_routers.py registers the user_management aggregator and none of the four routers it mounts. `_defines_routes` now answers the route question directly, and only in decorator position -- `.get` registers a route as a decorator and reads a dict everywhere else, so a call-position scan would report routes in every file that reads a dictionary and silently disable the branch it gates. The mixed case now classifies both halves: an ungated mount is an open door however well the module's own routes are gated. The aggregator recursion had no cycle guard, in a file that documents why an unbounded walk HANGS a guard rather than failing it. `classify` now threads `seen` through the mount graph. Tests assert the mixed case and the dict-read false positive on parsed source rather than on a live module: the tree contains no mixed module today, and a guard that can only fail once one appears is a guard reporting clean about a case it never examined. The review's third finding -- ACCESSOR_NOT_DECISION applied at 2 of 5 `_calls_named` call sites -- is NOT fixed here. Applying it at all five is correct in principle and makes `api.knowledge` report UNGATED, and that verdict is wrong: the file carries six `Depends(check_admin_permission)` routes and `check_admin_permission` raises 401/403. So the accessor exclusion is entangled with gate-chain resolution in a way this PR's scope cannot settle, and shipping it would trade a false GATED for a false UNGATED. Filed separately with the evidence.
73c33f8 to
1e727d5
Compare
Three corrections to this PR's commit message, before mergeA re-review of the 1. The deferral claimed a filing that did not exist — now #16202The commit message says of the Now filed as #16202, with the measurement: six A deferral is only legitimate when the thing deferred is tracked. Until #16202 existed, this was an 2. "the tree contains no mixed module today" is false
What is actually true is narrower, and the in-code comment at 3. "2 of 5 call sites" is 2 of 6
What the re-review confirmedAll three original findings fixed, traced rather than assumed:
Known-untested line
|
…n site (#16202, #16187) Resolves the finding this PR previously deferred, and the deferral's stated reason turns out to have been a misattribution. ACCESSOR_NOT_DECISION was subtracted at 2 of the 6 `_calls_named` call sites, and the four that skipped it (`_router_level`, `_per_route` x2, `_inline`) are the four that decide a verdict directly. So a module whose only apparent signal was an accessor that authenticates nobody read as GATED. Applying it at all six was tried earlier on this branch and `api.knowledge` went UNGATED, so it was reverted and filed as #16202 with "why not was not established". That reason was wrong. The revert did NOT fix the failure -- it persisted -- which proved the accessor was never its cause. The actual cause was the aggregator branch, fixed separately in this PR. The accessor change was judged by a test failure that belonged to a different defect. Re-tested on top of that fix: 17/17 pass. The exclusion is at the wrong granularity on its own, which is why the fix is not simply "subtract everywhere". `get_auth_middleware()` fetches; `get_auth_middleware().get_user_from_request(req)` authenticates. Excluding the accessor wholesale made `check_admin_permission` -- which raises 401/403 and gates six routes in `api/knowledge.py` -- unresolvable, because its body's only vocabulary hit was the accessor. ACCESSOR_DECISION_METHODS names the four methods that DO decide, derived from what the tree actually calls on the accessor rather than guessed: get_user_from_request (31 sites), check_file_permissions (11), verify_jwt_token, _extract_user_from_device_jwt. The rest fetch config or mint and hash credentials, and authenticate no caller. Verified verdicts rather than only the suite: api.knowledge gated per-route: check_admin_permission, get_current_user (was gated only via an incidental inline accessor call -- right verdict, wrong evidence, one tidy-up from a false UNGATED) api.jwks UNGATED -- reaching get_auth_middleware() still does not gate totals 96 routers, 84 gated, 12 ungated; no new entry on KNOWN_UNGATED It also gives the mixed-case evidence line real coverage. The re-review noted that no live module drove `ungated` non-empty; one does now -- `api.knowledge` mounts `api.knowledge_search_aggregator`, which is on the frozen ungated list, and the evidence line names it.
…ssue-16188-lazy-imports
Correction: my claim about what the accessor fix achieved was wrong, twiceThe delta re-review of 1.
|
…the match is name-only (#16187) Two minors from the delta re-review of 8cac39b. `_local_gates` and `_resolve_gate_chain` still subtracted ACCESSOR_NOT_DECISION explicitly after `_calls_named` began doing it internally. Harmless -- subtracting from a set that no longer contains the name is a no-op -- but it reads as though those two sites need something the other four do not, which is exactly the asymmetry the previous commit removed. One subtraction now, inside `_calls_named`. ACCESSOR_DECISION_METHODS matches `Attribute.attr` by NAME, without resolving what object the attribute hangs off. The review verified that is safe today: each of the four has exactly one definition in the tree, all in `auth_middleware.py`, and every call site under BACKEND resolves to the accessor. But it is one same-named method on an unrelated class away from reporting FALSE GATED -- the direction that hides a hole rather than raising a false alarm. Recorded at the declaration, with the fix that would be correct then (resolve the base object) and the one that would not (drop the name, which is what makes `check_admin_permission` resolvable at all). The re-review also corrected two things I had published about the previous commit, both now corrected on the PR: `api.knowledge` gated via per-route BEFORE that fix as well -- verified by reverting it and re-running, mechanisms identical -- so the "right verdict, wrong evidence" account was wrong, and the change is a consistency fix that alters no verdict. And `Depends(check_admin_permission)` appears 35 times in api/knowledge.py, not the six I wrote from a truncated grep.
1 participant
Closes #16187, #16188, #16202
Update — the deferred finding is now fixed, and the reason for deferring it was wrong
The commit message on
1e727d5badeferred theACCESSOR_NOT_DECISIONfinding, saying applying itat every call site made
api.knowledgereport UNGATED. That reason does not hold. Reverting theaccessor change did not fix that failure — it persisted — which proved the accessor was never its
cause. The actual cause was the aggregator branch, fixed separately in this PR. The accessor change
was judged by a test failure belonging to a different defect.
Re-tested on top of the aggregator fix in
8cac39b7a: 17/17 pass, and the verdicts are rightfor the right reasons:
The exclusion was at the wrong granularity, which is why the fix is not simply "subtract
everywhere":
get_auth_middleware()fetches,get_auth_middleware().get_user_from_request(req)authenticates.
ACCESSOR_DECISION_METHODSnames the four methods that decide, derived from whatthe tree actually calls on the accessor —
get_user_from_request(31 sites),check_file_permissions(11),verify_jwt_token,_extract_user_from_device_jwt. The rest fetchconfig or mint and hash credentials, and authenticate no caller.
It also closes the coverage gap the re-review flagged: that pass noted no live module drove the
mixed-case
ungatedbranch. One does now —api.knowledgemountsapi.knowledge_search_aggregator,which is on the frozen ungated list, and the evidence line names it.
Thinking Path
Two defects in the same scope —
repo_tests/guard modules that answer cleanly aboutthe wrong thing, or charge every importer for work no importer asked for. They touch
disjoint files, so they ride one CI run.
#16187 — the router sweep was wrong four ways. The enumerator merged in #16170 reported
routers as UNGATED that are gated. Verifying #15738's premise surfaced the third case and
fixing it surfaced the fourth:
_check_admindefined locallyget_user_service→get_tenant_context→get_current_userinclude_routerThe depth was never a declared quantity — it was whatever the recursion happened to reach.
It is now
MAX_DEPENDENCY_HOPS = 3, named and asserted, withseenas a mandatory cycleguard: the dependency graph here is recursive in practice, and an unbounded walk hangs the
guard rather than failing it.
#16188 — three guard modules did their sweep at import.
reach_declarations_testimports every guard to enumerate
declare()calls, so a change to_reach.pypaid thefull import cost before a single assertion ran. Import time is charged to every consumer
whether or not it uses the result — the pre-push hook, every importer, and collection
itself, including runs that deselect all of the module's tests.
What Changed
router_auth_enumerator.pyMAX_DEPENDENCY_HOPS = 3;_resolve_gate_chain(..., seen, depth);_included_modules()excludes aggregators;ACCESSOR_NOT_DECISIONexcludesget_auth_middleware;_parse_cached/_ENUMERATEDmemoisationrouter_auth_coverage_test.pyno_duplicate_dict_keys_test.py_swept()/_trees()—_sweep()parses 37,561 dict literals, now on first useman_page_indexer_path_resolves_15853_test.py_found_and_swept()replaces module-scope_FOUND, _SWEPT = ...model_pricing_tables_agree_15912_test.py_aliases()/_tables()/_shared()replace four module-scope bindings; one of them started an Ollama client at importVerification
Sweep result after the fix: 98 routers · 86 gated · 9 ungated · 2 documented-open · 1 unreadable
(was 98 · 83 · 12 · 2 · 1 — three of the twelve were false).
enumerate_routersacross 6 testsno_duplicate_dict_keys_testimportman_page_indexer_path_resolvesimportmodel_pricing_tables_agreeimportPre-push ran
router_auth_coverage_test.pyand passed within budget — the same changesettimed out three times before the import cost came down.
Two of the four false UNGATED verdicts were published as findings on #15745 before review
caught them; that is what prompted declaring the depth rather than inheriting it.
Model Used
Claude Opus 5