Move the scenario onto exp_roles and remove the legacy role system

[bbassie]

Part two of #448. Moves every call site off expcore.roles, deletes the legacy module and config, simplifies jail, and seeds the roles on first run. Taking "improve the role api with no restraint, no external consumers other than the mono repo" at its word, the lua api is redesigned rather than kept compatible, and the permission names are tidied up while nothing holds them yet.

Lua api

The legacy action strings and the transform which mapped them onto permission names are gone. Call sites check the clusterio permission directly, so grep exp_scenario.command.kill finds the definition, the check, and the seed, and there is no second transform which silently breaks every check if it drifts.

Before	After
player_allowed(p, "command/kill")	player_has_permission(p, "exp_scenario.command.kill")
player_has_flag(p, "report-immune")	player_has_permission(p, "exp_scenario.bypass.reports")
define_flag_trigger(flag, fn)	define_permission_trigger(permission, fn) — any permission can have one
events.on_role_assigned / on_role_unassigned	events.on_player_roles_changed with assigned and unassigned names
get_player_highest_role(a).index < get_player_highest_role(b).index	player_outranks(a, b), plus player_outranks_role(a, role)
get_role_from_any / get_role_by_name / get_roles_ordered	get_role(name | id | role) / get_roles()
config.order / config.roles / config.players	removed; role:get_player_names() covers the one use
assign_player(p, roles, by, skip_checks, silent)	assign_player(p, roles, by, silent)

Flags were only permissions with a change trigger, so the separate concept goes. Every consumer registered both role events for the same handler, so they become one, and it is now also raised when the roles a player holds change on the controller, or when a role they hold is edited there. The old events were never raised for either, which left guis stale after a web ui change. player_outranks applies the core.admin bypass consistently; two of the six index comparisons did not.

A role now carries a permission group, which the legacy system mapped roles to and #448 dropped. A player is moved into the group of their most privileged role which names one, which is how Jail actually restricts a player. It sits with the other in game properties on the role page.

Permission names

Explicit list in exp_scenario/permissions.ts, no transform. The action and flag buckets only reflected how the old config was written, so: exp_scenario.bypass.* (entity protection, nuke protection, deconstruction log, reports), exp_scenario.decon.* (standard, fast trees — with descriptions that say what they gate), exp_scenario.player.* (admin, spectator, instant respawn, system commands), exp_scenario.chat.commands, and exp_scenario.gui.player_list.kick / .ban for the player list buttons which were never commands. command.* and gui.* are unchanged apart from clear_tag.always → tag_clear.always to match the command it belongs to.

Commands derive exp_scenario.command.<name> in the authority, so assign-role, unassign-role and get-roles get scenario permissions rather than the core ones #448 mapped them to. The in game command is bounded by the lower role check, core.user.update_roles is not — granting it to Moderator would have let any mod with a web account change anyone's roles.

Dropped: defer_role_changes (priority replaced it), command/give-warning (no role held it; the player list now keys the warn button on create_warning, the permission the command already needed) and command/report (never defined; now create_report). Both buttons were only ever visible to root. _ipc and _sudo are added so every command has a definition.

Jail

jail_player gives the Jail role, unjail_player takes it away. Jail.old_roles and the stash/restore are gone, since priority suppresses the other roles for as long as the role is held.

Seeding

On the first run (empty role properties datastore) the controller creates the roles the scenario shipped with and gives the listed players their roles. The seed keeps the parent relationships from the old config and flattens them, since clusterio roles do not inherit; the flattened sets were diffed against the legacy config and match for every role. Cluster Admin and the default role already exist, so those entries only set properties — the default role gets its permissions through grantByDefault, which is checked to equal the old Guest list. Roles which already exist by name are reused and only gain permissions, so seeding a cluster which already has some is safe.

Verification

• 118 behavioural assertions over the real module under a stubbed environment: lookup, priority suppression, ranking, holders, every sync entry point, pending confirmation, rejection rollback, local only roles, events and messages for both local and controller originated changes, triggers and permission groups, jail.
• End to end in a dev cluster: seed on controller start (15 roles, 38 users), instance on Factorio 2.1.14 starts with the full scenario, permission checks over rcon resolve as expected, in game assignment confirmed by the controller, assignment for an unknown user rejected and rolled back, ctl user set-roles reaching the game.
• emmylua lint as ci runs it: 0 findings. tsc and the web bundle clean.

Notes

• exp_scenario.command.kill.always and spawn.always stay defined; /kill never checked kill.always before either, its lower role parser already bounds it. Left for a later tidy.
• The seed data is the old config, staff list included, so nothing new is in the repo.
• Web ui screenshots are not included — the controller run --dev web build is not set up here, the form only gains one text field next to the existing ones.

🤖 Generated with Claude Code

[Cooldude2606]

Along with all the changes highlighted in this review; can you also weigh the pros and cons of using oop for the roles vs only using module level methods. At the moment there are a few places where we have mutliple ways to do things or where things can be cumbersome, so we should align on a strong dx api interface for working with roles and covering the use cases we have or would expect.

[bbassie]

Pushed bea8d46 covering every point. The api question first, since it decided the shape of the rest.

Object methods vs module functions

I weighed it as: module functions are easiest when the role is incidental to the call — a permission check where the caller has a player and a permission name and never thinks about roles, or anything that spans roles such as ordering. Methods are easiest when the caller already holds a role, because there is only one way to say it and the role cannot be given three ways (name, id, object) as it could before. The old api mixed both, which is where the duplication came from: assign_player and assign_player_local, player_has_role next to role:get_players, get_role_from_any accepting anything.

So the split is now by what the caller has in hand:

• Module functions take a player or nothing and answer questions about players or the role list: get_role(id), get_role_by_name(name), get_roles(), get_default_role(), get_higher_roles(role), get_lower_roles(role), get_player_roles(player), get_player_highest_role(player), player_has_permission(player, permission), player_outranks(player, other), define_permission_trigger(permission, fn).
• Everything done to or with a role is a method: role:assign(player, options), role:unassign(player, options), role:has_player(player), role:has_permission(permission), role:is_higher_than(other), role:is_lower_than(other), role:get_players(online), role:get_player_names(), role:print(message).

There is one assignment entry point, role:assign(player, { by_player_name, silent, local_only }), with local only as an option rather than a second function. Players are LuaPlayer only, nil or index 0 for the server. Roles are looked up by id; get_role_by_name is a search over the list and is used by the few places which only know a name — the configs, jail, and the role parser — which I think is the right trade rather than maps to keep in step. The name map, the ordered list and the index field are gone; get_roles() sorts on demand and comparisons go through the methods, order stays on the object as data but is documented as not for comparing.

The one cost of methods worth naming: a call site that wants the roles of a player and then something about each of them now does two steps, get_player_roles then a method, where a flat player_has_role(player, "Jail") was one. Jail is the only place that pattern existed and it reads fine as jail_role():has_player(player).

The rest

• Permission groups removed from roles, the web form and the seed; exp_groups owns that mapping. Lua no longer touches game.permissions.
• Seed is a SeedRolesRequest (core.role.create) behind a "Seed roles" button in a section on the roles page, roles and permissions only, no assignments. Existing roles are matched by name and only gain permissions, so it is safe to press again. Verified against the dev cluster by deleting a role and seeding: it came back, the rest were untouched. The seed lists each permission once at the lowest role which has it (player.admin, player.spectator and bypass.reports on Trainee, instant_respawn on Moderator and Sponsor) and the parent chain carries it up; the flattened sets still match the legacy config for every role.
• System commands unlock on core.admin, and exp_scenario.player.system_commands is gone. Note this means Senior Administrator can no longer use /_rcon unless they also hold Cluster Admin, which the old is_system flag allowed — I take that as intended.
• Player list auth uses Roles.player_outranks directly. The get_higher_roles callers (reports, protected entities) loop with role:print.
• The event now carries role ids in assigned / unassigned rather than names, to match.
• While there I registered the role metatable with Storage.register_metatable: the role records live in storage and would have lost their methods on load.

Tests updated for the new api, 108 assertions passing; emmylua still 0 findings; tsc and the web bundle clean; the instance starts with the full scenario and the methods answer as expected over rcon.

[Cooldude2606]

You mention some tests you are running, do you think those could be added to a test dir within this plugin? We are planning to move away from mocha so can you write them using tap. Please attempt to keep all tests independent, although instance reuse is allowed so long as a method is used fetch / start the instance on its first use rather than having a magic global process always running.

[Cooldude2606]

To avoid importing storage we can unroll this and also avoid the module name call by using the plugin name.

[Cooldude2606]

In a number of places we return sorted roles when there is little reason to. This is an extra cost we can remove. In the places where we need it sorted we can expose an ordered roles method or can optimise further in other ways.

[Cooldude2606]

This is just seen from held role ids. Becuase this is an internal method let's just return array, set from the one method as we get both for the same cost.

[Cooldude2606]

This doesnt work becuase a player can be in both lists.

[Cooldude2606]

Although not used yet, let's add "has any" and "has all" the same has how the webui has it.

[bbassie]

Pushed 8a15745 and e79822b covering all six points.

The api changes

• player_has_any_permission and player_has_all_permission, varargs like the web ui's account checks, with the same edge semantics: any of none is false, all of none is true.
• Nothing sorts on the way out anymore. get_ordered_roles() and a public sort_roles(roles) cover the three places which present roles in order (the player list gui, /get-roles, the readme welcome line); the highest role is found with a single scan; get_higher_roles / get_lower_roles and the per-check get_player_roles return unsorted.
• get_held_role_ids returns the list and the set it was already building, and the second function is gone.
• get_player_names collects into a set before listing. You were right to stop on it — the old guard did dedup the both-lists case, but only if you read the placement of seen very carefully, and the test suite now pins the case: a role held in both the synced and the local list (local only assign, then the controller assigns the same role) counts once.
• Metatable registered directly as script.register_metatable("exp_roles.Role", ...), storage import dropped.

Tests

exp_roles/test/, tap as the runner: pnpm --filter @expcluster/roles test (needs the workspace built first for the seed and message tests, which read from dist). 1092 assertions.

The lua module is tested for real rather than reimplemented: test/helpers/lua.js creates a lua state on first use per test file (fengari, so no lua install needed), stubs the factorio surface and the three clusterio modules, and loads module/control.lua from disk. Each of the five lua files — lookup, players, assignment, sync, holders — gets its own state, so they are fully independent; the environment is passed to each chunk as an argument, no globals, which also keeps the lint clean over the test files. The javascript side covers the message records round tripping through their typebox schemas, the indexed permission encoding, and the seed: every permission it grants must be defined by exp_scenario, and every parent must exist with its permissions carried into its children.

Writing them caught one real thing beyond the get_player_names case: my helper originally returned assert(role, message) as a tail call, and assert returns all its arguments, so the message leaked as an extra value in table constructors. The module itself already guarded against this in get_player_highest_role, worth knowing as a pattern.

These don't run in this repo's ci since the workspace @clusterio/* deps only resolve inside the parent monorepo — same reason there is no js ci today. If you want them wired up once that changes, happy to.

Lint 0 findings, tsc and the web bundle clean, all tests passing.

[Cooldude2606]

For the lua side of the test harness can we do the following two things:

• Add a env.test so we can name specific tests and pass env as a param to the test function, allowing a reset each time.
• Make generic stubs and move the file into a folder in the root of the mono repo to allow reuse between plugins. There should still allow a custom env.lua to load additional stubs fpr tests.
  For the js tests, can we have coverage of the various plugins parts such as controller and instance. We do not need coverage of the webui.

[bbassie]

Pushed 59a0e0b and 24d36db covering all five points.

Harness

The generic parts now live in test/ at the repository root, ready for other plugins: test/lua/stubs.lua (the factorio surface and the clusterio module stubs, recording what the module does to them), test/lua/framework.lua (the test registry and check helpers), test/lua/runner.js (the fengari runner), and test/common.js, which is clusterio's testMatrix / testRoundTripJsonSerialisable pair. A plugin composes them from its own test/lua/env.lua — exp_roles' one adds its fixtures and loads its module, and extends the stub require table, so another plugin only needs its own env.lua.

Tests are declared as Test.test(name, function(env) ... end) and every test function receives a fresh environment, so nothing carries between them; a test which errors is reported as a failed check with the error as detail rather than taking the file down. Each stub raises on properties it does not implement — game.player is the one read allowed while unset, since the module checks it for the acting player. Test.deep_eq added, keys checked from both sides.

Coverage

• messages.test.js runs every record, event and request through the same matrix helpers as the clusterio tests, covering every optional field combination — 256 cases for the meta record alone.
• controller.test.js runs the real ControllerPlugin against faked controller internals (a memory datastore for the roles, fake users): property creation and orphan sweeping on init, record building and the default flag, broadcasts from role and property changes, subscription replay filtering, assignment validation and application, auto assignment with the blocking role and the on-leave trigger, and seeding twice to show reuse.
• instance.test.js runs the real InstancePlugin against a faked instance: the subscription and initialise flow with the indexed permission encoding asserted on the wire, the missing default role warning, sync mode gating in all three modes, forwarding of both update events, and the rejection rollback reaching lua.

The web ui is not covered, as requested.

Also

get_effective_roles builds its list in one pass — the list restarts when a higher priority is found and appends on a tie.

1147 assertions passing, lint 0 findings, tsc clean.

[Cooldude2606]

The test framework is very close now, still some refinement needed to get a good dx. As for the tests themselves can we structure them similar to clusterio core where we name them after the files / classes / methods (for both lua and ts) this makes it easy to check we have coverage and understand where new tests should be added. This of course only works for unit tests / stubbed tests; integration tests should be process based similar to what has currently been written.

[bbassie]

Pushed 7adcdde covering all seven points.

Naming between the parts

The three parts are now: Stubs (the shared factorio surface), the plugin's env.lua (the module specific extension, building environments and owning the fixtures), and a Suite of tests. The framework's registry is the suite, and it is created around the environment factory — Framework.new_suite(make_env) — so env.lua ends with exactly that call and the interaction between the parts is one line. The suite returned there becomes ... in each test file, and both env.lua and the test files now say so where it happens. "finish" is gone; a test file ends with return Suite.run().

Framework

pass(name) and fail(name, detail) are the primitives, check routes through them, and eq / deep_eq are now assertions rather than comparators — they record through pass/fail and a failure carries both values in the detail, which already made the test files shorter and the failures readable. names moved out of the shared framework onto the roles env, as it was role specific.

Stubs

• extend_requires, extend_script, extend_game, extend_defines, all backed by a recursive merge which adds missing values and raises when one already exists, so nothing mutates the tables directly.
• Strict labels carry the factorio class names: LuaBootstrap, LuaGameScript, LuaPlayer <name>, LuaPlayer <server>.
• script.register_metatable records what was registered, and stubs.save_load() copies the script data the way factorio saves it: functions are refused and only registered metatables survive the copy. The roles env wraps it as env.save_load() which also calls Roles.on_load(). The new test — "on_load restores the state after a save and load" — assigns roles, round trips, and checks methods and held roles still answer. It genuinely depends on the module registering its metatable: removing the script.register_metatable call from control.lua fails it.

Test naming

Both sides are named after what they cover, clusterio style. The lua tests read as get_ordered_roles returns the most privileged first and role:assign applies locally and is sent to the controller, so a scan of the names against the module is a coverage check. The javascript files wrap their tests in class ControllerPlugin / class InstancePlugin with subtests named per method (handleSeedRolesRequest creates the roles and reuses them by name), the message tests are one subtest per class, and module.test.js is renamed control.test.js after the file it covers. The dev cluster runs stay process based, as you said.

1159 assertions passing, lint 0 findings.

[Cooldude2606]

From looking closer at where this is used, names can be made more generic and included in the suite. But it should use more generic variable names to make it clear that it works for any table with a name property. Such as players, forces, surfaces, events, roles, commands, etc.

[Cooldude2606]

To work the same way as factorio we need to have a meta table on this that allows indexing by player id.

[Cooldude2606]

We dont need to store id seperatly, rather data.name should be set to the id, as well as data.tick to the current game tick.

[Cooldude2606]

We dont need this as the test should manually call on_load as this is inline with how we manually call other event handlers.

[Cooldude2606]

We should be using deep_eq for sent messages and factorio events.

[Cooldude2606]

Why do we use fake users and fake user manager when we import the real versions from controller. The controller itself can also be used directly without side effects if start is not called.