SIGN IN SIGN UP

core: declarative inbound hooks — a gamedata-declared engine detour (#77)

* plan: declarative inbound hooks (7 tasks)

* shim: the engine-free half of inbound hooks — shape vocabulary, bypass latch, collapse

* shim: harden hook_dispatch tests — out-of-range bypass ids, shape round-trip, ASan/UBSan gate

Fix round 1 findings from review: the bounds guard in S2Hook_BypassArm/BypassTake had no
regression protection (deleting either guard still passed the suite because it only ever
drove ids 0/1 through them); the shape round-trip only ever asserted id 0; and the test
script had no sanitizer probe unlike its siblings (test-defer-queue.sh, test-call-validate.sh).

* shim: sentinel-guard the bypass latch storage so its bounds check proves itself without a sanitizer

Fix round 2: the round-1 out-of-range test passed unchanged with either bounds guard removed and
sanitizers off, because g_bypass was a bare array next to the separate g_ops global — two
independent globals have unspecified relative address order, so an off-the-end access could land
anywhere, including nowhere observable. Move the latch array into a BypassState struct flanked by
guardLo/guardHi sentinel bytes: struct members keep declaration order and an all-bool struct has no
padding, so slots[-1]/slots[S2_HOOK_MAX] are guaranteed to alias the sentinels and nothing else.
Added TEST-ONLY S2Hook_DebugSetSentinels/DebugGuardLo/DebugGuardHi accessors so the test can plant a
dirty sentinel before driving an out-of-range BypassTake, proving the guard rejects by id alone
without ever touching adjacent storage — deterministic with sanitizers off.

S2Hook_BypassArm/BypassTake's signatures and semantics are unchanged; only the internal storage and
test coverage changed.

* shim: inbound hook thunks, lazy detour install, and the block-scoped arg view

* shim: range-check the patch site, liveness-gate the arg view, gate S2EngineOps field order

Fix round 1 on the inbound-hook engine half.

- S2_HookInstall proves the whole 14-byte patch window is inside some loaded
  module's executable range before s2detour reads a byte. A stale offset that
  resolves to a wrong-but-mapped address used to decode fine and get patched
  (engine corruption reported as success); an unmapped one SEGV'd inside
  hde64_disasm. InModuleText's rule, re-asked module-agnostically, stays in
  engine_calls.cpp so engine_hooks.cpp keeps no SDK dependency.
- The arg view now carries a liveness token: each thunk publishes its frame in
  g_activeView for exactly its dispatch (save/restore, so a nested hook hands
  the outer frame back), and every accessor requires the caller's pointer to BE
  that view. A retained view is -1, not a write into a reused stack frame.
- core/src/v8host.rs mirrors the two appended S2EngineOps fields, and
  scripts/check-engine-ops-order.sh gates the two field lists positionally by
  name. Nothing else catches a reorder: core does unsafe { *ops }, so a
  one-field slip calls the wrong pointer with the previous call's registers.
- The 'no entity' sentinel moves to S2_ENTITY_HANDLE_NONE in engine_calls.h;
  InfoFor is constexpr and every shape id is static_asserted to fit the view;
  S2_HookResetAll clears the slot table beside s2detour::RemoveAll().

* core: the declarative hook registry — mandatory validators, lazy install, engine:hooks

* core: bind a hook view to its own dispatch; refuse an unpairable latch and a non-representable write

* ctx: a generic game-package namespace extension point, and CS2's two hook namespaces

* ctx: refuse a game-package namespace that would clobber a built-in ctx member

Fix round 1 on Task 4 (declarative inbound hooks). The merge loop now runs
LAST, after every built-in ctx member is attached, so `ns in ctx` is a
complete collision test instead of a hand-maintained list that only
protected whichever built-ins happened to be assigned before it. A
colliding namespace, a non-function factory, and '__proto__' are each
refused with a named WARN instead of silently dropped or clobbering.

Pinned by v8host.rs::game_ctx_namespace_cannot_clobber_a_builtin, which
registers a synthetic @s2script/cs2 package declaring events (collision)
and gameRules (no collision) and asserts ctx.events.on survives while
ctx.gameRules still merges.

* gamedata: declare onTerminateRound and onRespawn as inbound hooks

* sdk: generate the ctx hook augmentation from the descriptors, and gate its freshness

* changeset: hooks ctx augmentation (@s2script/sdk, @s2script/cs2 minor)

* core: a hook's surfaced receiver is a real EntityRef, not an array shaped like one

* docs: declarative inbound hooks — the model, the bypass semantics, and the shape rule

* core: refuse an out-of-f32-range write, and name a re-entrant hook skip

Two silently-wrong-value paths found by the whole-branch review, plus the three tests
whose absence let them (and one live guard) survive.

I2 — a float write outside f32 range became +inf and REPORTED SUCCESS. The int arm
range-refused; the float arm only checked finiteness, and 1e300 is a perfectly finite
f64 whose `as f32` is INFINITY. `hook_write_f32` returned 0, so there was no note_miss
and no WARN, and the thunk handed the engine an infinite round-restart delay. The
comment two lines above already claimed the rule the code did not implement.

I3 — a re-entrant dispatch VANISHED. The bypass latch is scoped to (owner, call name)
where SourceMod's g_pIgnoreTerminateDetour is global, so "unlatched => engine-originated"
only holds for the hook's own `bypassWith` descriptor; any other JS->engine path to the
same address (a plugin's own `calls` entry under `engine:calls`) arms nothing, the detour
fires while HOST is borrowed, and `fan_out_collapsing` discarded the `Deferred`. Skipping
is the safe direction and stays — the engine call proceeds unhooked — but it is now
NAMED: `dispatch_hook` takes `fan_out_inner` directly and reports the skip through
`note_miss`, rate-limited to once per (owner, hook) because this can fire on every engine
call. The cap is on the bounded axis (declared hooks, <= MAX_HOOKS) rather than a counter
or a clock, because the condition is structural — some path reaches this address without
the latch — so the first occurrence carries the whole finding. Cleared by
drop_owner/reset_all so a reload re-arms it.

Tests, each proven to fail before its fix:
- the float non-representable arm ("reached the engine as inf" before, refused after)
- the re-entrant skip ("must be NAMED, not silent: available" before)
- the SAME-hook epoch case, which is the one the whole epoch-over-hook-id argument rests
  on and which only the cross-hook case covered. Mutating the binding token from the
  per-dispatch epoch to the hook id makes the pre-existing cross-hook test still PASS and
  this one FAIL — the exact discrimination the epoch exists for.
- the setter's `Dead` branch: deleting its log_warn left the suite green, which made a
  write through a dead view entirely silent (unlike a read, which returns a visible
  `undefined`, a lost assignment looks identical to one that worked).

Also records on `PERMISSION` that the non-exempt branch is reachable only from cargo test
in v1 — kept as-is rather than narrowed to match; see the docs commit.

cargo test -p s2script-core: 538 passed, 0 failed (baseline 534).

* shim+gate: assert the sentinel layout, clear latches at teardown, tie params to shape arity

Three ledger items from the whole-branch review, all of the self-degrading kind: a guard
that stops proving anything, a teardown that forgets half its state, and a gate that
cannot see the mismatch it was built next to.

1. static_assert on BypassState's layout. The entire bounds-guard proof in
   hook_dispatch_test rests on slots[-1] and slots[S2_HOOK_MAX] aliasing guardLo/guardHi
   exactly, i.e. on sizeof/offsetof — and a later member insertion degraded that silently,
   leaving a test that passes while proving nothing. Now a compile error: inserting a
   member fires all three asserts by name. The header comment claiming "C++ guarantees no
   padding" is reworded too — that follows from alignof(bool) == 1, an x86-64 SysV fact,
   not an ISO C++ mandate, which is exactly why it is now asserted rather than asserted-in-
   prose.

2. S2_HookResetAll did not clear the bypass latches. It reset g_hooks and g_activeView;
   nothing cleared hook_dispatch.cpp's g_bypass. This answers Task 1's own unresolved
   carry-forward: only the thunk clears a latch, so an armed-but-never-taken one survives
   unload, and slot ids are REUSED across a reload — the next load's first genuine
   engine-driven call to that id would be silently bypassed. S2Hook_BypassResetAll() is
   called from inside S2_HookResetAll rather than beside it at the Unload site, so "forget
   every installed hook" stays one operation. It clears `slots` only, deliberately not the
   sentinels — scrubbing those would scrub the evidence of an earlier out-of-bounds write.
   The new test fails (3 checks) with the reset stubbed out.

3. check-call-descriptors.sh now ties `params` arity to the shape's. This was the one
   place the "shape is compile-time, location is data" split leaked: `params` is data that
   must agree with a compile-time fact and nothing connected them. A fifth param on the
   4-arg shape previously reported "13 descriptor(s) well-formed", hookgen emitted
   `readonly _unused5: number`, check-hooks-generated.sh was satisfied by the regenerated
   file, and every runtime read of it was `undefined` behind a `number` type.

   The arity is derived FROM THE SHAPE NAME (`this_void` -> 0, else count the tokens after
   `this_`), never from a second table here — the name is the ABI, and a table would be a
   fifth copy of a closed cross-language set needing its own drift gate. A shape name that
   does not follow the rule fails the gate loudly instead of skipping the check.

   Drove the reviewer's exact mutation: the gate fires and names the offending param, on
   both arms (5 params on this_f32_i32_i32_i32; 1 param on this_void). Restored; gamedata
   is byte-identical.

* docs: game-package-declared hooks in v1, and the bypass latch is a semantic, not a proof

Two documented claims stronger than the code, narrowed to what actually ships. No runtime
check is weakened to match either one.

I1 — plugin-declared hooks cannot be built at all, and the docs said they could. The plan's
goal said "a game package OR PLUGIN declares an engine detour"; ARCHITECTURE.md §2.0.7
documented `engine:hooks` as operator-facing. But `s2s build` hard-fails on both halves:
"gamedata section 'hooks' is not supported in v1" and "unknown permission engine:hooks".
An operator who read §2.0.7 and granted the permission would find no plugin author could
produce a .s2sp that uses it.

Narrowed rather than opened, because the SDK validator is not the only gap: a plugin also
has no typed subscribe surface (`__s2_hook_on` is absent from globals.d.ts, `__s2pkg_game_ctx`
is a game-package-only extension point) and hookgen is hardcoded to gamedata/cs2/game.cs2.jsonc.
Opening it properly is its own slice, and the three things it needs are now written down in
all four places someone would look: ARCHITECTURE.md §2.0.7, the spec, the plan's goal line,
and validate.ts itself — where the omission is now explained so it is not "fixed" alone,
which would let a plugin ship a .s2sp whose hooks nothing can subscribe to.

Core's plugin path is left EXACTLY as it is. It is the correct check for the design; a
default-deny gate relaxed to fit a temporary scope stops being a gate.

I3 (doc half) — "by construction the one case where core is not borrowed" was stronger than
the code. Our latch is scoped to (owner, call name) where SourceMod's is global, so the
inference only holds for the hook's own `bypassWith` descriptor. §2.0.7 and spec §3 now name
the fan-out's re-entrancy skip as the actual backstop, and say what the latch does guarantee:
SourceMod's semantic, not the absence of the re-entrant case. The skip itself is now a named,
rate-limited degrade (previous commit).

* gamedata: carry the hooks section through the owner merge, and report a section nobody consumes

* gate: the hookgate live fixture — bypass, suppress, mutate, and the receiver EntityRef

The fixture that drove the six live-gate checks, kept beside a4gate/a5bgate/ddqgate so the
next person can re-drive them rather than re-derive them. Not a shipped plugin.

* docs: record the hooks live gate — arming and lazy install hold, both installs refused

The gate ran and found the blocker one layer below this slice: s2detour cannot relocate a
rip-relative lea or a stolen call rel32, and both targets have one inside the 14-byte steal
window. Records what passed, what is not yet server-proven, and the two instruction classes the
next slice has to handle.
G
Gabriel Hirakawa committed
f0cb022f665054926a894197367ab9a4f9eded3b
Parent: 79dede5
Committed by GitHub <noreply@github.com> on 8/3/2026, 6:05:40 PM