Defects and Lessons¶

The do-not-reproduce list.

Every entry is a real defect found in legacy/applejack-old-final/, with the mechanism that prevents it recurring in the new codebase. When an Architecture/ document departs from legacy behaviour, it should be able to cite an entry here or an ADR.

This document exists because the most dangerous failure mode of a rewrite is faithfulness. A bug that has been in a codebase for fifteen years reads as intentional.

Severity key¶


🔴	Security or data-integrity issue
🟠	Player-visible correctness bug
🟡	Architectural hazard — no visible symptom, but it makes change dangerous
⚪	Waste — works correctly, costs more than it should

🔴 D-01 — SQL built by string concatenation¶

metatables/sv_player.lua:627-750. Save statements are assembled by concatenating values into INSERT INTO … VALUES ("…") and UPDATE … SET k = "v" strings. Only two columns are escaped — _Name and _Clan (libraries/sv_player.lua, player.saveEscapeKeys).

Every other player-influenced field is interpolated raw. Any value that can contain a quote is an injection vector against the character database.

Prevention. No hand-built query strings anywhere in the new codebase. Persistence goes through IPersistenceBackend, which serialises typed objects — the JSON backend has no query language at all, and the HTTP backend sends a JSON body to a parameterised endpoint. See Architecture/06_PERSISTENCE.md and ADR-0003.

🔴 D-02 — No server-side validation layer¶

Every client→server action funnels through a single concommand.Add("cider", …) dispatcher (libraries/sv_commands.lua) taking raw string arguments. The dispatcher checks argument count and access flags, then pcalls the handler. There is no type validation, no range validation, no schema, and rate limiting is ad-hoc per-command timestamps (_Next*).

Prevention. Every client request is an [Rpc.Host] call with typed parameters, and the host re-validates authority, state and rate before mutating anything. Typed parameters remove an entire class of parsing bug for free. See Architecture/07_NETWORKING.md.

🟠 D-03 — Per-item carry limits never work¶

libraries/sv_inventory.lua:25-26:

if not force and item.max and player.cider._Inventory[id] + amount > item.max then
    return false,"You can't carry any more "..item.plural.."!"

It reads item.max and item.plural — lowercase. Every item definition in the game sets ITEM.Max and ITEM.Plural — capitalised (items/base/vehicle.lua:6, items/illegal_goods/headcrab.lua:12, and all 40+ ITEM.Plural definitions). Lua tables are case-sensitive, so item.max is always nil and the entire branch is dead code.

Effect: no item has ever had a working stack limit, except items/misc/boxed_pocket.lua which reimplements the check by hand against self.Max (:22). Vehicles were meant to cap at 1; they never did.

Prevention. Definitions are typed C# properties on a GameResource, not string keys in a dictionary. A casing mistake is a compile error. Stack limits get an explicit unit test.

🟠 D-04 — Multi-base item inheritance silently does nothing¶

metatables/sh_item.lua:Register():

if (type(self.Base) == "table") then
    for _,id in ipairs(self.base) do        -- ← self.base, not self.Base
        table.Merge(base, GM.Items[id] or {});
    end

The branch tests self.Base and then iterates self.base. Any item declaring an array of bases inherits nothing and falls back to whatever defaults it happens to set itself. No error is raised.

Prevention. Inheritance is explicit, resolved once at load, and validated: an unresolvable base is a hard load failure with a named asset, not a silent empty merge. See Architecture/09_ITEM_FRAMEWORK.md.

🟠 D-05 — Inventory desync is permanent until relog¶

Client inventory state (libraries/cl_inventory.lua, cider.inventory.stored) is rebuilt purely from incremental cider_Inventory_Item messages. There is no periodic reconciliation and no full-state resync except the one-shot replay on PlayerInitialized.

Usermessages are unreliable in practice. One dropped message and the client's view of its own inventory is wrong for the rest of the session — and because the client UI drives what the player asks to do, this produces confusing failed actions rather than an obvious error.

Prevention. Inventory is host-owned [Sync] state with a defined authoritative representation, not a stream of deltas the client integrates. A late-joining or recovering client converges. See Architecture/08_DATA_MODEL.md.

🟠 D-06 — 64-byte message limit silently truncates¶

Usermessages cap at 64 bytes. Chat lines, item descriptions, door names and player names are all sent as strings through them with no length check at the call site. Long values are truncated without warning.

Prevention. Strings are validated and length-bounded at the point of entry (a door name has a documented maximum), and the transport no longer has an arbitrary frame cap.

🟡 D-07 — The global hook dispatcher is monkey-patched¶

libraries/sh_plugin.lua replaces hook.Call itself. Every hook invocation in the entire game — including Sandbox's and the engine's — is routed through a function that iterates every registered plugin table, looks the hook name up as a method, and pcalls it. The first non-nil return short-circuits and is returned to the caller before the gamemode's own handler runs at all.

Consequences: plugin execution order is table-iteration order (i.e. undefined); a plugin can silently suppress core behaviour by accidentally returning a value; every hook in the game pays the cost of the plugin scan; and debugging a hook means understanding a patched dispatcher.

Prevention. A typed event bus with explicit subscription, documented ordering, and explicit cancellation — a handler that wants to suppress default behaviour must say so, not accidentally return something truthy. Nothing global is ever replaced. See Architecture/04_EVENT_BUS.md and ADR-0002.

🟡 D-08 — Load order is load-bearing¶

GM:LoadPlugins() runs before GM:LoadItems(), which runs before sh_jobs.lua, because item files call GM:GetPlugin("hunger") at include time and job definitions reference item categories. There is no dependency declaration; the ordering is implicit in sh_init.lua and breaks silently when disturbed.

GM:GetPlugin(id) compounds this by doing fuzzy name matching — a lookup can resolve to the wrong plugin as the plugin set grows.

Prevention. Modules declare dependencies; the loader topologically sorts them and fails loudly on a cycle or a missing dependency. Lookups are by type, not by fuzzy string.

🟡 D-09 — Character inventories and containers are two systems¶

Character inventory is ply.cider._Inventory handled by libraries/sv_inventory.lua. Container inventory is entity._Inventory handled by libraries/sv_container.lua. They use the same Size-based capacity maths — reimplemented separately — but have different mutation paths, different networking, different hooks and subtly different rules.

Every feature touching storage has to be written twice, and the two drift.

Prevention. One inventory model, used by characters, containers, vehicles and corpses alike. See ADR-0006.

🟡 D-10 — Items have no identity¶

_Inventory is { itemUniqueID = count }. Two "AK-47"s are the same integer. There is therefore no possible representation of durability, remaining ammunition, a container's contents, a signed document, a named object, or anything unique.

This is the single most limiting decision in the original. Nearly every "we can't do that" in the legacy design traces back to it, and it is why items/misc/money.lua had to become a pseudo-item that intercepts its own storage (D-12).

Prevention. ADR-0006 — item instances with identity and per-instance state. Stacking becomes an explicit opt-in property of a definition rather than an unavoidable property of the storage shape.

⚪ D-11 — Networked variables rebroadcast unconditionally, forever¶

player.AddAutoCSVar registers a variable that is re-sent to its owner every second, whether or not it changed (libraries/sv_player.lua, the 0.1s master tick). Thirteen variables are registered (_Money, _Salary, _Gender, _Sleeping, _GPS, _Stunned, _JobTimeLimit, …).

Cost is constant and proportional to players × 13, regardless of activity. A server full of players standing still pays exactly as much as one in a firefight.

Prevention. [Sync] replicates on change. Do not build a polling loop on top of it. Any periodic broadcast in the new codebase needs a written justification.

⚪ D-12 — Money is a fake item that intercepts its own storage¶

items/misc/money.lua is registered as an inventory item, but its onUpdate callback intercepts every write, batches the change through _TMPMoneyUpdate on a 0.1s timer, and returns true so the generic inventory code never actually stores it. Money lives in ply.cider._Money.

The mechanism works, but it means the inventory system has a special case it doesn't know about, expressed as an item that lies about being an item.

Lesson. The escape hatch (onUpdate may pre-empt the system) is what made this possible. Escape hatches get used. The new item framework should make currency either a genuine item or genuinely not one — not a third thing.

⚪ D-13 — Every library file executes twice¶

sh_init.lua includes libraries/ with an explicit prefix-dispatched loop at :105, then again via the generic doload("libraries/") at :157. Every library runs twice per boot. It is survivable only because the libraries happen to be idempotent — which is luck, not design.

Prevention. One loader. Module initialisation is explicitly once, and re-entry is an error rather than a coin flip.

⚪ D-14 — God files¶

File	Size	Holds
`sv_commands.lua`	58 KB	~66 unrelated command implementations
`init.lua`	47 KB	Bootstrap, payday loop, damage model, spawn logic
`cl_init.lua`	36 KB	Client bootstrap, HUD, ESP, view modification
`libraries/cl_chatbox.lua`	924 lines	An entire custom VGUI chat client
`metatables/sv_player.lua`	965 lines	The whole character model and its persistence
`libraries/sv_entity.lua`	812 lines	~40 functions of ownership and door logic

Prevention. Standards/00_CODING_STANDARDS.md — 500 lines preferred, 1000 absolute maximum per file, one responsibility per class, enforced at review.

⚪ D-15 — Three unrelated persistence backends¶

Players → MySQL via tmysql
Per-map plugin data (doors, spawn points, prison points) → flat files with glon
Prop protection → local SQLite via sql.Query

Three formats, three failure modes, three backup procedures, no shared versioning or migration story anywhere.

Prevention. One IPersistenceBackend abstraction with a versioned document envelope and a migration chain, and all persistent state goes through it. See Architecture/06_PERSISTENCE.md.

⚪ D-16 — Entity state packed into an untyped bitfield¶

Door and container state lives in ent:GetDTInt(3) as bit flags — OBJ_LOCKED=1, OBJ_SEALED=2, OBJ_PADLOCKED=4, OBJ_CONTAINER=8, OBJ_INUSE=16 — alongside stringly-typed NWString "Name" and NWString "Warrant". There is no schema; the meaning of slot 3 is convention held in two files that must agree.

Prevention. State is typed [Sync] properties on components with names. If a bitfield is ever justified on bandwidth grounds, it is a private implementation detail behind a typed property, and the justification is written down.

The meta-lesson¶

Read the defects above and notice how many share one root cause: the language let a mistake be silent. self.base vs self.Base, item.max vs item.Max, a stray truthy return suppressing a hook, an unvalidated string reaching SQL.

The strongest single argument for the C# rewrite is not performance or engine features. It is that most of this list would not compile.