Skip to content

feat(scripts): bind typed event handlers in TypeScript and Lua - #646

Merged
Mathih13 merged 9 commits into
mainfrom
feat/typed-script-events
Oct 10, 2026
Merged

Mathih13 merged 9 commits into
mainfrom
feat/typed-script-events

Conversation

@Mathih13

@Mathih13 Mathih13 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Runtime Script authors had to write string event names and numeric IDs in comments, and read a generic global event. The new authoring form binds a named function to a typed event in TypeScript and uses the same Event Binding in Lua:

function welcome(event: PlayerLoginEvent): void {
  send_chat(event.player, "Welcome!");
}
events.player.onLogin(welcome);

Generate editor types and core hook dispatch from one event catalogue. Each source has one Event Binding. Login and level-up expose a required Character handle, and level-up exposes the attained newLevel. The wrapper preserves numeric Script Answers and the existing effect staging rules.

The compiler records stable IDs in script-ids.json, preserves prior artifact and legacy IDs, and allocates unused IDs across the enabled Package Inventory for new sources. Existing directives and stored Script Artifacts remain supported. This is Package API version 2; version 1 examples retain their earlier toolchain contract. The CLI pin includes the identity digest and scaffold changes from LyraCoreProject/lyracore-cli#72. LyraCoreProject/packages#80 migrates the examples, tests the compiled Package Event through the new ask_artifact_offline Package test helper, and publishes the matching api-v2 collection tag.

Validation: 76 Bun tests, Datascript type checking, Module unit tests, Package API lint, Package Delta tests, workspace Clippy and Wasm checks passed. All five durable Runtime Script tests passed on disposable Standalones. The typed-handler case applied the compiled Lua, awarded quest XP through core gameplay and observed Ding 2 then Ding 3. Separate adversarial standards and specification reviews passed after migration, Unicode and allocation fixes.

Made with GPT-6 Astra and GPT-6.1 Sol through the Codex harness in T3 Code.

Copy link
Copy Markdown
Contributor Author

Code review: Standards and Spec

I reviewed the diff from main...78c1b3f on two separate axes. No issue is linked, so the PR description is the spec. I also used the acceptance criteria from #320 as background. Priorities: P1 must fix before merge, P2 should fix, P3 nit.

Standards

Breaches of documented standards

  • P1. "player" is used as a noun in the new public API. CONTEXT.md and CORE_TERMS.md list the Character term with Avoid: "player (as a noun in code)". AGENTS.md says new names must not use Avoid words. The breaches:

    • events.player.* and PlayerEntity (runtime-script.base.d.ts:23)
    • the payload fields player and victimIsPlayer (events.json)
    • ScriptPayload.player (module/src/runtime_script.rs) and let mut player (module/build.rs)

    generate-events.ts already uses the correct phrase, "a required Character Entity". Package authors will write against this API, so a later rename breaks every Package that uses it. Rename it now.

  • P2. "registration" is an Avoid word for Event Binding ("hook registration"). It appears in EventRegistrationOptions (runtime-script.base.d.ts:35, generate-events.ts) and in the registration key in events.json. Suggested names: EventBindingOptions and binding. The PR text says "the same registration in Lua", so it needs the same change.

  • P2. "return value" is an Avoid word for Script Answer. It appears in the comment at lua-emit.cjs:22.

  • P3. Commit b8be816 uses "probe", which is an Avoid word for Verification. The scope script in 78c1b3f does not match scripts in the other four commits.

  • P3. Prose:

    • docs/package-api.md:471 is passive: "a conflict is refused".
    • packages/README.md:226 writes "script identity" in lowercase. The term is "Script Identity".

Smells (judgement calls)

  • P2, Duplicated Code. tsBinding and luaBinding in bindings.ts repeat the same steps after they parse: the own-event offset, the handler declaration check, the argument count check, the options loop and localName. Have each parser return the same small shape and check it in one function.
  • P2, Duplicated Code. The ID range exists twice: FLOOR/CEILING in script-ids.ts:6-7 and SCRIPT_ID_FLOOR/SCRIPT_ID_CEIL in build-scripts.ts:20-21. The error helper also exists twice: refuse in bindings.ts and build-scripts.ts, and refusal in script-ids.ts, all with the same body.
  • P2, mixed field style. The new payload fields are camelCase (newLevel, instanceId). Entity fields on the same Lua table are snake_case (max_health, map_id).
  • P3, Duplicated Code and Speculative Generality. Three places check the required Character field: checkCatalogue, the build.rs assert and bindInvocation. required is never false.
  • P3. bindings.ts imports EVENTS from the generator script. The import runs checkCatalogue() and reads a file as a side effect.
  • P3. CODING_STANDARDS says "One test covers one behavior". Each of these tests covers two:
    • compiled_typescript_login_handler_sends_chat_through_its_event_parameter checks success and the missing Character failure.
    • compiled_package_handler_keeps_zero_and_negative_answers_and_discards_failed_effects branches on level == 0.
  • P3. CODING_STANDARDS says "Remove a displaced path". Legacy Script Directives stay next to Event Bindings and nothing says when they go. Add a removal condition.

Spec

Most claims in the description hold:

  • events.json feeds build.rs, the .d.ts, the Lua definitions and event-names.rs.
  • A test asserts that the parser's event list equals the dispatch list (script_binding.rs:246).
  • The TS and Lua parsers both allow only one Event Binding per file.
  • The wrapper returns the handler's Script Answer. Tests cover zero and negative Answers and discarded effects.
  • A bun test fails when the generated files are stale.

(c) Implemented, but the implementation looks wrong

  • P2. Spec: "Existing directives and stored Script Artifacts remain supported." Any top-level call on a global events in a legacy Lua script now counts as an Event Binding (bindings.ts:156-159). The build then refuses the script as "both" or as an "unknown Event Binding".
  • P3. Spec: "Generate … core hook dispatch from one event catalogue." The move to JSON lost the per-event notes from the old module/build.rs on actor and target: the corpse target, no actor on on_hp_threshold, and the stale level-up snapshot. Only some of these notes are now in packages/README.md.
  • P3. Spec: "The wrapper preserves numeric Script Answers." It does. But bindInvocation (bindings.ts:216-227) runs the whole file before the handler on each Invocation, so effects from top-level code are staged too. That matches the old rules, but the docs only say that it "captures the declared function and calls it".

(a) Missing or partial

  • P2. Spec: "allocates unused IDs for new sources." IDs are unused only inside one Package. allocateScriptId (script-ids.ts:105-118) hashes the package name and stem, then checks only that Package's ledger. All Packages share one ID range, so two Packages can get the same ID. Nothing reports it until lyracore-delta-check, and the docs do not tell the author how to fix it (by editing script-ids.json?).
  • P2. Spec: "This is Package API version 2." Version 2 exists only as a header in docs/package-api.md. No code records or checks it. The api-v1 tag is in another repository, so I could not check it.
  • P3. Background, Runtime Scripts: compile TypeScript authoring output to Lua #320: "CI typechecks TypeScript." The new root tsconfig.json (packages/*/scripts/*.ts) and .luarc.json do not run in .github/workflows/runtime-scripts.yml.

(b) Not asked for

  • P2. build-scripts.ts:282 adds a new name check, ^[a-z0-9_.-]{1,64}$. A source whose file stem has an uppercase letter or another character now fails to build, and nothing in the docs or the changelog says so. This conflicts with "existing directives remain supported".
  • P3. ask_artifact_offline is new in the Package API (docs/package-api.md:267, module/src/package_test.rs). The description does not mention it.
  • P3. The code that recovers identities reads every *.json file in data/.generated/, not only the canonical artifact (script-ids.ts:51-84). An extra file in that folder can change or stop ID allocation.

Not checked: sourceHash (build-scripts.ts:178) now includes script-ids.json. It must match the CLI Build Identity from lyracore-cli#72 at the pinned revision.


Summary: Standards has 13 findings and 1 nit; the worst is the P1 use of "player" as a noun in the new public authoring API. Spec has 9 findings; the worst are the P2 cross-Package script ID clashes and the P2 legacy Lua scripts that are refused because they call on a global events.


Generated by Claude Code

@Mathih13

Copy link
Copy Markdown
Contributor Author

Addressed in the latest commits:

  • New IDs reserve saved identities, legacy headers and prebuilt artifacts across the enabled Package Inventory, including Package directory symlinks. Tests cover cross-Package collisions and retained IDs. Published conflicts name both owners and refuse; no stored ID changes silently.
  • Legacy directives now select the legacy source contract before binding parsing. A legacy Lua source can assign and call a global events table unchanged.
  • The agreed public events.player / PlayerLoginEvent API remains. CONTEXT.md and CORE_TERMS.md now distinguish that public authoring vocabulary from Core's Character terminology; the new internal payload flag uses character.
  • Renamed EventRegistrationOptions and catalogue registration to Event Binding terms. Removed the redundant required flag and shared the legacy header reader and ID range authority.
  • Split success and failure behavior tests. Added the missing payload notes, top-level Staged Effect semantics, naming constraints and legacy support/removal condition. The PR description now names ask_artifact_offline and Packages teleport_player cross-map branch leaks a game_entity_motion row per hop #80.

Some findings describe existing contracts or required compatibility work:

  • The lowercase/64-character name constraint already exists in lyracore-package-delta::ScriptName; the compiler now reports it before emitting an unusable artifact.
  • Noncanonical artifact discovery is required. The CLI already accepts files such as personality.json, and migration must retain their IDs.
  • The CLI reads the Package API header in src/cmd/packages/official.rs to select api-v<N>. Packages teleport_player cross-map branch leaks a game_entity_motion row per hop #80 targets this Core revision; its publication job will update api-v2 while retaining api-v1.
  • The CLI and compiler source hashes were checked together by actual packages build / packages check, and by an independent review of the recorded hashes.
  • The TS and Lua readers keep language-specific AST checks. They already share final Event Binding validation; another parser adapter would add structure without removing those AST differences. Small local error helpers remain local for the same reason.
  • New payload fields use camelCase, while existing Entity Handle fields retain their names for compatibility. The root editor configuration is for installed Packages; CI exercises actual TS compilation, negative type cases and generated declaration freshness through Bun tests.

The follow-up adversarial review found no blockers. Validation: 76 Bun tests, 1,566 Module unit tests, workspace Clippy and the Wasm check passed. All five durable Runtime Script tests also passed before the internal flag rename and test split; focused Host tests passed afterward.

@Mathih13
Mathih13 merged commit 46bf948 into main Oct 10, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant