Plan Lua plugin scripting with built-ins as the first plugins - #14
Merged
Conversation
Adds docs/plans/lua-plugins.md: a contribution seam for Lua plugins, scoped in v1 to declarative contributions only — context menu items, toolbar buttons, commands, repo event handlers, and per-plugin localization files. No widget trees; the constrained panel schema that would replace them is deferred behind the same parse-at-the-boundary design, so nothing in v1 blocks it. Records the decisions that are expensive to revisit: KeraLua over MoonSharp (reflection interop does not survive Native AOT), menu items parsed into a sum type before plugins can construct one, a closed anchor enum with frozen ctx schemas, and an injected enumerable registry rather than an ambient one. Dogfoods the seam by converting four shipped features into bundled plugins — shell-tools, open-remote, copy-paths, tag-actions — each proving a different part of the API. Their strings move out of the seven app locale files into per-plugin string tables, so plugin localization ships covering real translations rather than an English-only stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Restructures the plan around isolation. Four projects with the contracts in the middle: GitBench.Extensibility depends on nothing, and the app and the plugin host both depend on it and never on each other. Contracts run in both directions — inbound extension interfaces the app consumes, and outbound host ports the plugins call — so neither side can reach into the other. States the property to verify rather than describing isolation in prose: deleting two project references and one PluginBootstrap.Install line leaves an app that compiles and behaves identically. Adds the CI architecture test that keeps it true, since Rule 2 asks for boundaries that are machine-checked rather than reviewed. Budgets the blast radius honestly: nine main-app edits and one new adapter file, with the shape of a feature-side touch spelled out. Drops the Item sum-type refactor from the critical path. Twenty-nine files touch that type, and the invariant it was protecting is already enforced at the parse boundary — plugins produce a sum type that the host projects onto Item, so no illegal Item can originate from Lua whatever the type permits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Notes written while reviewing docs/plans/lua-plugins.md: repo identity is Guid rather than path, toolbar labels and enabled state are reactive so snapshot contributions regress, open-in-terminal already means two different things across anchors, and localized strings are source-generated with keys shared across features. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Corrects errors verified against the code. The Runtime decision cited glfw-static-linking.md as supporting precedent when its first line reads NOT VIABLE AS WRITTEN, and cited oniguruma as static-linking precedent when it is a per-RID dynamic native that same document puts explicitly out of scope. Static linking leaves v1; KeraLua's dynamic native is the baseline. Four RIDs on four runners, not three platforms. Adds two prerequisite phases. There is no PR-time CI at all — one tag-triggered workflow — so the architecture test three sections depend on has nothing to run it. And contracts no longer freeze before a measured Lua-under-AOT spike, because StartupHealth.cs states a managed exception unwinding a native callback frame fail-fasts under NativeAOT, and Lua raises by longjmp, which together decide whether the synchronous-with-deadline threading model is available at all. Fixes the contracts. Repo identity is Guid everywhere in this app, but five of six menu contexts carried no repo and the sixth used a path string, so contributions now travel in a MenuTarget envelope. Menu extensions return contributions the app merges with a pure function, rather than receiving and returning the item list — a plugin returning an empty list previously deleted the built-in items. Contributions carry thunks rather than snapshots, because toolbar labels are Props bound to follow a locale switch without a rebuild. Ports become intent-shaped: writes route to the flows that already own their dialogs, and open-in-terminal is one port because it already means two things. Splits the deletion test into a property that holds forever and is CI-checkable, and one that expires when bundled plugins own shipped features. Reorders the conversions to put the only true transliteration first. Revises the string migration from ~15 keys to 25-30, makes it two-phase for reversibility, and budgets the CLDR plural resolver that seven locales need at runtime — Arabic carries five categories. Adds sections the plan lacked: threading and error propagation, crash containment against the RecoveryUpdater loop, testing, API versioning, and rollback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Plurals leave the critical path. Measured: none of the ~16 keys the bundled plugins own is pluralized — every plural key in the catalog belongs to a stage/unstage/discard/stash item that stays in C# as a core git verb. v1 ships no plural support and rejects a plural object at load; the CLDR resolver waits for a caller that needs it. Shared keys are referenced rather than copied. Plugins read the app catalog as @app/<key>, so common.open_folder stays in the app where its three call sites live instead of being duplicated into seven locales of drift. This also makes the migration two-phase almost for free: a conversion ships with no string table at all, and private keys move a release later. The pseudo locale is generated at runtime from the same transform rather than shipped, so plugin strings stay in the layout-QA pass and no plugin author writes a pseudo.json. Revises the migration from 25-30 keys to ~16. Shared keys now stay put, and routing tag-actions through the app's own DeleteTagDialog keeps five of that dialog's six strings in the app. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Reverses the decision to defer plural support. A string-table format
cannot gain plurals later without breaking, and the first third-party
plugin that needs one would hit a wall.
The real hazard is not implementing plural selection but implementing the
wrong one. Measured: ja, ko and zh-Hans carry meaningfully different one
and other forms — files.stage is "ステージ" against
"{count}個のファイルをステージ" — while CLDR gives those languages only
"other". The app is using a simplified n == 1 selector with extra
categories for Arabic and Russian, so a textbook CLDR resolver in the
plugin path would silently disagree with the app in languages nobody on
the team reads.
The design extracts the app's real rule empirically rather than reading
the generator in the framework submodule or assuming CLDR, implements it
as a table in GitBench.Extensibility, and pins the two implementations
together with a differential conformance test over every plural key,
every locale and counts 0-200. Missing categories fail at the parse
boundary rather than rendering blank.
Bounded by design: the resolver needs rules only for the members of
Locale, so it grows when the app gains a language, which already touches
the catalogs.
Also notes an incidental find — ar.json carries review.context_stage and
review.context_unstage, absent from every other catalog.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Checking out the framework submodule showed the previous plural design
was solving a problem that does not exist. Plural selection is already a
runtime call: the generator bakes only the forms into a PluralForms
struct and emits string.Format(_culture, PluralRules.Select(...), count),
and PluralRules is public API in ZGF.Gui, which GitBench already
references. The empirical extraction, the reimplemented rule table and
the differential conformance test all collapse into calling it.
PluginStrings lives app-side, so GitBench.Extensibility still depends on
nothing.
Corrects the rule itself. Russian and Arabic are verbatim CLDR, not extra
categories bolted onto a simplified selector, and there is an fr/pt rule
for languages the app does not yet ship — so it was never a
seven-locale table to re-derive. The CJK reasoning was right: ja, ko and
zh do take n == 1, deliberately departing from CLDR, and a test pins it.
Fixes load validation to reuse the generator's own required-category
table rather than a stricter invented one. The previous rule would have
demanded an Arabic zero form that no catalog in this app ships.
Records that interpolation must go through string.Format with the
culture, that Russian's other form is unreachable for integer counts,
and that the pseudo transform runs on the positional string after
{count} becomes {0}, so a naive runtime pass computes a different pad.
Flags the ~16-key move list as provisional: sharing is the norm, and
commits.context_delete_tag is itself shared across two call sites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
No behavioural change to the design — this removes the accumulated revision commentary and states each decision once, on its merits. Git carries the history. The localization section is the part that shrank most. Plugins load their string files into a per-language dictionary, reference shared app keys as @app/<key>, and hand plural forms to ZGF.Gui.Localization.PluralRules.Select — the same runtime call the generated Strings makes, in a library GitBench already references. Load validation reuses the generator's own required-category table rather than a stricter one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Records KeraLua 1.4.9 and what its package actually contains: natives for all four release RIDs, with macOS served by a single osx folder holding a universal binary that RID fallback resolves for both arm64 and x64. Documents the binding layer — one lua_State per plugin, the gitbench global built from delegates rooted on the host object, one total wrapper converting faults to Lua errors, text-only chunk loading, and a single walk of the returned table into contract records. Rejects NLua alongside MoonSharp, with the reason each fails: NLua adds the reflection-based object binding on top of KeraLua, and we hand-write bindings regardless; MoonSharp can be made AOT-viable through hardwired descriptors, but that is a codegen step to maintain. Corrects the error-containment wording. KeraLua marshals callbacks with Marshal.GetFunctionPointerForDelegate rather than UnmanagedCallersOnly, so the rule is that no managed exception may unwind into native Lua, and every delegate must be held in a field for the lifetime of its state — the returned pointer does not keep it alive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Six checks over a real NativeAOT publish: that the native loads, that a managed exception unwinding into Lua is survivable, that a lua_error beneath a managed frame is catchable and whether finally still runs, that an [UnmanagedCallersOnly] callback works against the same native, that a count hook can abort a runaway script, and what one interpreter boot costs against the 15 ms budget. Exits non-zero on failure so it can run across all four release RIDs in CI. Q2c and Q3 use hand-rolled bindings rather than KeraLua, because KeraLua's API is shaped around instance delegates marshalled with Marshal.GetFunctionPointerForDelegate and cannot express a static delegate* unmanaged. That comparison decides whether to use KeraLua or only its native and own the bindings. Kept out of GitBench.sln so it cannot affect the app build. Measured while writing it: Lua 5.4.7 builds to a 313 KB shared library in 5.2 seconds from 33 dependency-free source files, so owning the native side is not the GLFW situation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Drops KeraLua for a hand-written P/Invoke surface over the Lua 5.4 C API. The deciding factor is [UnmanagedCallersOnly]: it takes a static method and a plain function pointer, so there is no delegate for the GC to collect while Lua holds a pointer to it and no reliance on Marshal.GetFunctionPointerForDelegate, which KeraLua uses and ships with no AOT claim. KeraLua's API is shaped around instance delegates and structurally cannot offer that path. The consequence to design around is that static callbacks cannot capture, so per-plugin context lives on the Lua side rather than in a C# closure. Puts static linking back in scope. glfw-static-linking.md rejected it because DirectPInvoke fails silently AND the shared native still shipped, so the fallback worked and the failure was invisible. Owning the build removes the second half: with no lua54 dynamic library shipped at all, a failed link is an unresolved load at the first Lua call. That review's mechanics still bind — the Import must sit in GitBench.csproj after its first PropertyGroup, item types are NativeSystemLibrary and NativeFramework rather than LinkerArg, and CI verifies the absence of a dynamic dependency rather than trusting a green publish. The Windows import-library collision it flagged does not apply, since we only ever produce the static archive. Vendors Lua source under vendor/lua alongside vendor/XtermSharp, built by scripts/build-lua.cs as a C# file-based app rather than a shell script and a PowerShell script that drift. Measured: 33 dependency-free files, one compiler invocation, 5.2 seconds. Records the two new costs — the Import is a third thing to delete for the removability property, and we now carry a C library and its security patches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Drops the KeraLua dependency entirely. Every call now goes through a hand-written LibraryImport surface against a statically linked Lua, so the spike tests the shape we intend to ship rather than an approximation of it. Callbacks are static [UnmanagedCallersOnly] methods pushed as function pointers, which is the whole reason for hand-rolling. Adds scripts/build-lua.cs, a C# file-based app that builds the static archive for the host RID on Windows, macOS and Linux from one implementation. It prefers vendor/lua and otherwise shallow-clones the pinned tag, excluding onelua.c, lua.c and luac.c. The csproj carries the DirectPInvoke and NativeLibrary items guarded on RuntimeIdentifier, with a build-time error if the archive is missing, and the README records how to verify the link rather than trusting a green publish — DirectPInvoke emits no diagnostic when it fails, and only the absence of a shipped dynamic native makes that failure loud. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Two roots: bundled plugins ship as Content next to the executable and are replaced wholesale by a Velopack update, while user plugins live under AppData and survive one. Records that release.yml copies the publish directory into Contents/MacOS on macOS, so bundled plugins resolve there rather than in Resources, and that AppPaths needs an AppDataDir helper next to its existing file-only AppDataPath. Pins the load order around the line that matters: manifests are read and contributions registered without creating a lua_State, and the interpreter appears only on first invocation of a contribution. Id collisions resolve bundled-wins, so a dropped-in folder cannot silently replace a shipped feature. Keeps disabled ids and the quarantine marker in the host's own files rather than Preferences, which is a serialized public record and would carry a plugin field as dead public API after removal — the convention Preferences.cs already argues for the assistant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Declares identity, commands, toolbar entries and menu entries as data, so the registry is built without an interpreter. Menu and toolbar entries carry a command id rather than their own handler, so one behaviour serves several surfaces — shell-tools implements "open a terminal here" once and surfaces it on the toolbar and two menu anchors. Adds the dynamic escape hatch and sharpens the loading rule with it. A contribution whose shape depends on context, like one submenu per tag on a commit, cannot be declared statically; it says so and the host calls Lua the first time that menu opens. So Lua loads on first invocation of a command or first open of a menu carrying a dynamic contribution — not the looser "first invocation" the plan claimed. Menus open on a user gesture, so that load stays off the startup path, and the toolbar, which does need frame one, is always static. when and enabled_when name predicates from a closed host-evaluated set with a leading ! for negation — no expression language, because that would be a second thing to sandbox. Being host-evaluated and reactive is what lets a plugin toolbar button grey out with no repo open, matching the built-ins, without loading Lua at all. Lists what the manifest parse rejects, including a string key absent from the plugin's own en.json, which otherwise surfaces as a blank menu item much later in a locale nobody tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Declaring contributions in plugin.json and implementing them in Lua made every item exist twice across two files that can drift, surfacing as menu entries that do nothing or handlers nothing invokes. The manifest now carries identity and api_version only — the two things that must be known before deciding to execute a plugin's code — and init.lua registers everything imperatively. This removes machinery rather than adding it. Conditional items are an if statement instead of a closed predicate vocabulary with its own parser and per-anchor validity rules. Dynamic items stop being a separate code path behind a "dynamic": true flag and become an ordinary loop in a builder. Reactive enablement still works because contributions already carry thunks, so a Lua function wrapped in an observable greys a toolbar button out with no repo open, at the cost of one Lua call per rebuild. The declarative design was justified by a startup cost that was never measured. Running every init.lua at load costs N interpreter boots, but luaL_newstate plus openlibs plus a small script is a fraction of a millisecond, and Phase 0.5 question 4 measures it. If it does come back over budget the answer is to move the work to a worker and let the toolbar pick contributions up through the observables it already binds to — the pattern AppHostSetup and AppServices already use for font fallbacks and repo sweeps — not to reintroduce the duplication. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
Plugins never see IMessageBus. Of the twenty message types, most are commands rather than events — ShowDialog, OpenDiffWindow and the optimistic-update trio are internal UI mechanics — and PullDiverged carries a whole Repo, so exposing the bus would hand plugins an app type through a back door. The decisive reason is that subscribing by message type would make every new message a silent widening of the plugin API. IPluginEvents publishes a closed set of eight domain events instead, each carrying a RepoRef, mapped by one adapter in the app. Delivery is asynchronous and cannot veto. Broadcast runs handlers on whatever thread broadcast, git work runs on Task.Run, and Lua is UI-thread only, so events marshal through IUiDispatcher and arrive after the broadcast completes. Handlers are notifications: a plugin that needs to affect an operation uses a contribution or an intent, and one that cares about current state re-reads rather than trusting the payload. WorkingTreeChanged arrives in file-watcher bursts, so delivery coalesces per event and repo within a dispatcher tick. The host holds one bus subscription per exposed event and fans out, so nothing touches the unlocked MessageBus dictionary per plugin. Also fixes two lines left stale by earlier revisions: the framing still called contributions declarative, and the out-of-scope row still listed static linking, which the Runtime and Native build decisions adopt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds docs/plans/lua-plugins.md: a contribution seam for Lua plugins,
scoped in v1 to declarative contributions only — context menu items,
toolbar buttons, commands, repo event handlers, and per-plugin
localization files. No widget trees; the constrained panel schema that
would replace them is deferred behind the same parse-at-the-boundary
design, so nothing in v1 blocks it.
Records the decisions that are expensive to revisit: KeraLua over
MoonSharp (reflection interop does not survive Native AOT), menu items
parsed into a sum type before plugins can construct one, a closed anchor
enum with frozen ctx schemas, and an injected enumerable registry rather
than an ambient one.
Dogfoods the seam by converting four shipped features into bundled
plugins — shell-tools, open-remote, copy-paths, tag-actions — each
proving a different part of the API. Their strings move out of the seven
app locale files into per-plugin string tables, so plugin localization
ships covering real translations rather than an English-only stub.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01RiwZW9LeDty5CWkgNUG4U3