fix: restore remote-cli-pre-build/log-stream/post-build after the -m CliFunction dispatcher was retired - #148
Merged
Conversation
…CliFunction dispatcher was retired
Every remote (AWS/K8s) orchestrator build has been broken since this
package's own CLI entrypoint (src/cli.ts) was rewritten to yargs -
confirmed independently by both my own trace and a fresh audit agent's,
converging on the same root cause from different angles.
Root cause, traced end to end:
1. Integration Tests on main fail identically across the last 6+ runs
(Aug 22-25), on AWS/K8s/Local Docker Provider Tests specifically -
not the MockAWS/Rclone jobs, and not a MiniStack pull/readiness
failure (MiniStack starts and the S3 bucket creates successfully in
every failing run).
2. The real failure: `[WARN] Unknown argument: m` immediately precedes
`Build failed with exit code 1`. `[WARN]` is game-ci/cli's own
logger format (src/core/logger/index.ts), and Cli.handleFailure
(src/cli.ts) does exactly `log.warning(message); process.exit(1)`
on any yargs failure - this is that failure, not a random tool.
3. build-automation-workflow.ts invokes
`node ${builderPath} -m remote-cli-pre-build` (and, at 7 more call
sites, `-m remote-cli-log-stream` / `-m remote-cli-post-build`) for
every non-local-docker provider. `-m <name>` is a legacy dispatch
protocol: a `@CliFunction(name, ...)` decorator registers a static
method with CliFunctionsRepository, which some *other* file used to
read process.argv's `-m` value and invoke the matching one.
4. That dispatcher is gone. CliFunctionsRepository
(model/cli/cli-functions-repository.ts) is explicitly a "Bridge
file - stub" that only stores registrations - nothing calls
GetCliFunctions() against `-m` anywhere. This package's own bin
entry (src/cli.ts, "game-ci": "./dist/cli.js") was independently
rewritten to yargs subcommands too, with no `-m` support either.
5. start.sh (docker/index.ts) runs under `set -e`, so this isn't a
swallowed warning - it's fatal to the entire remote build.
remote-cli-pre-build isn't diagnostic-only, so simply deleting the call
site was never on the table: RemoteClient.setupRemoteClient() bootstraps
the actual git workspace (full clone, incremental sync, or
retained-workspace reuse) and runs before-build hooks - build-critical.
remote-cli-post-build pushes the Library/Build caches. remote-cli-log-stream
pipes build output into a log file (and, on K8s, echoes it to stdout for
kubectl logs).
Fix: three real yargs commands (matching this package's own existing
command-per-file convention under cli/commands/), registered in
src/cli.ts, replacing the `-m <name>` flag at all 9 call sites in
build-automation-workflow.ts with the plain positional command. Each
handler reads entirely from the environment via mapCliArgumentsToInput +
BuildParameters.create() - the original invocations took no CLI flags at
all (remote-cli-log-stream's one exception, --logFile, is preserved as a
real yargs option).
Also fixed, found while making the new commands' import chain actually
load under bun (not just tsc, which erases type-only imports at compile
time and never surfaces this): three files imported SyncState/SyncStrategy
(type-only exports) as regular values instead of `import type` -
services/sync/index.ts, services/sync/sync-state-manager.ts,
services/sync/incremental-sync-service.ts, and remote-client/index.ts
itself. This was a real, if previously dormant, bug - nothing in the old
CLI's dependency graph ever loaded remote-client/index.ts, so it never
surfaced until this fix made these modules load for the first time.
Verified: `game-ci --help` now lists all three commands; invoking
`remote-cli-pre-build` directly no longer hits "Unknown argument: m" and
proceeds into real setup logic (reaches a real `mkdir -p /data`, which
only fails here because this is a Windows dev sandbox, not the Linux
container the code assumes - expected, not a bug). tsc --noEmit clean.
144 tests passing across the touched areas (12 new command tests, plus
existing build-automation-workflow, sync, and mock-aws suites - the
closest thing to the AWS integration path this environment can exercise
without real AWS/Docker). Full suite: same 2 pre-existing, unrelated
flakes as before this change (cli-integration timing out under parallel
load - passes 9/9 in isolation - and orchestrator-rclone-steps, which
fails identically on a clean main checkout).
|
Warning Review limit reachedNext included review available in 48 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 selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesRemote CLI command flow
Type-only sync imports
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change restores the remote build commands without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BuildWorkflow
participant RemoteCLI
participant RemoteClient
BuildWorkflow->>RemoteCLI: invoke remote-cli-pre-build
RemoteCLI->>RemoteClient: initialize before engine build
BuildWorkflow->>RemoteCLI: invoke remote-cli-log-stream with log file
RemoteCLI->>RemoteClient: stream remote build output
BuildWorkflow->>RemoteCLI: invoke remote-cli-post-build
RemoteCLI->>RemoteClient: push post-build cache
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed bug context, root cause, implementation details, verification results, and test limitations. It does not use the exact Changes and Checklist headings, but it includes the required information in equivalent sections. ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
This suite has been the sole cause of flaky Integration Test failures on several PRs today (#144, #147, and this one) - always the same signature, just landing on a different parallel shard each run: FAIL src/cli/__tests__/cli-integration.test.ts > ... > exits 0 ... Error: Test timed out in 5000ms. Root cause: every test spawns a real `node --require ts-node/register/transpile-only` process to exercise the actual CLI end to end - genuinely slow (full TS transpilation plus this package's whole dependency graph), slower still under CI's parallel test-shard contention. runCli()'s own execFile call already allows up to 60s for that child process. But vitest's *test*-level timeout defaults to 5000ms regardless of what the child process itself is allowed - so the wrapping `it()` was timing out long before the process's real budget was ever exhausted. A stale comment ("Per-test timeout configured via vitest options at the file/describe level") describbrowsed an intent that was never actually implemented. Fix: describe(..., { timeout: 30_000 }, ...) applies a realistic per-suite default. Verified the option is genuinely honored, not silently ignored, by temporarily setting it to 1ms - all 9 tests failed instantly (583ms total vs the normal ~30s), confirming this actually controls vitest's timeout rather than being dead configuration. 30s comfortably covers real CI contention without masking an actual hang, which would still exceed it. Passes reliably now; typecheck 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.
The bug
Every remote (AWS/K8s) orchestrator build has been broken on
mainfor at least 2+ days (confirmed back to Aug 22).Integration Testsfails identically across the last 6+ runs, and only onAWS Provider Tests/K8s Provider Tests/Local Docker Provider Tests— not theMockAWS/Rclonejobs. I traced this independently, and a fresh audit agent (no shared context) converged on the exact same root cause from a different angle.It is not a MiniStack flake — MiniStack starts and the S3 bucket creates successfully in every failing run (
make_bucket: game-ci-team-pipelines). The real failure:[WARN]isgame-ci/cli's own logger format (src/core/logger/index.ts), andCli.handleFailuredoes exactlylog.warning(message); process.exit(1)on any yargs failure — this is that failure, not some unrelated tool.Root cause
build-automation-workflow.tsinvokesnode ${builderPath} -m remote-cli-pre-build(and 7 more call sites for-m remote-cli-log-stream/-m remote-cli-post-build) for every non-local-docker provider.-m <name>is a legacy dispatch protocol: a@CliFunction(name, ...)decorator registers a static method withCliFunctionsRepository, which some other file used to readprocess.argv's-mvalue and invoke the matching one.That dispatcher is gone.
CliFunctionsRepositoryis explicitly a "Bridge file — stub" that only stores registrations — nothing callsGetCliFunctions()against-manywhere. This package's own bin entry (src/cli.ts) was independently rewritten to yargs subcommands too, with no-msupport either.start.shruns underset -e, so this isn't a swallowed warning — it kills the entire remote build.Why this needed real commands, not a deletion
remote-cli-pre-buildisn't diagnostic-only:RemoteClient.setupRemoteClient()bootstraps the actual git workspace (full clone, incremental sync, or retained-workspace reuse) and runsbefore-buildhooks — build-critical.remote-cli-post-buildpushes the Library/Build caches.remote-cli-log-streampipes build output into a log file (and, on K8s, echoes it to stdout forkubectl logs).Fix
Three real yargs commands, matching this package's own existing command-per-file convention under
cli/commands/, registered insrc/cli.ts. All 9 call sites inbuild-automation-workflow.tsupdated from-m <name>to the plain positional command. Each handler reads entirely from the environment viamapCliArgumentsToInput+BuildParameters.create()— the original bare invocations took no CLI flags at all (remote-cli-log-stream's one exception,--logFile, is preserved as a real yargs option).A second bug found while verifying this under bun (not just tsc)
Three files imported
SyncState/SyncStrategy(type-only exports) as regular values instead ofimport type:services/sync/index.ts,services/sync/sync-state-manager.ts,services/sync/incremental-sync-service.ts, andremote-client/index.tsitself. tsc erases type-only imports at compile time and never catches this; bun's isolated-module transpiler does not, and threwSyntaxError: export 'SyncStrategy' not foundthe moment these modules first actually loaded. This was a real, pre-existing latent bug — nothing in the old CLI's dependency graph ever loadedremote-client/index.ts, so it never surfaced until this fix made these modules load for the first time.Verification
game-ci --helplists all three commands.remote-cli-pre-builddirectly no longer hitsUnknown argument: mand proceeds into real setup logic — reaches a genuinemkdir -p /data, which only fails here because this is a Windows dev sandbox, not the Linux container the code assumes (expected, not a bug).tsc --noEmitclean.build-automation-workflow,sync, andmock-awssuites — the closest thing to the AWS integration path this environment can exercise without real AWS/Docker.cli-integrationtiming out under parallel load — passes 9/9 in isolation — andorchestrator-rclone-steps, which fails identically on a cleanmaincheckout).I don't have Docker/real AWS/K8s access in this environment, so I could not run the actual
Integration Testsworkflow end-to-end. Everything short of that has been verified directly.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation