Repository navigation
fix(types): narrow the boundary reads across src - #556
Merged
Merged
Conversation
…rage First of the boundary tranche. Three modules, 209 -> 187. `asobi_console_session` was the useful sample: a `gen_server:call` result, an ETS row, a `persistent_term` read, a `min`/`max` clamp and a `?LOG_` return, all in one module. The session narrowing is real rather than cosmetic - it now checks all five fields of `session()` and validates cap atoms against the four `class()` values, where before it returned whatever the call handed back. `asobi_storage_controller` needed care rather than a guard. `Col` and `Key` come out of cowboy's untyped `bindings`, so the head wants `is_binary/1` - but `guarded/2` only checks whether storage is enabled, it does not catch, so a bare guard would have turned a malformed binding from a 404 into a 500. Both handlers get an explicit fallback clause answering `storage.not_found`, which is the shape the unguarded version produced by querying for the value and finding nothing. Two modules were started and deliberately backed out rather than half-finished: - `asobi_extensions`. Its `persistent_term` reads narrow cleanly, but the remaining error needs `[problem()]` - an eleven-member tuple union - to flow out of `validate/1`, and every spec added to a producer pushed the checking one layer further into untyped internals. It wants to be its own piece of work, not a rider on a boundary pass. - `asobi_body_cap_plugin`. Its one error is `json:encode/1` on an `asobi_error:object()` whose `details()` carries `term()` values. The honest fix is tightening that type across modules, not casting at this call site. Both are left untouched so the gate does not demand them here. eunit 2362/0, fmt.
Seven more modules. Tree 187 -> 170, and 209 -> 170 across the tranche. All the same class: a value crossing a boundary the checker cannot see through - an ETS row, a `gen_server:start_link` result, `application:get_key/2`, `application:get_env/3` - reaching a typed context. Narrowed at the read, with the shape the writer put there. **A narrowing broke a real behaviour, and CT caught it.** `asobi_guest_reaper` caches its live-guest count with a TTL, and `unknown` is a cached *failure* - caching it is the whole point, so a failing count is asked once per TTL rather than once per call, which `asobi_guest_SUITE` asserts. Guarding the read with `is_integer/1` alone rejected it and re-queried every time. The guard now admits both, and the expected type said so all along: `'unknown' | integer()`. That is the third narrowing this cleanup that changed behaviour where the old code passed a value through, after asobi_fixture_erase and asobi_console. Each was caught by a test rather than by review, which is the argument for running the suites per module rather than per tranche. `asobi_ops_features` had two `list_to_binary(Vsn)` sites and I fixed one, so the second only surfaced on the re-check - worth noting that the checker finds the sibling the eye skips. eunit 2362/0, ct 390/0, dialyzer clean, fmt.
🟡 Code Coverage — 76.8%9297 of 12107 lines covered. 🟡 ELP Lint — 192 warnings212 diagnostics found. See job logs for details. ✅ ELP eqWAlizeNo diagnostics. |
Two agent reviews, three Mediums and eight lesser findings. The two that
mattered:
**`do_delete_storage/1` got the guard and not the fallback, and the PR body
said otherwise.** `guarded/2` checks whether storage is enabled; it does not
catch, so the guard alone turned a malformed binding from a 404 into a 500 -
the exact outcome the get-side comment explains the fallback prevents. Both
reviews reproduced it. Fixed, and there is now a test asserting the whole
handler family answers the same way, which fails with `[delete]` if the clause
goes again.
**`env_modes/1` inverted the operator/script precedence.** Re-narrowing it for
the checker dropped a malformed operator entry *before* the merge, so the
bundle's definition of that mode went live under a name the operator had
pinned - against this module's documented guarantee that no bundle reload can
redefine or drop an operator mode. The script layer is now shadowed by the
operator's declared names rather than by the entries this module can use, and
ignored entries are logged rather than vanishing. Probed: without the fix the
new test reports `got: #{module => script_mod, match_size => 8}`.
The rest:
- `caps/1` was a second copy of `asobi_ops_caps:class_names/0`, in a module
whose sibling `asobi_ops_token:caps/1` documents the opposite policy - refuse
an unknown class rather than drop it, so a mint-side typo is not a 403 far
from its cause. The allowlist now lives in `asobi_ops_caps:class_of/1` and
the console asks.
- `secret/0` kept a `<<>>` fail-open default under a CSRF guarantee that rests
on the key being secret. An empty HMAC key makes the token computable from
the session id. Default removed.
- The count-cache guard admits `non_neg_integer() | unknown`, which is what its
spec claims; `is_integer/1` alone asserted something wider on a public table.
- `evict` was the one leaderboard read that widened `[]` to `_`, and it is the
erasure path. A row whose score is unusable now still leaves the index.
- The discovery listing is validated at the WRITE instead of filtered on every
read. Filtering on the read either dropped a world from a browse silently or
rebuilt the list per hit; checking once per cache fill does neither, and lets
`cache_lookup/2` keep promising `[map()]`.
- `list_to_binary([C || C <- Vsn, is_integer(C)])` silently truncated an iolist
vsn like `["1.0", ".0"]`. One shared `vsn_binary/1` handles chardata and
falls back rather than raising.
- `capabilities/0` normalises `enabled` instead of filtering, so a capability
that answers oddly reports `false` on the diagnostic surface rather than
disappearing from it.
- Dropped the duplicate label normaliser in the controller; `label/1` already
does it, and `create/1` now accepts what `label/1` accepts.
Note the count moved 170 -> 178 -> 172 while working these: widening
`cache_lookup/2` to `[term()]` exported eight errors to modules this branch
does not touch. The gate would not have caught that - it is diff-scoped - which
is the argument for checking the tree total as well as the gate.
eunit 2364/0, ct 390/0, dialyzer clean, fmt. Zero errors in all ten touched
modules.
The gate rejected the previous commit: the new test called the controllers directly, so eqwalizer checked its bare map against cowboy_req:req() and found three errors. Every other test in this file routes through a `fun asobi_storage_controller:F/1` in an unspec'd list, which is why they do not hit it. Matched that. Also: my local pre-push check passed because it used a touched-file list generated before the commit that added the test files. The gate compares against origin/main and does not have that problem, which is the whole point of it - but it means a stale local check is worse than none.
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.
Tranche 3 of #435, scoped to the boundary class: a value crossing something the checker cannot see through — an ETS row, a
gen_server:call/start_linkresult,persistent_term,application:get_key/env— reaching a typed context.Tree: 209 → 170. Ten modules.
What a boundary narrowing means here
Not a cast. Each read is narrowed to the shape the writer put there, so a clobbered key or an unexpected row is a legible
function_clauserather than a value flowing on as the wrong type.asobi_console_sessionis the clearest example: it now checks all five fields ofsession()and validates cap atoms against the fourclass()values, where before it returned whatever thegen_server:callhanded back.Three places this needed care, not a guard
asobi_storage_controller.ColandKeycome out of cowboy's untypedbindings, so the head wantsis_binary/1. Butguarded/2turned out to only check whether storage is enabled — it does not catch — so a bare guard would have turned a malformed binding from a 404 into a 500. Both handlers get an explicit fallback answeringstorage.not_found, which is the shape the unguarded version produced anyway by querying and finding nothing.asobi_guest_reaper— a regression I introduced, caught by CT. The live-guest count is cached with a TTL, andunknownis a cached failure; caching it is the point, so a failing count is asked once per TTL rather than once per call, whichasobi_guest_SUITEasserts. Guarding withis_integer/1alone rejected it and re-queried every time. The expected type had said so all along:'unknown' | integer().That is the third narrowing in this cleanup to change behaviour where the old code passed a value through, after
asobi_fixture_erase(#555) andasobi_console(#555). All three were caught by a test, none by reading. I now run the affected suites per module rather than per tranche.asobi_ops_featureshad twolist_to_binary(Vsn)sites; I fixed one and only the re-check found the sibling.Two modules deliberately left alone
Not deferred vaguely — each has a reason the gate would otherwise force me to fudge:
asobi_extensions: itspersistent_termreads narrow cleanly, but the remaining error needs[problem()]— an eleven-member tuple union — threaded out ofvalidate/1, and every spec added to a producer pushed the checking a layer deeper into untyped internals. Its own piece of work.asobi_body_cap_plugin: one error,json:encode/1on anasobi_error:object()whosedetails()carriesterm(). The honest fix is tightening that type across modules, not casting at the call site.Reverted both to untouched so the gate does not demand them here.
Checks
eunit 2362/0, ct 390/0, dialyzer clean, fmt, and
elp eqwalizeNO ERRORSon every touched module.