fix: load orchestrator through the plugin mechanism, not a static import - #121
Merged
Merged
Conversation
src/cli.ts (core) statically imported orchestratorPlugin straight from plugins/orchestrator's private internals via a relative path (../plugins/orchestrator/src/cli-plugin/index.ts) to make orchestrate work without requiring --plugin @game-ci/orchestrator-plugin. That's core depending directly on a plugin's source at compile time - every other plugin (including this same orchestrator, when loaded explicitly via --plugin) goes through PluginLoader's dynamic import instead, resolved by public package name, never a relative path into internals. Orchestrator now goes through the same PluginLoader.load() mechanism, by its public package name (@game-ci/orchestrator/cli-plugin) - it's just always in the default load list, alongside unity/godot/unreal. "Built-in" now means "always loaded", not "statically imported". Two things had to be fixed to make this work correctly, not just compile: 1. Root package.json never declared a dependency on @game-ci/orchestrator at all - which is *why* the code had to fall back to a raw relative path in the first place; there was no package-manager-created symlink to resolve the name through. Added it as a real workspace dependency. 2. @game-ci/orchestrator's package.json "exports" pointed only at its compiled dist/ output. Today's static import bundled straight from orchestrator's TypeScript source with zero separate build step - switching to the published dist/ path would have silently introduced a new build-order dependency (plugins/orchestrator's own `tsc` build would need to run before the root CLI's `bun build`) that isn't wired into any script or the release workflow today. Instead, added a "bun" export condition pointing at the TypeScript source directly - Bun's module resolver picks this over "default" automatically, so bundling behavior is unchanged (verified byte-identical: 1756 modules, 10.52MB, both with and without this fix, with zero pre-built orchestrator dist/ present) while non-Bun consumers of the published package still resolve to compiled dist/ as before. Verified functionally, not just structurally: `orchestrate <path> --targetPlatform=... --providerStrategy=local` produces byte-identical output on both the original static-import code and this fix (down to the exact same downstream git error for a nonexistent project path) - confirms the plugin's actual command/option wiring is unaffected, not just that it type-checks. Verification: - tsc --noEmit: 736 errors vs 737 on baseline (one fewer - removed a .ts-extension-import error, not a new one). - bun test ./src: 199 pass, 0 fail, including a new regression test confirming orchestrator is registered via PluginRegistry during default setup() and the existing "does not register built-in plugins more than once" test still passes for it. - bun run build: succeeds, byte-identical module count/bundle size to baseline. - oxfmt --check: clean.
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
7 tasks
frostebite
added a commit
that referenced
this pull request
Aug 24, 2026
New plugins/steam-deploy package, thin-wrapper-migrated from a real,
production Steam deployment action (deploy-to-steam/action.yml,
~1,070 lines) rather than reimplemented from scratch. Only the
genuinely portable Steam-domain logic was ported - deliberately
excluded everything gameclient-private:
Ported (real Steam-domain logic, generically useful to any studio):
- VDF generation (app build manifest + depot definition), including
the file-exclusion list reflecting real hard-won Unity-build
knowledge (Burst debug info, backup folders that shouldn't ship).
- Local-vs-Docker steamcmd execution, with Steam config dir mounting
for auth persistence in Docker mode.
- SteamCMD's output-parsing success/failure heuristic: exit code alone
isn't reliable (a dropped connection or depot failure can still
exit 0), so this reads "Successfully finished" / BuildID / known
error signatures from the actual output text, exactly as the
production script does.
Deliberately NOT ported (gameclient-private, does not belong in an
open-source plugin):
- Project-name auto-detection from path string matching and
hardcoded per-project Steam AppIDs.
- ProfileLoader.ps1/frameworks.yml integration.
- A custom git-checkout-with-broker-token fallback (unrelated to
Steam deployment anyway).
- A step posting build metadata to platform.frostebite.com, a private
internal dashboard.
- Hardcoded drive-letter/folder-convention build-path discovery -
replaced with an explicit --buildPath argument.
Command: `game-ci deploy steam <buildPath> --appId --depotId [--branch]
[--mode] [--steamCmdPath] [--steamConfigDir] [--extraExclusions]`.
STEAM_USERNAME/STEAM_PASSWORD read from environment only, never CLI
arguments (argv can leak through process listings).
Two small, generic (non-Steam-specific) extensions to the plugin
system were needed, since `deploy` is the first command with no
associated engine:
- PluginRegistry.createCommand now checks commandPlugins registered
with engine: '*' after exact-engine matches, mirroring
configureOptions' existing '*' handling for options plugins.
- CommandFactory special-cases `deploy` to skip engine detection
entirely (same pattern already used for build-unity-image) - a
deploy target's contents don't carry Unity/Godot/Unreal project
markers for detectEngine() to find.
- cli.ts's registerCommand middleware folds yargs' named `target`
positional (from `deploy <target> [buildPath]`) back into the
command array passed to CommandFactory, since yargs only puts
*undeclared* trailing tokens into `_` - a named positional never
lands there. Caught via a real functional smoke test (not just
types/unit tests): `deploy steam <path> --appId=... --depotId=...`
initially failed with "Unknown arguments: appId, depotId" because
configureOptions was silently never reached; fixed, then reverified
with the same command end-to-end (VDF files written with correct
paths, fails at the expected final step - no steamcmd installed on
this dev machine).
steam-deploy itself is loaded exactly like orchestrator - via
PluginLoader.load('@game-ci/steam-deploy'), never a static import -
matching the app/plugin boundary fixed in #121, not repeating that
mistake for a second plugin.
Verification:
- tsc --noEmit: 737 errors, matching baseline exactly (confirmed via
git stash -u comparison, correctly including the new untracked
plugin directory in the baseline).
- plugins/steam-deploy's own tsc --noEmit: clean.
- bun test ./src: 202 pass, 0 fail (was 199 before this commit),
including a new integration test confirming steam-deploy loads via
PluginLoader and `deploy steam` resolves without engine detection.
- plugins/steam-deploy's own vitest: 9/9 pass (VDF generation,
SteamCMD output-parsing heuristic - both the pure, most reusable,
most valuable logic from the original script).
- bun run build: succeeds.
- Real functional smoke test end-to-end (see above).
- oxfmt --check: clean.
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
1 participant
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.
Summary
src/cli.ts(core) statically importedorchestratorPluginstraight fromplugins/orchestrator's private internals via a relative path (../plugins/orchestrator/src/cli-plugin/index.ts) to makeorchestratework without requiring--plugin @game-ci/orchestrator-plugin. That's core depending directly on a plugin's source at compile time — every other plugin (including this same orchestrator, when loaded explicitly via--plugin) goes throughPluginLoader's dynamic import instead, resolved by public package name, never a relative path into internals. This came up while reviewing the repo's own architecture boundary: plugins must never be a direct/compile-time dependency of core, only loaded "using a mechanism."What changed
Orchestrator now goes through the same
PluginLoader.load()mechanism, by its public package name (@game-ci/orchestrator/cli-plugin) — it's just always in the default load list, alongside unity/godot/unreal. "Built-in" now means "always loaded", not "statically imported".Two things had to be fixed to make this work correctly, not just compile:
package.jsonnever declared a dependency on@game-ci/orchestratorat all — which is why the code had to fall back to a raw relative path in the first place; there was no package-manager-created symlink to resolve the name through. Added it as a real workspace dependency ("@game-ci/orchestrator": "workspace:*").@game-ci/orchestrator's package.json"exports"pointed only at its compileddist/output. Today's static import bundled straight from orchestrator's TypeScript source with zero separate build step — switching to the publisheddist/path would have silently introduced a new build-order dependency (plugins/orchestrator's owntscbuild would need to run before the root CLI'sbun build) that isn't wired into any script or the release workflow today. Instead, added a"bun"export condition pointing at the TypeScript source directly — Bun's module resolver picks this over"default"automatically, so bundling behavior is unchanged (verified byte-identical: 1756 modules, 10.52MB, both with and without this fix, with zero pre-built orchestratordist/present) while non-Bun consumers of the published package still resolve to compileddist/as before.Verification
orchestrate <path> --targetPlatform=... --providerStrategy=localproduces byte-identical output on both the original static-import code and this fix (down to the exact same downstream git error for a nonexistent project path) — confirms the plugin's actual command/option wiring is unaffected, not just that it type-checks.tsc --noEmit: 736 errors vs 737 on baseline (one fewer — removed a.ts-extension-import error, not a new one).bun test ./src: 199 pass, 0 fail, including a new regression test confirming orchestrator is registered viaPluginRegistryduring defaultsetup(), and the existing "does not register built-in plugins more than once" test still passes for it.bun run build: succeeds, byte-identical module count/bundle size to baseline.oxfmt --check: clean.Test plan
'orchestrator') viaPluginRegistryduring defaultCli.setup(), with no--pluginflag neededorchestrateinvocation, before and aftertsc --noEmit/oxfmt --checkcleanbun test ./srcpassesbun run buildsucceeds with unchanged bundle size (confirms no new build-order dependency was introduced)