Skip to content

fix(extensions): close three contract gaps found building the first extension - #364

Merged
Taure merged 1 commit into
mainfrom
fix/extension-contract-gaps
Aug 4, 2026
Merged

fix(extensions): close three contract gaps found building the first extension#364
Taure merged 1 commit into
mainfrom
fix/extension-contract-gaps

Conversation

@Taure

@Taure Taure commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Three gaps asobi_quests hit as the first real extension built against the
machinery merged in #359. Closes #360, closes #361, closes #362.

#360 - an extension cannot mint an error code

asobi_extension gains an optional codes/0:

codes() -> #{~"quests.already_claimed" =>
               #{status => 409, message => ~"This quest was already claimed."}}.

Shape, and why. The ticket offered a register_codes/1 call or a manifest
callback. The callback wins because it is checkable before boot and needs no
process, but the merge direction matters more than the declaration:

  • asobi_extensions:resolve/0 builds one Code => {Status, Message} map into
    persistent_term from validated manifests. asobi_error reads it; it never
    calls resolve/0. Nothing pushes into asobi_error, so the only writer is
    the resolve path and the set is closed per deployment - a string arriving in
    a request or a Lua script still cannot become a code. The literal-code rule
    asobi_error_contract_tests enforces over core is unchanged.
  • A code domain now claims the rpc namespace exactly as an RPC prefix does,
    since they are the same token by construction. That gets the whole
    reservation for free: storage.wedged is refused as reserved, a domain
    outside owns.rpc is an undeclared claim, and two extensions in one domain
    collide. No second mechanism.
  • asobi_error:core_codes/0 is new and is what asobi_extension_reserved
    derives from. Deriving from codes/0 once extension codes are installed
    would tell quests it may not claim quests - latent today because check/0
    runs before the term is written, live the moment anything re-validates a
    booted node.

A code the extension did not declare is still 500 and still logs
undefined_error_code. Owning the domain does not mint every code inside it.

#362 - no raw-query escape hatch

asobi_repo:increment/3, not query/2:

{ok, Row} = asobi_repo:increment(asobi_quest_progress,
                                 #{player_id => P, quest_id => Q},
                                 #{counter => 1}).

One INSERT ... ON CONFLICT ... DO UPDATE SET counter = t.counter + EXCLUDED.counter, with the insert half so the first event does not need an
insert-then-retry - which is the race the primitive exists to remove.

Why not the two-line query/2 the ticket proposed. A raw-SQL hole in the
data seam is an injection surface the moment any part of it takes a
caller-supplied string, and on a library that surface is permanent. The shape
here makes it structurally impossible: every identifier is a field of the
schema you pass, matched against Schema:fields() and rejected unless it is a
plain SQL name; every caller value is a bound parameter. query/2 could not
make that promise, and once it existed every extension would reach for it
rather than the safe primitive. If something else is not expressible, that is
a ticket against core, not a hole in the seam.

Where core has the same problem. Two sites, both named in the PR body
rather than changed:

  • asobi_economy:acquire_wallet_lock/2 - pg_advisory_xact_lock, not a
    counter and not something a counter primitive should grow to cover.
  • asobi_player_stats:bump/3 - the exact twin of the quests case. Left
    deliberately: bump/3 runs inside the transaction that makes a match final,
    and today a missing stats row is a 0-row UPDATE and a warning. Under an
    upsert a deleted player becomes a foreign-key violation, which in Postgres
    aborts the enclosing transaction and would take the match record with it.
    Worth doing, but behind its own change with its own test.

#361 - args length never checked against mfa arity

Confirmed: one guard clause. length(Args) =:= A in is_lua_binding/2, plus a
binding_problem/1 that names the mismatch instead of reporting it as a
generic malformed binding. Core's own asobi_fixture_quests_extension declared
status with args => [binary] and arity 2 - fixed to arity 1, and the same
mistake fixed in the guide's worked example.

Documentation drift

guides/extensions.md's "Two gaps you hit today" is gone: kura is pinned at
2.20.1, migrations run through asobi_repo:migration_apps/0 and
#kura_assoc.on_delete exists. The same stale claim in
asobi_repo:migration_apps/0's moduledoc and one test comment went with it.
The guide gains sections for error codes and counters.

Tests

12 new tests, each verified to fail when its fix is reverted:

Revert Failing tests
entry/1 extension lookup 2
code domains claim rpc 3
codes_problems/1 2
core_codes/0 in the reserved set 1
length(Args) =:= A 1
+ EXCLUDED -> replace 1 eunit, 1 ct

asobi_repo_increment_SUITE runs against real Postgres. Its second case
(creates_the_row_when_it_is_missing) covers the insert half and passes under
the accumulate-revert, since only the conflict branch changes there.

fmt, xref, dialyzer, ex_doc clean. Full eunit 1418/0. ct for
asobi_repo_increment_SUITE, asobi_match_stats_SUITE and
asobi_economy_SUITE green.

…xtension

An extension could not mint an error code, could not increment a counter
without naming a kura internal, and could ship a Lua binding whose args and
mfa arity disagree.

- asobi#360: `asobi_extension` gains an optional `codes/0`. The declared set
  is read once at resolve time and merged into `asobi_error:status/1`,
  `message/1` and `codes/0`, so an extension's domain condition answers its
  own status instead of 500 and stops logging as a core defect. A code domain
  claims the rpc namespace exactly as an rpc prefix does, so `rebar3 asobi
  check` refuses a code in core's namespace or another extension's, and
  refuses a bare one. `asobi_error:core_codes/0` is the reservation source, so
  an extension is never told it may not claim the namespace it just claimed.

- asobi#362: `asobi_repo:increment/3` upserts integer counters with
  `SET c = t.c + EXCLUDED.c`. Every identifier comes from the schema and every
  value is a bound parameter, so the seam gains no injection surface - which a
  general `query/2` could not promise. Core's remaining raw-SQL sites are
  `asobi_economy:acquire_wallet_lock/2` and `asobi_player_stats:bump/3`.

- asobi#361: one guard clause in `is_lua_binding/2` compares `length(args)`
  with the mfa arity, plus a message that names the mismatch. Core's own
  quests fixture declared `status` with one arg and arity two; fixed.

Also drops the extensions guide's stale kura claims: migrations run and
foreign keys cascade on the pinned kura 2.20.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🟡 Code Coverage — 74.1%

6201 of 8374 lines covered.

@Taure
Taure merged commit 9d1a389 into main Aug 4, 2026
15 checks passed
@Taure
Taure deleted the fix/extension-contract-gaps branch August 4, 2026 10:20
Taure added a commit that referenced this pull request Aug 4, 2026
* feat(extensions): make rpc/0 and lua/0 reachable

`rpc/0` and `lua/0` were validated and then read by nothing.
`asobi_extensions:resolve/0` had three callers and none touched either,
`asobi_lua_api:install/2` installed a hardcoded namespace list, and there
was no rpc.call handler anywhere in src. An extension could declare a
method and a namespace, have both accepted, and be reachable by nobody.

The RPC dispatcher (`asobi_rpc`):

- `rpc.call` / `rpc.ok` / `rpc.error` on the existing socket, `protocol: 1`
  in the payload, `params` and `result` always objects.
- `cid` is required and validated here (1-64 printable ASCII), unlike the
  optional echo elsewhere on the socket: it is the only way a client pairs
  a reply with its call, and it is reflected into a frame. A rejected cid
  is not echoed back.
- The handler contract is specified: `(Params, Ctx) -> {ok, map()} |
  {error, Code} | {error, Code, Details}`, so `rpc/0` targets are arity 2
  and `asobi_extensions` now refuses anything else at `rebar3 asobi check`.
  The failure half is a code, not a status: status and object both derive
  from it, and #364 gave extensions their own code domain, so an
  extension's error surfaces as its own code rather than `internal`.
- `asobi_readiness:guard/0` runs before every dispatch, so the window
  between route compilation and migrations answers `not_ready` (503).
- Only a raise, a return outside the contract, a non-2 arity or a result
  that cannot be encoded becomes `internal`, each with one logged line.

The Lua injector (`asobi_lua_api`):

- Extension namespaces are pre-created alongside core's and installed in
  the same PreInstall window, so a script's closures can see them.
- Declared `effects` runs through the existing `pick/3` dry-run swap, so a
  `write` binding is stubbed in a probe VM exactly like core's.
- `{M, F, A}` is applied fully qualified, never captured as a fun.
- `vms` is honoured: the install site now states its VM kind, and a binding
  absent from that kind does not even get its namespace table created.
- The binding contract is specified: positional declared arguments,
  decoded to their `args` types, returning `{ok, term()} | {error, binary()}`
  through the same `{ ok = }` / `{ error = }` envelope core uses. A wrong
  or missing argument is a legible error at the script's call site.

No capability class on `rpc/0`. `read | player_data | config` (ADR 0007)
is an operator vocabulary a player never holds, so tagging a socket method
with one makes it deniable for every caller the dispatcher has - another
unreachable declaration. The reachable home for an operator-only method is
the ops plane, which is read-only by assertion today.

* docs: reference lua/0 as a callback so ex_doc resolves it

ex_doc's `mod:fun/arity` form resolves exported functions; lua/0 is a
behaviour callback, so it needs the `c:` prefix the codebase already uses
elsewhere (see asobi_join_ctx referencing c:asobi_match:join/3).

Four warnings on this branch, none on main. ex_doc is on the pre-push
checklist and was the one gate this PR skipped.
Taure added a commit that referenced this pull request Aug 4, 2026
…queue claims (#372)

Four gaps the first real extension (widgrensit/asobi_quests) filed and #364 did
not close.

Gap 6 - an extension holding rows for a player could make that player
undeletable, and had no way to say otherwise. Cascade is declarable
(`on_delete = cascade` on the `#kura_assoc`, carried into the generated
migration by rebar3_kura), but it is wrong for anything holding a financial or
audit row - the exact case that rejected a blanket cascade - and such an
extension had no alternative to register. So the delete became a constraint
violation the guest reaper swallowed as `skipped`.

New optional callback erase_player/1, run by asobi_extension_erase from inside
core's own transaction, extensions before core, in dependency order. Not an
owns/0 key: owns/0 reserves names and executes nothing, and cascade is already
declared where the database enforces it, so a second declaration of the same
fact in the manifest could disagree with the schema that decides. Erasure is
atomic across extensions rather than best-effort - a half-finished erasure
reporting success is a worse answer to a data-subject request than one that
fails loudly and can be retried - and every extension shares asobi_repo, so the
single transaction costs nothing to arrange.

Gap 7 - a sup/0 child could not assume the pool was usable at init/1, and a
crash loop there ends with the extension dark and staying dark. asobi_sup
starts after migrations, so the readiness answer is already final when
asobi_extension_sup initialises: if it is false, no extension starts and one
line says which ones did not, instead of each crash-looping through its restart
budget with an OTP crash report as the only explanation. init/1 may now query,
and the two ordering guarantees are written down rather than implied.

Gap 9 - owns.queues was unenforceable. rpc and lua claims derive from the
manifest, so a collision is caught without owns/0 at all; a queue was only ever
what owns/0 said, so a typo was invisible. Queues now derive from queue/0 +
perform/1, and tables from table/0 + fields/0, on the extension's own modules -
through asobi_extension_reserved, the same rule that finds core's. That leaves
owns/0 doing one job, and the docs now say so: it is the closed-set assertion
over what was derived, not the source of anything.

And vms => [bot] installed nothing. A bot script loads through
asobi_lua_loader:new/1, whose PreInstall is the identity, so it never reaches
asobi_lua_api:install/2. Refused at rebar3 asobi check instead of silently
inert. Making it work was rejected: game.<ns> in a bot VM would be one
namespace floating in a game table with no game.log or game.economy under it,
and bot_Spark is not a player row, so the argument every binding takes cannot
be supplied. guides/lua-bots.md stays true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment