fix(gateway): extract program from side-effectful main#662
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
Clean structural refactor — the split is sensible and the test simplification is a clear win. One correctness issue needs to be addressed before merge.
Blocking issues
** is now exported from but still does not use it for the startup-failure log path.**
In the catch handler after Effect.runPromise writes a raw console.error call that reconstructs the JSON format by hand:
console.error(JSON.stringify({level: 'error', err: String(error), msg: 'gateway startup failed'}))This diverges from the logger format emitted by makeLogger (which spreads a ctx object), so the startup-failure log will look structurally different from every other log line the gateway emits. That's a correctness issue for any log-aggregation pipeline that depends on a uniform schema.
This code path existed before the PR, so it's not a regression introduced here — but the refactor is the natural point to fix it. The simplest option is to call makeLogger('error') at the top of main.ts and use it in the catch handler, which keeps the format consistent without pulling in any new dependencies.
If the intent is to leave this as a known limitation (config may not be loaded, so log level is unknown), a comment explaining that choice would make it non-blocking.
Non-blocking concerns
-
** is exported** — the comment above the function in
program.tssays it's exported "for unit testing only", but there are no tests formakeLoggeritself. If it stays exported, consider adding a test for level-filtering behavior (i.e.,debuglog is suppressed atwarnlevel). Minor — the logic is simple. -
makeDiscordClientFromConfigis imported intomain.tsbut the dependency direction now runsmain → program → discord/client— this is fine, just noting thatmain.tsimportsmakeDiscordClientFromConfigfromprogram.tsonly to pass it back as themakeClientdep. That's slightly indirect; a future reader might wonder ifmaincould just passcreateDiscordClientdirectly. No action needed.
Missing tests
- No new test coverage for the thin
main.tsrunner itself (startup-failure path,process.exit(1)). The PR description acknowledges test count is unchanged. This is acceptable — testingEffect.runPromise+process.exitin isolation would require mocking exactly what the old tests were fighting. Non-blocking.
Risk assessment: LOW
Pure code movement, no behavior change. The 154-test baseline passes per the PR description. The one structural concern (log format inconsistency in the startup-failure path) exists in the current codebase already and carries no new runtime risk.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26338482790 |
| Cache | hit |
| Session | ses_1aa36609fffebcUV1QkB5JVAoV |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
Clean structural refactor — the split is sensible and the test simplification is a clear win. One correctness issue needs to be addressed before merge.
Blocking issues
The startup-failure log in main.ts reconstructs the JSON format by hand instead of using the now-exported makeLogger.
The catch handler after Effect.runPromise in main.ts writes:
console.error(JSON.stringify({level: 'error', err: String(error), msg: 'gateway startup failed'}))makeLogger spreads ctx first then appends msg, so startup-failure entries look structurally different from every other log line. This breaks log-aggregation pipelines relying on a uniform schema.
This code path pre-existed, so it is not a new regression — but the refactor is the natural fix point. The simplest correction is to instantiate makeLogger('error') at the top of main.ts and use it in the catch handler. If the intent is to leave it as a known limitation (log level is unknown before config loads), a brief comment explaining that choice would make this non-blocking.
Non-blocking concerns
-
makeLoggeris exported fromprogram.tswith a comment saying "for unit testing only", but there are no tests for it. Level-filtering behavior (debugsuppressed atwarn) would be worth a test if the export stays. Minor — the logic is simple. -
main.tsimportsmakeDiscordClientFromConfigfromprogram.tsonly to immediately pass it back as themakeClientdep. The indirection is harmless; a future reader might wonder whycreateDiscordClientis not passed directly. No action needed.
Missing tests
No new tests for the thin main.ts runner (startup-failure path, process.exit(1)). The PR description acknowledges the test count is unchanged. Acceptable — testing Effect.runPromise + process.exit would reintroduce the mocking gymnastics this PR removes.
Risk assessment (LOW): likelihood of regression, security exposure, or blast radius
Pure code movement with no behavior change. The 154-test baseline passes per the PR description. The one structural concern (log format inconsistency on startup failure) pre-exists in the codebase and carries no new runtime risk.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26338482790 |
| Cache | hit |
| Session | ses_1aa36609fffebcUV1QkB5JVAoV |
2a108bd to
1c8655d
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
The blocking issue from the previous review has been addressed. main.ts now imports makeLogger from program.ts and uses it in the startup-failure catch handler, with a comment explaining why 'error' is hardcoded (config may not have loaded). Log format is now consistent across all paths.
Blocking issues
None.
Non-blocking concerns
None (prior non-blocking notes stand but require no action — makeLogger level-filtering is simple enough to trust without a dedicated test, and the makeDiscordClientFromConfig indirection is an intentional DI pattern).
Missing tests
None beyond what was noted previously. The 154-test baseline is unchanged and the test simplification (dropping the Effect.runPromise mock) is a genuine improvement.
Risk assessment (LOW): likelihood of regression, security exposure, or blast radius
Pure code movement plus a one-line improvement to the startup-failure log path. No behavior change to the happy path. No new dependencies introduced.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26338744425 |
| Cache | hit |
| Session | ses_1aa36609fffebcUV1QkB5JVAoV |
1c8655d to
7920faf
Compare
|
No new commits since the previous approval (run 26338744425). The PR remains in PASS state — no further action required. Run Summary
|
Moves makeGatewayProgram, GatewayProgramDeps, and the structured logger into a new side-effect-free program.ts module. main.ts becomes a thin runner; program.test.ts now exercises the program directly without mocking Effect.runPromise. No behavior change.
7920faf to
7aa9035
Compare
|
No new commits since the approval (run 26338744425). PR remains in PASS state. Run Summary
|
Moves
makeGatewayProgram,GatewayProgramDeps, and the structured logger into a newprogram.tsmodule.main.tsbecomes a thin runner that builds and runs the program. No behavior change.Why:
main.tswas both the runner (top-levelEffect.runPromise) and the source of the testable factory exports.main.test.tshad to mockEffect.runPromiseat module load to prevent the gateway from actually starting on import, and any test that wanted the realEffectruntime had to escape withvi.importActual('effect').What changed:
packages/gateway/src/program.ts(new) —makeLogger,makeDiscordClientFromConfig,GatewayProgramDeps,makeGatewayProgram. Side-effect-free.packages/gateway/src/main.ts— thin runner. ImportsmakeGatewayProgramfrom./program.js, runs it, handles startup failure via the samemakeLoggerso the startup-failure log line matches every other gateway log.packages/gateway/src/program.test.ts(renamed frommain.test.ts) — drops theEffect.runPromisemock entirely. CallsEffect.runPromise(makeGatewayProgram(...))directly.154/154 gateway tests pass (same count — no tests added or removed, just relocated). Build still emits
dist/main.mjscorrectly.